-
-
Notifications
You must be signed in to change notification settings - Fork 662
Add ObjectMerge type
#1324
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.
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
som-sm
wants to merge
14
commits into
main
Choose a base branch
from
fix/rewrite-merge-type
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Add ObjectMerge type
#1324
+533
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
89daf4d to
1a3b324
Compare
Merge: Fix behaviour with optional properties and index signaturesObjectMerge type
521fd43 to
2283d3a
Compare
375cb15 to
dc6b43b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces a new
ObjectMergetype that merges two object types in a way that aligns with runtime behaviour.For example, merging
{a: string}into{a: number}(ObjectMerge<{a: number}, {a: string}>) is a straightforward replacement, resulting in{a: string}. However, merging{a?: string}into{a: number}(ObjectMerge<{a: number}, {a?: string}>) is not as simple as producing{a?: string}. In this case, the correct result is{a: number | string}(or{a: number | string | undefined}whenexactOptionalPropertyTypesis disabled).ObjectMergecorrectly handles such scenarios, and many others, which are explained in detail below.Why not fix
Merge?I initially intended this PR to fix the behaviour of our existing
Mergetype, but it looks like theMergetype is only meant to be used at the type level, because its JSDoc example itself doesn't align with runtime behaviour.type-fest/source/merge.d.ts
Lines 13 to 40 in fe88ad8
For example,
Merge<{[x: number]: unknown}, {[x: number]: number}>cannot be{[x: number]: number}if it needs to align with runtime behaviour:So,
Mergesolves a different use case where we want properties from the second object to always override properties from the first object without considering runtime implications.ObjectMergevs Object spread inferenceSimple cases
In simple cases,
ObjectMergebehaves exactly like TS's default inference, where overlapping properties are typed according to the second object.All cases
type-fest/test-d/object-merge.ts
Lines 6 to 13 in 55409ab
Optional properties
If the overlapping property is optional in the second object, the behaviour of
ObjectMergedepends on theexactOptionalPropertyTypescompiler setting.With
exactOptionalPropertyTypesenabled, the behaviour is identical to TS's default inference.All cases
type-fest/test-d/object-merge.ts
Lines 15 to 38 in 55409ab
With
exactOptionalPropertyTypesdisabled, the default inference doesn't account for the fact that optional properties can also be set asundefined, which can lead to uncaught runtime errors.In this case,
ObjectMergeaccounts for the possibility ofundefinedby including it in the resulting type.Readonly properties
The behaviour of
ObjectMergeis exactly like TS's default inference, i.e., thereadonlymodifier is removed from all properties across both objects.All cases
type-fest/test-d/object-merge.ts
Lines 40 to 75 in 55409ab
Number-like string keys
ObjectMergerecognises that number0and string'0'are the same thing, just like TS's default inference.All cases
type-fest/test-d/object-merge.ts
Lines 195 to 246 in 55409ab
Index Signatures
In simple cases,
ObjectMergebehaves exactly like TS's default inference.In more complicated cases, the default inference completely collapses and this is where
ObjectMergereally shines.In this example,
ObjectMergecorrectly recognises that thestringindex signature from the second object has the potential to overwrite theaproperty from the first object, therefore it adjusts the resulting type accordingly.In this example, the inferred type is
{}, which is not very useful. The type produced byObjectMerge, however, remains accurate and usable.In this example, the default inference ignores the possibility that
acould be anumberat runtime, whereasObjectMergecorrectly accounts for it.There are open issues in TS related to this, refer Object spread drops index signature microsoft/TypeScript#27273, Object spreading produces wrong type with non-literal keys microsoft/TypeScript#56431.
All cases
type-fest/test-d/object-merge.ts
Lines 91 to 193 in 55409ab
There are many more cases that
ObjectMergehandles beyond the ones shown here. There’s a fairly comprehensive test suite, and I’ve tried to make sure that all kinds of scenarios are covered and thatObjectMergealways stays in line with the actual runtime behaviour.