Skip to content

feat(collections): option to merge undefined in deepMerge #6522

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+166 −2
Open

feat(collections): option to merge undefined in deepMerge #6522

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1908bf9
Add deepMerge option: undefined
WWRS Mar 27, 2025
88605c0
Rename undefined to undefineds
WWRS Apr 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions collections/deep_merge.ts
View file
Original file line number Diff line number Diff line change
@@ -208,6 +208,7 @@ export function deepMerge<
arrays: "merge";
sets: "merge";
maps: "merge";
undefineds: "replace";
},
>(
record: Readonly<T>,
@@ -224,14 +225,14 @@ function deepMergeInternal<
arrays: "merge";
sets: "merge";
maps: "merge";
undefineds: "replace";
},
>(
record: Readonly<T>,
other: Readonly<U>,
seen: Set<NonNullable<unknown>>,
options?: Readonly<Options>,
) {
// Extract options
// Clone left operand to avoid performing mutations in-place
type Result = DeepMerge<T, U, Options>;
const result: Partial<Result> = {};
@@ -252,7 +253,10 @@ function deepMergeInternal<

const a = record[key] as ResultMember;

if (!Object.hasOwn(other, key)) {
if (
!Object.hasOwn(other, key) ||
(other[key] === undefined && options?.undefineds === "ignore")
) {
result[key] = a;

continue;
@@ -285,6 +289,7 @@ function mergeObjects(
arrays: "merge",
sets: "merge",
maps: "merge",
undefineds: "replace",
},
): Readonly<NonNullable<Record<string, unknown> | Iterable<unknown>>> {
// Recursively merge mergeable objects
@@ -387,6 +392,18 @@ export type DeepMergeOptions = {
* @default {"merge"}
*/
sets?: MergingStrategy;

/**
* How to handle comparisons between non-`undefined` values and `undefined`.
*
* - If `"replace"`, the value in `other` is always chosen.
* - If `"ignore"`, the value in `other` is only chosen if not `undefined`.
*
* In both cases, a value of `undefined` is chosen over an omitted value.
*
* @default {"replace"}
*/
undefineds?: "replace" | "ignore";
Copy link
Member

@kt3k kt3k Apr 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WWRS Sorry for late reply, but undefineds sounds a bit strange to me. Because this affects only when undefined appears as object property, how about undefinedProp, undefinedProperty, or propertyUndefined? cc @ry

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just above, the options are named arrays, sets, and maps, so I think undefineds does match with those most closely

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the point, but the usage of the word undefineds seems very rare compared to sets, maps, arrays. I'm afraid this might be confusing to the users. (I personally prefer the original undefined over undefineds. undefined is not JavaScript keyword (ref), but just predefined variable name. I personally disagree with @ry's concern..)

};

/**
147 changes: 147 additions & 0 deletions collections/deep_merge_test.ts
View file
Original file line number Diff line number Diff line change
@@ -428,3 +428,150 @@ Deno.test("deepMerge() handles target object is not modified", () => {
quux: new Set([1, 2, 3]),
});
});

Deno.test("deepMerge() handles number vs undefined", () => {
assertEquals(
deepMerge<{ a: number | undefined }>(
{ a: 1 },
{ a: undefined },
{ undefineds: "ignore" },
),
{ a: 1 },
);
assertEquals(
deepMerge(
{ a: 1 },
{ a: undefined },
{ undefineds: "replace" },
),
{ a: undefined },
);
assertEquals(
deepMerge(
{ a: 1 },
{ a: undefined },
// Default is replace
),
{ a: undefined },
);
assertEquals(
deepMerge(
{ a: undefined },
{ a: 1 },
{ undefineds: "ignore" },
),
{ a: 1 },
);
assertEquals(
deepMerge(
{ a: undefined },
{ a: 1 },
{ undefineds: "replace" },
),
{ a: 1 },
);
assertEquals(
deepMerge(
{ a: undefined },
{ a: 1 },
// Default is replace
),
{ a: 1 },
);

assertEquals(
deepMerge(
{ a: undefined },
{ a: undefined },
{ undefineds: "ignore" },
),
{ a: undefined },
);
assertEquals(
deepMerge(
{ a: undefined },
{ a: undefined },
{ undefineds: "replace" },
),
{ a: undefined },
);
assertEquals(
deepMerge(
{ a: undefined },
{ a: undefined },
// Default is replace
),
{ a: undefined },
);
});

Deno.test("deepMerge() handles mergeable vs undefined", () => {
assertEquals<{ a: { b: number } | undefined }>(
deepMerge(
{ a: { b: 1 } },
{ a: undefined },
{ undefineds: "ignore" },
),
{ a: { b: 1 } },
);
assertEquals(
deepMerge(
{ a: { b: 1 } },
{ a: undefined },
{ undefineds: "replace" },
),
{ a: undefined },
);

assertEquals(
deepMerge<{ a: { b: number; c: number | undefined } }>(
{ a: { b: 1, c: 2 } },
{ a: { b: 1, c: undefined } },
{ undefineds: "ignore" },
),
{ a: { b: 1, c: 2 } },
);
assertEquals(
deepMerge(
{ a: { b: 1, c: 2 } },
{ a: { b: 1, c: undefined } },
{ undefineds: "replace" },
),
{ a: { b: 1, c: undefined } },
);
});

Deno.test("deepMerge() handles undefined vs omitted", () => {
assertEquals(
deepMerge(
{ a: undefined },
{},
{ undefineds: "ignore" },
),
{ a: undefined },
);
assertEquals(
deepMerge(
{ a: undefined },
{},
{ undefineds: "replace" },
),
{ a: undefined },
);
assertEquals(
deepMerge(
{},
{ a: undefined },
{ undefineds: "ignore" },
),
{ a: undefined },
);
assertEquals(
deepMerge(
{},
{ a: undefined },
{ undefineds: "replace" },
),
{ a: undefined },
);
});
Loading