Fix inconsistent merge onto Destination's optional entries and better handling of undefined vs optional entries #1209
Conversation
…s content anymore sindresorhus#1208 - wip: better "optional key VS | undefined" support
4bafb03 to
22b0e18
Compare
5874691 to
dbf6302
Compare
…s / undefined values sindresorhus#1208
…onal keys / undefined values sindresorhus#1208" This reverts commit 8263a27.
|
After a lot of time and a significant part of my brain lost in the process understanding the impact of My conclusion are the followings
Found 14 errors in 2 files.
Errors Files
1 test-d/exact.ts:230
13 test-d/internal/collapse-rest-element.ts:13
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? |
| // "| undefined" is added here and the only way to remove it | ||
| // would depend on exactOptionalPropertyTypes and a complex | ||
| // logic which may not worth the effort |
There was a problem hiding this comment.
Ideally, | undefined shouldn't be there.
There's a helper called IsExactOptionalPropertyTypesEnabled, can you try using that?
| nested: { | ||
| in_left: string; | ||
| in_right: string; | ||
| }; // "| undefined" is removed by Right |
There was a problem hiding this comment.
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;
}>();|
Hi @jclaveau, I've added a comment to the parent issue: #1208 (comment) Just a heads-up: Thanks. |
|
I'm going to close this for now, since it has a lot of conflicts and it's not moving forward. Feel free to open a new PR if you ever get to this again :) |
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 valuesthe 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
As a conclusion, I think the most intuitive strategy is to
| undefinedI 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 tovalue | undefinedat some point I struggle to identify in the codebase.Thanks in advance !
Interesting reading: