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

Improve typing of the union dispatcher function #2203

Merged

Conversation

thegedge
Copy link
Collaborator

@thegedge thegedge commented Aug 4, 2024

What does this PR do and why?

Closes #1627

Currently, the dispatcher function for union types has two issues with the typing on the dispatcher function:

  1. The input snapshot is typed as any.
  2. It expects any type to be returned.

This PR improves both of these by

  1. typing the input snapshot so that it's an input snapshot for the union types, and
  2. it restricts the possible types returned from the dispatcher to the types specified in the union.

Steps to validate locally

As always, this is all about the typechecking, so one would just have to play around with types.union and ensure it fails when you expect it to fail, and passes when you'd expect it to pass.

I've included a test to capture two key scenarios, but let me know if there should be more!

@coolsoftwaretyler
Copy link
Collaborator

Thanks @thegedge! I'll take a look soon. Will you please resolve the merge conflict? On my phone right now and GitHub is not showing precisely what's in conflict.

@thegedge thegedge force-pushed the improve-union-dispatcher-typing branch from 6a0e9ad to fbceb78 Compare August 4, 2024 17:48
@thegedge
Copy link
Collaborator Author

thegedge commented Aug 4, 2024

Ahh, I think master on my fork wasn't up to date, so was just a conflict on the addition of the test. Fixed!

@coolsoftwaretyler
Copy link
Collaborator

Thank you! Will take a look soon!

Copy link
Collaborator

@coolsoftwaretyler coolsoftwaretyler left a comment

Choose a reason for hiding this comment

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

Ooh yeah, this is great.

Before this change, if you had the same test code, snapshot on dispatcher would be any, but this code gives much much better inference.

Before:

Screenshot 2024-08-15 at 10 30 48 AM

After:

Screenshot 2024-08-15 at 10 32 10 AM

And if you mess around with the union in the _brokenUnion case, you can also see the return types. Changing line 1403 to type.string turns the @ts-expect-error into an error itself:

Screenshot 2024-08-15 at 10 34 37 AM

I love it.

I think for consistency's sake, we should push this out behind the upcoming v7 breaking version. It's definitely a "fix", but the issue has been around for so long (at least four years, by count of the issue) that I can see a lot of people ending up with broken types in a patch upgrade.

Thanks for this

) => Types[number]

export interface UnionOptions<Types extends IAnyType[]> {
/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

praise: @thegedge - thank you for the extra inline documentation. Very helpful.

@coolsoftwaretyler coolsoftwaretyler merged commit 8f36402 into mobxjs:master Aug 15, 2024
1 check passed
@thegedge thegedge deleted the improve-union-dispatcher-typing branch August 15, 2024 16:41
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.

Union dispatcher snapshot undefined
2 participants