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-redux] libdef incompatible with flow v0.85 #2946

Closed
elvorfirilmathredia opened this issue Nov 7, 2018 · 14 comments

Comments

Projects
None yet
9 participants
@elvorfirilmathredia
Copy link

commented Nov 7, 2018

The todos-flow example just works with flow v0.78 and the current flow-typed react-redux types, but after upgrading to flow v0.85 there are a lot of flow errors I am not able to fix.

Full repo here and full error log here.

> todos-flow@0.0.1 flow 
> flow

Error ------------------------------------------------------------------------------- src/containers/FilterLink.js:29:19

Missing type annotation for `CP`. `CP` is a type parameter declared in function type [1] and was implicitly instantiated
at call of `connect` [2].

   src/containers/FilterLink.js:29:19
                          v-------
    29| const connector = connect(
    30|   mapStateToProps,
    31|   mapDispatchToProps
    32| );
        ^ [2]

References:
   flow-typed/npm/react-redux_v5.x.x.js:133:34
                                         v
   133|   declare export function connect<
   134|     Com: ComponentType<*>,
   135|     A,
   136|     S: Object,
   137|     DP: Object,
   138|     SP: Object,
   139|     RSP: Object,
   140|     RDP: Object,
   141|     CP: $Diff<$Diff<ElementConfig<Com>, RSP>, RDP>,
   142|     ST: $Subtype<{[_: $Keys<Com>]: any}>
   143|     >(
   144|     mapStateToProps: MapStateToProps<S, SP, RSP>,
   145|     mapDispatchToProps: MapDispatchToProps<A, DP, RDP>
   146|   ): (component: Com) => ComponentType<CP & SP & DP> & $Shape<ST>;
          --------------------------------------------------------------^ [1]


Error ------------------------------------------------------------------------------- src/containers/FilterLink.js:29:19

Missing type annotation for `DP`. `DP` is a type parameter declared in function type [1] and was implicitly instantiated
at call of `connect` [2].

   src/containers/FilterLink.js:29:19
                          v-------
    29| const connector = connect(
    30|   mapStateToProps,
    31|   mapDispatchToProps
    32| );
        ^ [2]

References:
   flow-typed/npm/react-redux_v5.x.x.js:133:34
                                         v
   133|   declare export function connect<
   134|     Com: ComponentType<*>,
   135|     A,
   136|     S: Object,
   137|     DP: Object,
   138|     SP: Object,
   139|     RSP: Object,
   140|     RDP: Object,
   141|     CP: $Diff<$Diff<ElementConfig<Com>, RSP>, RDP>,
   142|     ST: $Subtype<{[_: $Keys<Com>]: any}>
   143|     >(
   144|     mapStateToProps: MapStateToProps<S, SP, RSP>,
   145|     mapDispatchToProps: MapDispatchToProps<A, DP, RDP>
   146|   ): (component: Com) => ComponentType<CP & SP & DP> & $Shape<ST>;
          --------------------------------------------------------------^ [1]
@AndrewSouthpaw

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

It's a known issue, flow 0.85 breaks a ton of stuff. @villesau is aware, he's the genius behind the react-redux libdef and hasn't had the time to work on upgrading to 0.85 yet.

@AndrewSouthpaw AndrewSouthpaw changed the title [react-redux] todos-flow example not working out of the box with flow v0.85 [react-redux] libdef incompatible with flow v0.85 Nov 8, 2018

@villesau

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

Here is an answer from Flow team member with even some examples: https://medium.com/@samgoldman/ville-saukkonen-thanks-and-thanks-for-your-thoughtful-questions-24aedcfed518

and related issue in Flow: facebook/flow#7125

Unfortunately I don't know when I will have time to look at this. In the mean time pull requests are more than welcome! I believe react-redux prevents many people from updating to 0.85.0.

@xthule

This comment has been minimized.

Copy link

commented Nov 9, 2018

