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 excess property checking for nullary sums #4

Merged
merged 3 commits into from
Jan 15, 2024

Conversation

samhh
Copy link
Member

@samhh samhh commented Aug 11, 2023

Fixes #3 with only type-level changes.

@samhh samhh requested a review from Magellol August 11, 2023 12:14
@samhh samhh requested a review from astlouisf January 7, 2024 20:49
Comment on lines +26 to +28
// It should be noted that excess property checking won't help us if the input
// object is first defined elsewhere and then provided to our function. This
// should be considered an unsafe, unsupported use of this API.

Choose a reason for hiding this comment

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

Wasn't this already a limitation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, it just seemed worth writing down.

return (
eq === undefined ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(eq as unknown as Eq<Value<A>>).equals(xv as any, yv as any)
Copy link

@astlouisf astlouisf Jan 10, 2024

Choose a reason for hiding this comment

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

Do you know how eq gets its type const eq: Eqs<A>[keyof Eqs<A>] & ({} | null) after the equality test with undefined while it was simply const eq: Eqs<A>[keyof Eqs<A>] before it?

I'm trying to understand what makes the type assertion necessary; I don't exactly understand how eq can be inferred as anything other than Eq<Value<A>>.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not seeing that inferred intersection. I wouldn't waste much time on it though, these libraries are all absolutely littered with assertions. I don't think there's much we can do about it (though you're welcome to try 😉). The only real concern for me is what's consumer-facing.

@samhh samhh merged commit f546c0c into master Jan 15, 2024
@samhh samhh deleted the fix-nullary-excess-checks branch January 15, 2024 16:08
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.

No excess property checks for nullary sums
2 participants