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: replace intlshape intersection type with spread #3695

Merged
merged 2 commits into from
Jan 13, 2020
Merged

fix: replace intlshape intersection type with spread #3695

merged 2 commits into from
Jan 13, 2020

Conversation

meandmax
Copy link
Contributor

@meandmax meandmax commented Jan 10, 2020

Hello Flow Typed Team,

this change removes a bug which we experienced with IntlShape. In our case, everything except the first of type in the intersection $npm$ReactIntl$IntlConfig was missing when we did a spread of the property. This makes me believe that $npm$ReactIntl$IntlConfig is treated exact in our code base, even though it is not declared as exact.

We do not experience the bug in https://flow.org/try/ unless we make the first type of the intersection $npm$ReactIntl$IntlShape exact as you can see below:

example

The solution for us is using spreading instead.

  • Type of contribution: fix

Other notes:

@jbrown215 is the change to make most of the types inexact intentional?

Thank you very much!
Cheers Max

@jbrown215
Copy link
Contributor

is the change to make most of the types inexact intentional?

Not sure which change you are referring to, but generally: intersections are only meaningful on inexact object types. Most intersections with exact object types are cannot be inhabited by a value, since an object can't have exactly one set of properties and exactly a different set of properties.

Maintianers: FWIW, this change looks good to me. If possible, $npm$ReactIntl$IntlConfig and $npm$ReactIntl$IntlFormat should be made exact so that $Exact isn't necessary

@pascalduez
Copy link
Member

@meandmax Thanks for the PR!
Would it be possible that you also add a test replicating your issue/use-case? So that next changes or refactor doesn't unintentionally revert this behaviour.

@meandmax
Copy link
Contributor Author

Howdy @pascalduez,

I'm more than happy to add a test. Unfortunately, this error does only appear in the repl if I make $npm$ReactIntl$IntlConfig explicitly exact.

In our project, it appears to me that for some reason $npm$ReactIntl$IntlConfig is treated exact (even though it is declared as inexact) and as a result, the intersection type which is replaced in this PR does not work correctly specifically if we try to spread properties.

I'm unsure how should I test it. I added some tests checking exactness and if it is happy with spreading, but I might need some help.

Thank you,
Cheers Max

@meandmax
Copy link
Contributor Author

@jbrown215 under which circumstances does a type which is declared as inexact behave like an exact type, because this seems to be the case in our project.

Thanks for your help :)

@pascalduez
Copy link
Member

@pascalduez pascalduez merged commit 2cbbfd7 into flow-typed:master Jan 13, 2020
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

3 participants