fix: merge symbol keys instead of overwriting them#181
Conversation
`_defu` iterated only `Object.keys(baseObject)`, which skips symbol keys,
while `{ ...defaults }` copies them via spread. As a result symbol-keyed
properties from later (lower-priority) arguments overwrote earlier ones
instead of being merged.
Add a `getOwnEnumerableKeys` helper that returns own enumerable keys
including symbols (mirroring object-spread semantics) and use it when
iterating the base object. Non-enumerable symbols stay excluded.
Fixes unjs#145
|
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 (1)
📝 WalkthroughWalkthrough
ChangesSymbol-key merge support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/defu.test.ts (1)
138-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a symbol-keyed custom merger regression.
These tests cover recursion and array concatenation, but not the
merger(object, key, value, namespace)branch insrc/defu.tsLine 26. Since symbol enumeration now changes that path too, a small callback-specific test would lock down the remaining contract from issue#145.🤖 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/defu.test.ts` around lines 138 - 165, Add a regression test for the symbol-keyed custom merger path in defu, since the current symbol tests only cover default recursion and array merging. Extend test/defu.test.ts with a case that uses the defu custom merger callback path from src/defu.ts (the merger(object, key, value, namespace) branch) and verifies symbol keys are still handled correctly when symbols are enumerated. Keep the test focused on the existing defu symbol behavior and ensure it guards the callback-specific contract from issue `#145`.
🤖 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/defu.test.ts`:
- Around line 138-165: Add a regression test for the symbol-keyed custom merger
path in defu, since the current symbol tests only cover default recursion and
array merging. Extend test/defu.test.ts with a case that uses the defu custom
merger callback path from src/defu.ts (the merger(object, key, value, namespace)
branch) and verifies symbol keys are still handled correctly when symbols are
enumerated. Keep the test focused on the existing defu symbol behavior and
ensure it guards the callback-specific contract from issue `#145`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43ba4f10-a10c-4947-9250-b649e74f9e9e
📒 Files selected for processing (3)
src/_utils.tssrc/defu.tstest/defu.test.ts
Summary
Fixes #145.
_defuiterated over the base object withObject.keys(baseObject), which skips symbol keys. The result object is built from{ ...defaults }, and spread does copy enumerable symbol keys. So symbol-keyed properties from later (lower-priority) arguments overwrote earlier (higher-priority) ones instead of being merged.Changes
getOwnEnumerableKeystosrc/_utils.ts— returns own enumerable keys including symbols, mirroring object-spread semantics. Non-enumerable symbols stay excluded (consistent with howObject.keysexcludes non-enumerable string keys and with the spread used to build the result)._defuinstead ofObject.keys.This keeps recursive merging, array concat, and the merger callback working for symbol keys too.
Tests
Added three regression tests to
test/defu.test.ts:All checks pass locally:
pnpm lint,pnpm test:types, andpnpm vitest run(26 tests).Summary by CodeRabbit
New Features
Bug Fixes
Tests