fix: correct isJSONSerializable to handle null without throwing#572
fix: correct isJSONSerializable to handle null without throwing#572EduardF1 wants to merge 2 commits into
Conversation
|
@EduardF1 I'm the creator of the issue and this is exactly what fix I had in mind. LGTM Note: Another partially related thing is that the body serialization logic of the fetch here doesn't allow to have a body of |
|
@magion33 thanks for the LGTM, and good catch on the body-serialization gap. Looking at https://github.com/unjs/ofetch/blob/main/src/fetch.ts#L130: if (context.options.body && isPayloadMethod(context.options.method)) {The leading I'd prefer to keep this PR narrowly focused on the dead-code/throws-on-null bug so it's easy for maintainers to review and merge, and then land the body-serialization fix as a follow-up that references this one. Happy to open that follow-up PR right after this merges (or in parallel if a maintainer signals it's fine to bundle). |
|
@EduardF1 Yes, I agree. It's a different topic, just slightly related. |
SummaryFixes Status
@pi0 happy to adjust scope or naming if needed. Thanks! |
|
Thanks again @magion33 for validating the fix and for agreeing to keep the body-serialization case separate. I am happy to send that follow-up on its own after this one lands if maintainers want it. |
|
Hi team, checking in on this PR! Let me know if you have any questions. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughFixes a bug in null serialization fix
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #572 +/- ##
===========================================
+ Coverage 56.86% 85.55% +28.68%
===========================================
Files 16 5 -11
Lines 728 263 -465
Branches 113 133 +20
===========================================
- Hits 414 225 -189
+ Misses 303 33 -270
+ Partials 11 5 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Problem
isJSONSerializable(null) throws a TypeError and incorrectly returns
alse.
Root cause
Two bugs on the same line:
ull. So === null can never be rue.
ull, execution continues to
alue.buffer, which throws TypeError: Cannot read properties of null (reading 'buffer').
Verified:
ull is valid JSON (JSON.stringify(null) → "null"), so the function should return rue for it.
Fix
Replace === null with
alue === null:
This correctly checks the value itself rather than its ypeof result, so
ull hits the early
eturn true before reaching
alue.buffer.
Fixes #571
Summary by CodeRabbit
nullis now correctly treated as a valid JSON value.nullreturnstruein the JSON serializability check.