fix(utils): treat null as explicit value in mergeDeep, only skip undefined#8204
Open
pbomb wants to merge 4 commits into
Open
fix(utils): treat null as explicit value in mergeDeep, only skip undefined#8204pbomb wants to merge 4 commits into
pbomb wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: 3e738d0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR distinguishes ChangesNull Value Handling in mergeDeep
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/utils/tests/mergeDeep.test.ts (1)
76-78: 💤 Low valueConsider renaming this test for clarity.
The test name "filters nullish values" is now slightly misleading, as the fix ensures that
nullis no longer filtered—onlyundefinedis. The test itself is correct since it only includesundefinedin the test case.Consider renaming to something like "filters undefined values" or "skips undefined sources" to more accurately reflect the behavior after this fix.
🤖 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 `@packages/utils/tests/mergeDeep.test.ts` around lines 76 - 78, Rename the test description for the it(...) that asserts mergeDeep([{ a: 'dsa' }, { a: 'dd', b: 1 }, undefined]) to accurately reflect behavior (e.g. "filters undefined values" or "skips undefined sources") so the test name matches the implementation of mergeDeep; update the string passed to the it call that references mergeDeep in the test file accordingly.
🤖 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 `@packages/utils/tests/mergeDeep.test.ts`:
- Around line 76-78: Rename the test description for the it(...) that asserts
mergeDeep([{ a: 'dsa' }, { a: 'dd', b: 1 }, undefined]) to accurately reflect
behavior (e.g. "filters undefined values" or "skips undefined sources") so the
test name matches the implementation of mergeDeep; update the string passed to
the it call that references mergeDeep in the test file accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 640ed839-bfd1-43b2-9c0d-245a796a5970
📒 Files selected for processing (3)
.changeset/fix-merge-deep-null-handling.mdpackages/utils/src/mergeDeep.tspackages/utils/tests/mergeDeep.test.ts
Author
|
Addressed in 3e738d0 — the test was already renamed to |
Author
|
@coderabbitai help |
Contributor
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/utils/tests/mergeDeep.test.ts (1)
80-86: ⚡ Quick winAdd one regression test for top-level
nullsource overwrite.The new tests cover property overrides, but they don’t pin the top-level behavior change described by this PR (
mergeDeep([{ a: 2 }, null]) === null). Please add that case so the full null-vs-undefined contract is protected.🤖 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 `@packages/utils/tests/mergeDeep.test.ts` around lines 80 - 86, Add a regression test in packages/utils/tests/mergeDeep.test.ts that asserts mergeDeep([{ a: 2 }, null]) returns null to protect the top-level null-vs-undefined contract; locate the existing tests around the mergeDeep spec (the tests that check property overrides) and add a new it(...) case that calls mergeDeep with the array [{ a: 2 }, null] and expects the result toEqual(null)..changeset/fix-merge-deep-null-handling.md (1)
5-5: ⚡ Quick winClarify changeset wording for full behavior change.
The note currently mentions property values only. Consider adding that
nullas a source is now treated as an explicit value too (while onlyundefinedis skipped), to match the shipped behavior.🤖 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 @.changeset/fix-merge-deep-null-handling.md at line 5, Update the changeset text to explicitly state that mergeDeep now treats null as an explicit source value (i.e., null will overwrite target properties) while undefined continues to be skipped; reference the mergeDeep behavior change by name and clarify that "null property values are treated as explicit overrides rather than skipped" so readers know null is preserved/assigned and only undefined is ignored.
🤖 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 @.changeset/fix-merge-deep-null-handling.md:
- Line 5: Update the changeset text to explicitly state that mergeDeep now
treats null as an explicit source value (i.e., null will overwrite target
properties) while undefined continues to be skipped; reference the mergeDeep
behavior change by name and clarify that "null property values are treated as
explicit overrides rather than skipped" so readers know null is
preserved/assigned and only undefined is ignored.
In `@packages/utils/tests/mergeDeep.test.ts`:
- Around line 80-86: Add a regression test in
packages/utils/tests/mergeDeep.test.ts that asserts mergeDeep([{ a: 2 }, null])
returns null to protect the top-level null-vs-undefined contract; locate the
existing tests around the mergeDeep spec (the tests that check property
overrides) and add a new it(...) case that calls mergeDeep with the array [{ a:
2 }, null] and expects the result toEqual(null).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 147991b1-4196-4135-9239-a6e3243b472e
📒 Files selected for processing (3)
.changeset/fix-merge-deep-null-handling.mdpackages/utils/src/mergeDeep.tspackages/utils/tests/mergeDeep.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #8197
mergeDeep([{ a: 'foo' }, { a: null }])now correctly returns{ a: null }instead of{ a: 'foo' }.Root cause
The guard
if (source == null) { continue; }uses loose equality (==), which skips bothnullandundefinedsources. The intent was to skipundefined(meaning "not provided"), butnullis an explicit value and should override earlier values.Fix
Change
source == nulltosource === undefined. One-line change inpackages/utils/src/mergeDeep.ts.Note on behavior change:
mergeDeep([{a: 2}, null])now returnsnullrather than{a: 2}. This is the correct semantic —nullis an explicit "replace with nothing", distinct fromundefinedwhich means "no value provided".Tests
packages/utils/tests/mergeDeep.test.tsGenerated with Claude Code