fix(utils): treat explicit undefined as an override in mergeDeep#8302
Conversation
…mergeDeep Previously the top-level `if (source === undefined) continue` (added for PR ardatan#8204's null handling) also fired when a key's value was explicitly undefined, since the by-key merge recursed through the same mergeDeep call. That silently discarded the override and kept the prior value, indistinguishable from the key being absent entirely. Now the by-key merge only recurses when the incoming value isn't undefined; an explicit undefined is assigned directly, so it overrides like null does, while an absent key (never visited by `for...in`) is still left untouched.
🦋 Changeset detectedLatest commit: 2b28751 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesmergeDeep undefined handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
|
@enisdenjo This PR is similar to #8204 which you just approved. I didn't realize that this issue was also a blocking issue for me, but uncovered after upgrading to the latest release. |
|
thank you for the PR! will be releasing sooon 🙏 |
2 participants
Summary
Follow-up to #8204, which made
mergeDeeptreat an explicitnullproperty value as an override rather than skipping it. This does the same for an explicitundefinedproperty value, while keeping it distinct from the property being absent entirely.Before this change,
mergeDeep([{ a: 'foo' }, { a: undefined }])returned{ a: 'foo' }— the explicitundefinedhad no effect, indistinguishable frommergeDeep([{ a: 'foo' }, {}]). This is surprising for consumers (e.g. test mock builders) that rely onundefinedto explicitly clear/override a field.Details
The by-key merge loop recursed into
mergeDeep([output[key], source[key]])for every key present onsource, including ones whose value was explicitlyundefined. That recursive call re-triggered the top-levelif (source === undefined) continue, silently discarding the override and keeping the prior value — the same skip that intentionally lets a wholeundefinedsource in the top-levelsourcesarray be ignored (e.g.mergeDeep([defaults, options])whereoptionswasn't provided).The fix only recurses when the incoming value isn't
undefined; an explicitundefinedis assigned directly. This keeps the "whole source isundefined" skip (still exercised by the existing'skips undefined sources'test) fully intact, while making an explicitly-present-but-undefinedkey override likenulldoes. A key that's absent fromsourceis never visited byfor...in, so it's naturally left untouched — distinct from the explicit-undefinedcase, per the added tests.Tests
undefinedproperty value overrides a previously merged value ('a' in mergedistrueandmerged.aisundefined).utilsandmergepackage test suites (353 tests) — all passing.