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 type inference of the action callback arguments #2213

Merged
merged 2 commits into from
Nov 23, 2019
Merged

Fix type inference of the action callback arguments #2213

merged 2 commits into from
Nov 23, 2019

Conversation

kubk
Copy link
Collaborator

@kubk kubk commented Nov 19, 2019

Fixes: #2211

  • Added typescript typings
  • Verified that there is no significant performance drop (npm run test:performance)

I prepared the links to TS playground with multiple test cases so you can verify that type inference works now.

Example 1 - No changes were made from my side. I copy-pasted types of the IActionFactory from here:

export interface IActionFactory {

As you can see, the type inference doesn't work here, the first argument of the action callback has type unknown. Moreover, TypeScript fails to compile actions with 2 more arguments when --noImplicityAny is enabled (default in Create React App).

Example 2 - All function overloads were removed. Now TypeScript is able to infer types of function arguments. There is only one problem left: type inference doesn't work with promises.

Example 3 - <T extends Function> is replaced with <T extends Function | null | undefined>. This change fixes the problem with promises. It looks like type ((value: T) => TResult1 | PromiseLike<TResult1>) | undefined | null is not a subtype of type Function.

@danielkcz
Copy link
Contributor

That's really interesting! I always thought this overload hell is a necessary evil. It's nice to see there are some ways around. Although now it depends if it doesn't break some other edge cases we cannot think of.

@kubk
Copy link
Collaborator Author

kubk commented Nov 20, 2019

To prevent typing regressions in the future we can cover TS types with tests. It looks like Mobx already includes tests for the Flow typings: https://github.com/mobxjs/mobx/blob/master/test/flow/test.js__FIXTHIS
There are multiple packages that can help us:

Will be happy to help with this task.

@danielkcz
Copy link
Contributor

I definitely support that idea, go ahead, please :) Probably in different PR first and then we can continue here.

Not sure which package to choose, have no experience with either.

@danielkcz danielkcz merged commit d07e405 into mobxjs:master Nov 23, 2019
danielkcz pushed a commit that referenced this pull request Nov 23, 2019
* Fix type inference of the action callback arguments (#2213)

* Fix type inference of the action callback arguments

* Fix type inference of the action callback arguments

(cherry picked from commit d07e405)

* Type test for the action callback arguments (#2217)

(cherry picked from commit 4f59db8)
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.

Incorrect type inference of the action callback arguments
2 participants