feat(collections): add "union" to DeepMergeOptions.arrays#7049
feat(collections): add "union" to DeepMergeOptions.arrays#7049DonBLong wants to merge 1 commit intodenoland:mainfrom
"union" to DeepMergeOptions.arrays#7049Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7049 +/- ##
=======================================
Coverage 94.38% 94.38%
=======================================
Files 628 628
Lines 50178 50181 +3
Branches 8840 8841 +1
=======================================
+ Hits 47360 47363 +3
Misses 2251 2251
Partials 567 567 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
444e16e to
d30e9e7
Compare
bartlomieju
left a comment
There was a problem hiding this comment.
The new unstable_deep_merge.ts (598 lines) is a near-verbatim copy of the existing deep_merge.ts (592 lines). The only meaningful differences are:
import { union } from "./union.ts"addedArraysMergingStrategytype added (MergingStrategy | "union")DeepMergeOptions.arrayschanged fromMergingStrategytoArraysMergingStrategy- 3 lines in
mergeObjects()to handle the"union"case
Everything else — function signatures, JSDoc, all type utilities, helper functions — is duplicated. Maintaining two near-identical 600-line files is not sustainable.
This should instead modify the existing deep_merge.ts directly, since the change is purely additive (adding a new valid string literal to the arrays option). Existing callers are unaffected.
Additional issues
JSDoc examples import from wrong path — All doc examples import from "@std/collections/deep-merge" instead of "@std/collections/unstable-deep-merge", so doc tests would either fail or test the wrong module.
Merge type doesn't handle "union" for arrays — The Merge type only checks for { arrays: "replace" }. When arrays: "union" is passed, the type falls through to MergeAllArrays<T, U>, which happens to be correct by coincidence, but the intent isn't expressed.
Test file also duplicates existing tests — The test file (494 lines) copies most tests from deep_merge_test.ts. Only ~2 tests are actually new (the "union" merge test and the default strategy test).
Suggestion
Instead of a new file, add the "union" strategy to the existing deep_merge.ts:
- Add
ArraysMergingStrategy = MergingStrategy | "union" - Change
DeepMergeOptions.arraystype toArraysMergingStrategy - Add the 3-line
unionbranch inmergeObjects() - Add the union-specific tests to the existing test file
If it needs to stay unstable, the new type can be exported from an unstable_ entrypoint that re-exports and extends the stable module.
Adds a `"union"` `MergingStrategy` for `arrays` only, through a new exported type `ArraysMergingStrategy`. Implementation uses the `union` `function` imported from `"./union.ts"`
d30e9e7 to
462218c
Compare
"union" to DeepMergeOptions.arrays"union" to DeepMergeOptions.arrays
My bad. It's my first time contributing to this repo and I have misunderstood the contributing guides. I thought "new APIs" meant any addition to a stable API. I also thought that the maintainers would want to compare the diff between the I took your first suggestion, and applied the changes to the original I also modified the commit message to reflect that the feature is being added directly to the stable |
Note TL;DR The reason why I didn't touch the About that thoughIt's funny that you mentioned this type, because that's what confused me about the return of const a = { foo: [1, 2, 3] };
const b = { foo: [3, 4, 5] };
const abFoo = a.foo.concat(b.foo);The type of However, according to the const a = { foo: [1, 2, 3] };
const b = { foo: ["string", 4, 5] };
const abFoo = a.foo.concat(b.foo); // what deepMerge(a, b, { arrays: "merge" }) does internallyBecause the type of I thought that const a = { foo: [1, 2, 3] };
const b = { foo: ["string", 3, 4] };
const abFoo = union(a.foo, b.foo);
// OR: const abFoo = Array.from(new Set(a.foo).union(new Set(b.foo)));
assertEquals(abFoo, [1, 2, 3, "string", 4]);ConclusionI think the confusion comes from the name |
Adds a
"union"MergingStrategyforarraysonly, through a new exported typeArraysMergingStrategy. Implementation uses theunionfunctionimported from"./union.ts"closes: #7048