Skip to content

fix: handle null and FormData order in isJSONSerializable#589

Closed
leno23 wants to merge 2 commits into
unjs:mainfrom
leno23:fix/is-json-serializable-order-580
Closed

fix: handle null and FormData order in isJSONSerializable#589
leno23 wants to merge 2 commits into
unjs:mainfrom
leno23:fix/is-json-serializable-order-580

Conversation

@leno23
Copy link
Copy Markdown

@leno23 leno23 commented May 25, 2026

Summary

  • Return true for null via explicit check (typeof null is "object")
  • Check FormData / URLSearchParams before value.buffer to avoid throws on empty instances

Example

isJSONSerializable(null); // true (no throw)
isJSONSerializable(new FormData()); // false

Fixes #571
Fixes #580

Test plan

  • pnpm test passes
  • Added regression tests for null and FormData

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced JSON serialization validation to properly handle null values and correctly process form-related objects.
  • Tests

    • Added comprehensive test coverage for JSON serialization detection across null, FormData, and URLSearchParams inputs.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 25, 2026

Warning

Review limit reached

@leno23, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 6 minutes and 24 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 570e7f5e-a952-4d73-adef-26ea178fb42d

📥 Commits

Reviewing files that changed from the base of the PR and between 598cfa7 and 9c657a9.

📒 Files selected for processing (2)
  • src/utils.ts
  • test/index.test.ts
📝 Walkthrough

Walkthrough

isJSONSerializable now returns true for null via an explicit early-return and rejects FormData/URLSearchParams before accessing value.buffer. A Vitest suite was added to assert false for FormData/URLSearchParams (empty and with entries) and true for null.

Changes

Type Check Reordering and Test Coverage

Layer / File(s) Summary
isJSONSerializable null short-circuit and instanceof reorder
src/utils.ts
Adds an explicit value === null early-return and moves FormData/URLSearchParams instanceof checks before the value.buffer check.
Vitest coverage for edge cases
test/index.test.ts
Adds tests that dynamically import isJSONSerializable and assert it returns false for FormData and URLSearchParams (including populated FormData) and true for null.

🎯 2 (Simple) | ⏱️ ~10 minutes

A rabbit hops through checks with care,
FormData and URLSearchParams beware!
Null is safe, buffer checks aligned,
Tests confirm the order we assigned. 🐰✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main changes: handling null correctly and reordering FormData checks before buffer property access in isJSONSerializable.
Linked Issues check ✅ Passed The PR addresses all coding requirements from issues #571 and #580: null is now JSON-serializable, FormData/URLSearchParams checks precede buffer checks, and regression tests are added.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fix isJSONSerializable logic and add related tests; no unrelated modifications are present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
test/index.test.ts (1)

537-544: ⚡ Quick win

Add a populated URLSearchParams assertion for symmetry.

This suite checks populated FormData but not populated URLSearchParams; adding one assertion strengthens regression coverage for the same branch behavior.

Proposed test addition
   it("returns false for FormData and URLSearchParams before buffer checks", () => {
     expect(isJSONSerializable(new FormData())).toBe(false);
     expect(isJSONSerializable(new URLSearchParams())).toBe(false);
+    expect(isJSONSerializable(new URLSearchParams({ foo: "bar" }))).toBe(false);

     const data = new FormData();
     data.append("foo", "bar");
     expect(isJSONSerializable(data)).toBe(false);
   });
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/index.test.ts` around lines 537 - 544, Add a populated URLSearchParams
assertion symmetric to the existing FormData check: in the test block that calls
isJSONSerializable for FormData and URLSearchParams, construct a URLSearchParams
instance, append a key/value (e.g., "foo", "bar"), and assert
isJSONSerializable(populatedUrlSearchParams) === false so the test covers the
populated-URLSearchParams branch alongside the populated-FormData check; locate
this in test/index.test.ts within the "returns false for FormData and
URLSearchParams before buffer checks" test that references isJSONSerializable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@test/index.test.ts`:
- Around line 537-544: Add a populated URLSearchParams assertion symmetric to
the existing FormData check: in the test block that calls isJSONSerializable for
FormData and URLSearchParams, construct a URLSearchParams instance, append a
key/value (e.g., "foo", "bar"), and assert
isJSONSerializable(populatedUrlSearchParams) === false so the test covers the
populated-URLSearchParams branch alongside the populated-FormData check; locate
this in test/index.test.ts within the "returns false for FormData and
URLSearchParams before buffer checks" test that references isJSONSerializable.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0f015ae0-802e-43fc-b28f-9808217af2bd

📥 Commits

Reviewing files that changed from the base of the PR and between dfbe3ca and 492b285.

📒 Files selected for processing (2)
  • src/utils.ts
  • test/index.test.ts

Treat null as JSON-serializable via explicit check (typeof null is
"object") and probe FormData/URLSearchParams before value.buffer.

Fixes unjs#571
Fixes unjs#580
@pi0 pi0 closed this May 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: isJSONSerializable returns false for empty FormData and URLSearchParams isJSONSerializable bug

2 participants