-
Notifications
You must be signed in to change notification settings - Fork 12
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
fix: relax type on object properties #180
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK
This is very nice, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
This PR relaxes the type on object properties to accommodate for conflicting type inference as reported in #170
A simplified version of the issue we are facing is the following:
Is inferred as
properties
amongst other is typed as{ [id: string]: Schema }
. This index signature states that if we have a property, it has to be of typeSchema
and cannot beundefined
. This conflicts with the type that is inferred from e.g. an array provided toallOf
, where one element references a different property than the other. Similar to the example above one property in one branch of the union type will then be inferred aspropA?: undefined
.A possible solution is to allow keys with
undefined
values in the index signature using eitherPartial<{ [id: string]: Schema }>
or<{ [id: string]: Schema | undefined }>
. The caveat of is that obviously a key with anundefined
value does not make sense inproperties
. Worth paying that price?This PR does that and adds a test case for the bug from #170
Closes #170