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

[react-loadable] Make type application of ReactLoadable.Map more precisely #2934

Merged
merged 2 commits into from
Nov 2, 2018

Conversation

kogai
Copy link
Contributor

@kogai kogai commented Nov 2, 2018

Why

When a programmer uses Loadable.Map, he faces a situation where a loaded module hasn't any type-restriction.
(Actually, it may evaluate as empty)

Then, the programmer applies its own type arguments into Loadable.Map like below.

Loadable.Map<Props, { a: () => Promise<string>, b: () => Promise<number> }>({...})

But this application causes type error with a message like Cannot instantiate MapOptionsbecause number [1] is incompatible with string [2] in propertybof type argumentLoadable.Map..

This causes from a restriction of type argument named TModules.
It defined as MapOptions<TProps, TModules: { [key: string]: * }>,
When type arguments applied to TModules, It seems Flow may infer resctriction of TModules like {[string]: () => Promise<string> & () => Promise<number>}, of course actual value like this can't exist anyay.

Therefore, it would be better to relax restriction of TModules to a type of properties as covariant.
It makes Flow can infer TModules more pragmatically.

FYI: A situation described above can reproduce more simply as below.

type T<S: {[string]: * }> = S;
declare var t: T<{a: string, b: number}>;

(t.a); // Cause error

Try code.

@kogai
Copy link
Contributor Author

kogai commented Nov 2, 2018

Whoops, something occurred at Travis 🤔
Several PRs seems to face this problem, too 👀
(In my local environment, all specs are green)

#2933 (comment)
#2931 (comment)

It seems tracking at #2916

@kogai kogai changed the title Make type application of ReactLoadable.Map more precisely [react-loadable] Make type application of ReactLoadable.Map more precisely Nov 2, 2018
Copy link
Contributor

@AndrewSouthpaw AndrewSouthpaw left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR. Verified tests are green locally.

Nice work on the covariant inference, I didn't know that trick!

@AndrewSouthpaw AndrewSouthpaw merged commit 6b1c6a6 into flow-typed:master Nov 2, 2018
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.

None yet

2 participants