Fix inconsistent merge onto Destination's optional entries and better handling of undefined vs optional entries

[jclaveau]

As show here #1208: MergeDeep overwrites Destination's nested entries when they are optional

This PR fixes and test this merging issue.

Moreover, optional entries notion is deeply entangled with undefined as { entry?: string | undefined } is redundant but { entry: string | undefined } is not.

So this PR also implements the tests showing the issues the TS behavior generates.

I'm presently trying to implement a way to merge optional entries VS possible undefined values the way the most intuitive I can, but I still wonder if a configuration variable with optional merging strategies will be required:

"Currently" in the following code means, including the commit 22b0e18

type OptionalAndUndefinedNestedTest = {
	Left: {
		nested?: {
			in_left: string;
		} | undefined; // Redundancy is allowed and preserved by TS
	};
	Right: {
		nested: {
			in_right: string;
		};
	};
};

type OptionalAndUndefinedNestedRightIntoLeft = MergeDeep<
	OptionalAndUndefinedNestedTest['Left'],
	OptionalAndUndefinedNestedTest['Right']
>;
expectTypeOf<OptionalAndUndefinedNestedRightIntoLeft>().toEqualTypeOf<{
	nested: {
		in_left: string;
		in_right: string;
	} | undefined; // Currently, this " | undefined" is removed and IMHO shouldn't as it's on the value part
}>();

type OptionalAndUndefinedNestedRightIntoRight = MergeDeep<
	OptionalAndUndefinedNestedTest['Right'],
	OptionalAndUndefinedNestedTest['Left']
>;
expectTypeOf<OptionalAndUndefinedNestedRightIntoRight>().toEqualTypeOf<{
	nested?: {
		in_left: string;
		in_right: string;
	} | undefined;  // Currently this undefined is not kept but, IMHO, it should be (in case of further merge for example)
}>();

As a conclusion, I think the most intuitive strategy is to

• Overwrite keys: Overwrite Destination optional keys when they are not optional in the Source
• Merge values: Merge union containing | undefined

I would love to have someone's else insight about it.

Presently, my main technical issue is that Source[Key] with only the key being optional (not the value) is converted to value | undefined at some point I struggle to identify in the codebase.

Thanks in advance !

Interesting reading:

• https://devblogs.microsoft.com/typescript/announcing-typescript-4-4/#exact-optional-property-types

[jclaveau]

After a lot of time and a significant part of my brain lost in the process understanding the impact of exactOptionalPropertyTypes on MergeDeep.

My conclusion are the followings

• Source MUST erase the "|undefined" part of Destination to follow the behavior of Destination being "unknown"
• "| undefined" automatically added here would cost a lot to get rid of, and for what gain? As experimented here and here during way too much time
• type-fest has exactOptionalPropertyTypes in its tsconfig but not in its docs even if not tested without
• some tests fail without it but none of MergeDeep

Found 14 errors in 2 files.

Errors  Files
     1  test-d/exact.ts:230
    13  test-d/internal/collapse-rest-element.ts:13

• my work may have impacted MergeDeep takes type of property of source instead of using the destination type in case of undefined value #941

The present implementation seems to be the most robust and simple to me

[sindresorhus]

The present implementation seems to be the most robust and simple to me

By "present", do you mean the one in this PR or in main branch?

[som-sm]

Ideally, | undefined shouldn't be there.

There's a helper called IsExactOptionalPropertyTypesEnabled, can you try using that?

[som-sm]

Umm... shouldn't | undefined be not removed here, if it's not being removed in the previous test (as shown below)?

type OptionalOrUndefinedNestedRightIntoRight = MergeDeep<
	{nested: {in_right: number} | undefined},
	{nested?: {in_left: string}}
>;

expectTypeOf<OptionalOrUndefinedNestedRightIntoRight>().toEqualTypeOf<{
	nested?: {in_left: string; in_right: number} | undefined;
}>();