A quick workaround I found was changing the definitions for connect to use object spread instead of intersections. For example:

  declare export function connect<
    Com: ComponentType<*>,
    S: Object,
    SP: Object,
    RSP: Object,
    CP: $Diff<OmitDispatch<ElementConfig<Com>>, RSP>,
    ST: {[_: $Keys<Com>]: any}
    >(
    mapStateToProps: MapStateToProps<S, SP, RSP>,
    mapDispatchToProps?: null
  ): (component: Com) => ComponentType<CP & SP> & $Shape<ST>;

changing the last line to:

 ): (component: Com) => ComponentType<{...CP, ...SP}> & $Shape<ST>;

This works for me, but my component prop types use object spread to combine props provided by redux with the components own props, e.g.

type ReduxProvidedDispatchTypes = {|
    selectSchool: (_id: string) => void,
    unsetLoading: () => void,
    setLoading: () => void,
|};

type PropTypes = {|
    ...ReduxProvidedDispatchTypes,
    schoolsByStatus?: SchoolStatusType
|};
@kangax

This comment has been minimized.

Copy link
Contributor

commented Nov 19, 2018

@xthule tried your workaround and it brings errors in 0.85 in our app from 863 to 808 :D (only 10 failures due to react-redux though). I'll see if I can find the cause of the remaining ones.

@master-7

This comment has been minimized.

Copy link

commented Nov 26, 2018

Do you have any updates?

@jbrown215

This comment has been minimized.

Copy link

commented Dec 3, 2018

I think @peter-leonov did a really good job with https://gist.github.com/peter-leonov/e1fac25b6ade9b28c03c213e8a9d7121.

With these types, you should have OwnProps, StateProps, and DispatchProps (OP, SP, and DP) all be exact object types. In your component that combines all three of these types, use:

type Props = {| ...OwnProps, ...StateProps, ...DispatchProps |}

It's important that you don't use intersection with exact objects, since that will produce an empty type.

@zanettin

This comment has been minimized.

Copy link

commented Dec 11, 2018

@villesau how likely is it, that the typedef provided by @peter-leonov will find the way into the official typedef? we have to fix >700 flow errors and i really don‘t want to fix them twice 😄 thx a lot

@peter-leonov

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

We use these types with few minor improvements in a relatively complex project. If there is a chance to make some form of it official I'd be glad to make the typings up to date and production ready. And suppor them once more people start using them and find bugs :)

@villesau

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

@zanettin someone should open a PR :) I haven't had to check them out properly. Some test cases would help hugely! In practice I believe that most of these testes should succeed almost as is: https://github.com/flow-typed/flow-typed/blob/master/definitions/npm/react-redux_v5.x.x/flow_v0.68.0-/test_connect.js

@zanettin

This comment has been minimized.

Copy link

commented Dec 11, 2018

thanks for your replys! great to hear @peter-leonov ! i'll replace the libdef with your gist and try to fix our errors. hope you can open a PR to get your adoptions merged 👍

@jbrown215

This comment has been minimized.

Copy link

commented Dec 11, 2018

FWIW, I am working on a project to make type spread more predictable with inexact objects. Hopefully we can even get rid of the "exactness" requirement with some changes to flow.

Excited to see you guys adopting these new types!

@peter-leonov

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

@zanettin I really doubt that the incomplete typings from my gist will cover all the use cases as they are, so if you need any kind of support porting your project, just poke me via email: gojpeg@gmail.com

@villesau thanks for pointing to the tests, will try my best making them work now (hope to get it done by the end of the week).

@peter-leonov

This comment has been minimized.

Copy link
Contributor

commented Dec 15, 2018

@zanettin @villesau here is a promised PR #3012 😅

I'd say the adapted typings are production ready now. They support options and mergeProps arguments to connect() with proper connection to the component props and mapStateToProps, own props, etc. Most of the type arguments can be requested to be inferred with the new _ type placeholder.

@villesau

This comment has been minimized.

Copy link
Member

commented Dec 26, 2018

The issue is now fixed. Huge thanks to @peter-leonov for fixing the libdefs! If you encounter any issues, please open a new bug report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.