Skip to content
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

Merged
merged 4 commits into from
Feb 15, 2024
Merged

Conversation

sparten11740
Copy link
Contributor

@sparten11740 sparten11740 commented Feb 8, 2024

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:

const y = [
  { "X": true },
  { "Y": false }
]

Is inferred as

({
    X: boolean;
    Y?: undefined;
} | {
    Y: boolean;
    X?: undefined;
})[]

properties amongst other is typed as { [id: string]: Schema } . This index signature states that if we have a property, it has to be of type Schema and cannot be undefined. This conflicts with the type that is inferred from e.g. an array provided to allOf, 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 as propA?: undefined.

A possible solution is to allow keys with undefined values in the index signature using either Partial<{ [id: string]: Schema }> or <{ [id: string]: Schema | undefined }>. The caveat of is that obviously a key with an undefined value does not make sense in properties. Worth paying that price?

This PR does that and adds a test case for the bug from #170

Closes #170

@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f28dd6f) 99.54% compared to head (92bba82) 99.54%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@sparten11740 sparten11740 changed the title fix: loosen types fix: relax type on object properties Feb 8, 2024
@sparten11740 sparten11740 self-assigned this Feb 8, 2024
@sparten11740 sparten11740 marked this pull request as ready for review February 8, 2024 18:43
mvayngrib
mvayngrib previously approved these changes Feb 9, 2024
Copy link

@mvayngrib mvayngrib left a comment

Choose a reason for hiding this comment

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

utACK

.github/workflows/nodejs.yml Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
ChALkeR
ChALkeR previously approved these changes Feb 15, 2024
Copy link
Contributor

@ChALkeR ChALkeR left a 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!

@sparten11740 sparten11740 dismissed stale reviews from ChALkeR and mvayngrib via f1dd3d8 February 15, 2024 13:31
Copy link

@mvayngrib mvayngrib left a comment

Choose a reason for hiding this comment

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

utACK

@sparten11740 sparten11740 merged commit 4adf1ed into master Feb 15, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type 'undefined' is not assignable to type 'Schema'.ts(2345) error with allOf+if/then+different properties
4 participants