fix: handle null in isJSONSerializable to prevent TypeError#574
fix: handle null in isJSONSerializable to prevent TypeError#574Zelys-DFKH wants to merge 2 commits into
Conversation
Fixes unjs#571 - isJSONSerializable crashed when passed null because: 1. Line 22 checked 't === null' instead of 'value === null' 2. typeof null is 'object', so the dead code check never triggered 3. Line 28 then accessed value.buffer on null, throwing TypeError Solution: Add explicit null guard in early return, alongside undefined check. Also removed incorrect 't === null' condition since typeof returns string. Added regression tests for null, undefined, and other edge cases.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR makes isJSONSerializable return false for both undefined and null and adds a Vitest suite verifying behavior for null, undefined, primitives, plain objects/arrays, Uint8Array/ArrayBuffer, FormData, and URLSearchParams. ChangesisJSONSerializable Bug Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@test/index.test.ts`:
- Around line 558-561: The test description is misleading because ArrayBuffer
doesn't have a .buffer property and so the isJSONSerializable branch checking
value.buffer isn't exercised; update the test(s) so one explicitly uses a
TypedArray (e.g., new Uint8Array(10)) to hit the value.buffer branch and assert
isJSONSerializable(...) === false, and either rename the existing ArrayBuffer
test to reflect "returns false for ArrayBuffer" or split into two tests: one for
TypedArray (exercises value.buffer) and one for ArrayBuffer (verifies
non-serializable via constructor check) while keeping references to the
isJSONSerializable function in the test names for clarity.
- Line 538: Add the same ESLint suppression used elsewhere for null to the
failing assertion: before the expect(isJSONSerializable(null)) call add an
inline comment to disable the unicorn/no-null rule for the next line so CI won't
fail; locate the expect call referencing isJSONSerializable and add the same "//
eslint-disable-next-line unicorn/no-null" style suppression used at lines 58 and
208.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e89410c-737a-4086-b1cf-7a91ad5029ca
📒 Files selected for processing (2)
src/utils.tstest/index.test.ts
- Add ESLint suppression for null in isJSONSerializable test (line 538) - Split misleading ArrayBuffer test into two tests: - TypedArray test that exercises the .buffer property check - ArrayBuffer test that verifies non-serializable via constructor check Fixes CodeRabbit comments on PR unjs#574.
Fixes #571 —
isJSONSerializablecrashes withTypeErrorwhen passednull.Root cause:
t === nullinstead ofvalue === nulltypeof null === "object"(JavaScript quirk), this condition never matches (dead code)if (value.buffer)→ crashes on nullSolution:
t === nullcondition (typeof always returns a string, never "null")Changes:
src/utils.ts: 2-line fix (add null guard, remove dead condition)test/index.test.ts: 6 regression tests covering null, undefined, primitives, objects, special casesTest results: 34/34 passing ✅
Summary by CodeRabbit
Bug Fixes
Tests