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_v5.x.x] Add support for Flow 0.89+ #3012

Merged
merged 21 commits into from Dec 26, 2018

Conversation

Projects
None yet
@peter-leonov
Copy link
Contributor

commented Dec 15, 2018

EDIT: see the even better types here: #3035!

Hi guys,

Thanks for making Flow better every day!

This PR addresses the issue (#2946) with the very delicious bug fix in Flow 0.85 which required us to explicitly pass types into the exported generic types.

These typings allow passing the new placeholder type _ everywhere but in the first two arguments Props and OwnProps, which are the all props of the component being wrapped and of the newly created connected component. Other types are good to specify explicitly too with a concrete type to get better error messages.

Here is an example:

// @flow
import React from "react";
import { connect } from "react-redux";

// here goes a minimal Redux type set
type State = {|
  +stateProp: string,
|};
type Action = {|
  +type: "action",
|};
type Dispatch = Action => Action;
// WORK IN PROGRESS: in this version of typings we do not check
// if the return value is at all compatible with the dispatch function
const dispatchAction = () => ({ type: "action" });

// notice that OwnProps is an exact type
// as of Flow v0.89 it's required
type OwnProps = {|
  // this prop goes through the ConnectedComponent to WrappedComponent as is
  passesThrough: string,
  // this prop is only used by the mapStateToProps function
  forMapStateToProps: string,
|};
type Props = {
  // connect passes all the own props of the ConnectedComponent
  ...OwnProps,
  // a prop provided by mapStateToProps
  fromMapStateToProps: string,
  // a prop provided by mapDispatchToProps
  dispatchAction: typeof dispatchAction,
};

class WrappedComponent extends React.Component<Props> {}

const mapStateToProps = (state: State, ownProps) => ({
  fromMapStateToProps: state.stateProp + ownProps.forMapStateToProps,
});

const mapDispatchToProps = {
  dispatchAction,
};

// hover over those underscore signs in VS Code to see which types Flow inferred for you
export const ConnectedComponent = connect<Props, OwnProps, _, _, _, _>(
  mapStateToProps,
  mapDispatchToProps,
)(WrappedComponent);
<ConnectedComponent passesThrough="foo" forMapStateToProps="bar" />;

And here is a more complex example for usage with factory maps state function:

// @flow
import React from "react";
import { connect } from "react-redux";

// here goes a minimal Redux type set
type State = {|
  +stateProp: string,
|};
type Action = {|
  +type: "action",
|};
type Dispatch = Action => Action;
// WORK IN PROGRESS: in this version of typings we do not check
// if the return value is at all compatible with the dispatch function
const dispatchAction = () => ({ type: "action" });

// notice that OwnProps is an exact type
// as of Flow v0.89 it's required
type OwnProps = {|
  // this prop goes through the ConnectedComponent to WrappedComponent as is
  passesThrough: string,
  // this prop is only used by the mapStateToProps function
  forMapStateToProps: string,
|};
// it is required as of Flow v0.89 to have a separated type
// otherwise Flow cannot differentiate factory function from an error
// (I guess, it's kind of a bug in Flow)
type StateProps = {|
  // a prop provided by mapStateToProps
  fromMapStateToProps: string,
|};
type DispatchProps = {|
  // a prop provided by mapDispatchToProps
  dispatchAction: typeof dispatchAction,
|};

type Props = {
  // connect passes all the own props of the ConnectedComponent
  ...OwnProps,
  ...StateProps,
  ...DispatchProps,
};

class WrappedComponent extends React.Component<Props> {}

type MapStateToPropsFactory = (
  State,
  OwnProps,
) => (State, OwnProps) => StateProps;
const mapStateToProps: MapStateToPropsFactory = (state, ownProps) => (
  state,
  ownProps,
) => ({
  fromMapStateToProps: state.stateProp + ownProps.forMapStateToProps,
});

const mapDispatchToProps = {
  dispatchAction,
};

// hover over those underscore signs in VS Code to see which types Flow inferred for you
export const ConnectedComponent = connect<
  Props,
  OwnProps,
  StateProps,
  DispatchProps,
  State,
  Dispatch,
>(
  mapStateToProps,
  mapDispatchToProps,
)(WrappedComponent);
<ConnectedComponent passesThrough="foo" forMapStateToProps="bar" />;

One important RFC: do you think it's better to make type arguments to all forms of connect() follow the same pattern or what I've made with requiring only the needed ones is better. I made myself few typos because of sometimes it's connect<Props, State, …>() and sometimes it's connect<Props, Action, …>().

Still a bit TODO:

  • decide on type arguments order
  • update tests
  • support mapStateToProps and mapDispatchToProps factory functions
  • add comments in the typings code explaining what to do if Flow errors are misleading (good candidates are all the {| ...OP, ...DP |})
  • add a short readme in the typings themselves on how to use connect()
  • clean up unused types and misleading comment leftovers
  • add example on how to type a factory state/dispatch function

EDIT: updated example to reflect the lates changes.

@peter-leonov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2018

@villesau @hwdtech @maximeg Dear hardcore react-redux typers, could you please take a look at this PR which changes typings for our beloved react-redux module. I found you in git blame 🤓

@peter-leonov peter-leonov force-pushed the peter-leonov:react-redux-flow-85 branch from 98990e4 to 0f00a2b Dec 16, 2018

peter-leonov added some commits Dec 16, 2018

add `withRef`
Unfortunately without affecting `getWrappedInstance()`
@villesau

This comment has been minimized.

Copy link
Member

commented Dec 16, 2018

Hi, nice work!

Could you provide an example e.g on redux flow-example about the usage: https://github.com/reduxjs/redux/tree/master/examples/todos-flow ? I tried to change it to use these libdefs but couldn't get them working.

@villesau

This comment has been minimized.

Copy link
Member

commented Dec 16, 2018

When you start to work on fixing the tests, I'd suggest to go them trough one test function at a time. Remove the rest of the tests and focus on one. Add type parameters and see how it goes, then move to next one. I guess some $ExpectError locations might change, but otherwise I'd expect the specs to cover most of the use cases pretty well.

Unfortunately with libdef tests it's not possible to comment out tests since // //$ExpectError is still expecting error. Not sure if we could adjust that behaviour a bit though? it's just regex after all (although pretty frightening one: 'suppress_comment=\\\\(.\\\\|\\n\\\\)*\\\\$ExpectError' ).

@peter-leonov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 16, 2018

@villesau thanks! Currently I'm fixing the last 🤞 bug dew to a misunderstanding of mergeProps nature (never used them) and then I'm all up to have a good example for todos-flow.

BTW, here is the most advanced example with both state and dispatch arguments. The rest live in the tests folder. Yes, a bit simplified 😅 but exported to make Flow ask for explicit typings.

@rkurbatov

This comment has been minimized.

Copy link
Contributor

commented Dec 16, 2018

@peter-leonov is it possible to add support of the mapStateToProps factory in your rewritten type-definition?

@hewsut

This comment has been minimized.

Copy link

commented Dec 17, 2018

do you think it's better to make type arguments to all forms of connect() follow the same pattern or what I've made with requiring only the needed ones is better.

Same pattern. Explicit over magic, please!

@peter-leonov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

@rkurbatov sure, everything is possible 😅, adding to the TODO list in the PR.

@peter-leonov peter-leonov referenced this pull request Dec 18, 2018

Closed

Flow types #163

@peter-leonov peter-leonov force-pushed the peter-leonov:react-redux-flow-85 branch from 1cca038 to 560f0da Dec 19, 2018

@peter-leonov peter-leonov force-pushed the peter-leonov:react-redux-flow-85 branch from 7a65183 to 4b24e1d Dec 19, 2018

@peter-leonov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2018

Can anybody help me to get why travis reports errors in react-redux_v4.x.x/flow_v0.53.x-?

@peter-leonov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2018

Regarding the order of type arguments. What do you think of this order:

connect<Props, OwnProps, StateProps, DispatchProps, State, Action>(…)

Also, as far as I can tell for flow v0.89 only the first two are mandatory to specify, other 4 can be repaced with the new awesome type placeholder:

connect<Props, OwnProps, _, _, _, _>(…)
@fabiomcosta

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

@DragorWW

This comment has been minimized.

Copy link

commented Dec 20, 2018

@peter-leonov i check your typing, and find problem with this case:

type OwnProps = {
  text: string,
  ...React.ElementProps<'div'>,
};
type Props = {
  ...OwnProps,
  onClick: () => void,
}
connect<Props, OwnProps,  _, _, _, _>(null, {onClick: openModal})(Comp)

get error Flow: inexact OwnProps [1] is incompatible with exact object type [2].

  declare export function connect<-P, -OP, -SP, -DP, S, A>(
    mapStateToProps: null | void,
    mapDispatchToProps: MapDispatchToPropsFn<A, OP, DP> | DP,
    mergeProps?: null | void,
    options?: ?Options<S, OP, {||}, {| ...OP, ...DP |}>,
  ): Connector2<OP, ExtendProps<
  P, 
  {| ...OP, ...DP |} // <--- problem
>>;

@peter-leonov peter-leonov changed the title [react-redux_v5.x.x] Add support for Flow 0.85+ [react-redux_v5.x.x] Add support for Flow 0.89+ Dec 20, 2018

@peter-leonov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

@DragorWW nice catch! The reason is that there is something I really do not understand in how Flow mixes exact and inexact objects merging. In my typings and also in the tests the OwnProps object should be exact (tell me please if this works for you):

type OwnProps = {|
  text: string,
  ...React.ElementProps<'div'>,
|};
type Props = {
  ...OwnProps,
  onClick: () => void,
}

This trick also fixes an annoying error like property x is incompatible with undefined. I guess, this undefined can come from an inexact object. To make typing more stable (and also support the trend on making all the object types strict by default) I just decided to make an assumption that it's ok to have OwnProps always strict. Adding this to the PR message.

@peter-leonov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 25, 2018

@villesau

One interesting, though super minor, problem I noticed when trying out with reac-tredux todos-flow example was that these does not accept store type.

I'm not sure if this is what users of flow-typed typings for react-redux expect, but, it would be really nice to somehow express the dependency of despatch() type and action creators in Props. I'm currently using similar trick (giving Dispatchable instead of pure Action) in our in-house version of these typings for redux-thunk, but I was not sure if it's too much of a shift for a single release. I guess, too many surprises for Flow users these months 😅

What do you think?

@PinkaminaDianePie

This comment has been minimized.

Copy link

commented Dec 25, 2018

Not sure about the contribution process to flow-typed, but can it be done via multiple PRs? Just merge this one with main stuff, and do other minor tweaks in separate PRs?

I checked definitions from this PR on my pet project (only for some components) and everything worked. I'd like to see this merged, so I will be able to use it everywhere :) And no matter how many unfinished things left - it's still much better than previous definitions, just because they are broken with v0.85x+. And lots of other people can't update flow mainly because of react-redux, so merging this PR can help them. If there are any inconsistencies, they could be fixed later.

@villesau

This comment has been minimized.

Copy link
Member

commented Dec 25, 2018

@peter-leonov

I'm not sure how to change them. Current ones, at least, point in the right direction. If we just change them to what the current Flow version gives us, in this situation, we would be just documenting the bugs in the error messages. I expect Flow one day to simply error out in such type loop looking cases instead of the wild guessing it does now.

Two approaches: A) remove the misleading tests, or B) link the // $ExpectErrors to corresponding flow bug reports so that future contributors knows what to expect or C) change the message of // $ExpectError to correspond current behaviour. IMO B would be the best option here. We would have expectations and explanation on what the expect should be expecting. If there are some places where type errors should occur but doesn't something like // X missing, should be an error is good enough.

react-redux libdefs are probably the most important libdefs besides lodash and builtin react defs for React devs. That's why it is especially important that the libdefs are as maintainable and explanatory as possible. before last rewrite about a year ago, the libdefs were in such bad shape that both libdefs and tests had to be rewritten; tests no longer documented the behaviour in sane way nor were very understandable. The test suite is exceptionally large for react-redux, so keeping them maintainable as well is crucial for providing high quality libdefs.

So in short: If there are some discrepancies due Flow bugs, would be good to document what happens, and what's really expected to happen. Then future maintainers and contributors can have better understanding of the purpose of $ExpectError, or other places where error would be expected (and does not happen at all).

@villesau

This comment has been minimized.

Copy link
Member

commented Dec 25, 2018

I'm not sure if this is what users of flow-typed typings for react-redux expect, but, it would be really nice to somehow express the dependency of despatch() type and action creators in Props. I'm currently using similar trick (giving Dispatchable instead of pure Action) in our in-house version of these typings for redux-thunk, but I was not sure if it's too much of a shift for a single release. I guess, too many surprises for Flow users these months 😅

If you don't have clear way to fix this smoothly, this is by no means any kind of blocker. It can be sorted out later.

But I would imagine that having some sort of {...Props, {store?: ?StoreType}} could help here. The store type is anyways one of the type parameters so no need to add new type parameters at least.

explained the misleading errors better for better future understanding
Also moved one error suppression to match others.
@peter-leonov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 25, 2018

C) change the message of // $ExpectError to correspond current behaviour.

Done.

IMO B would be the best option here.

Completely correct. IMHO we can do this in another PR with the distilled bug examples reported and linked.

react-redux libdefs are probably the most important libdefs besides lodash and builtin react defs for React devs. That's why it is especially important that the libdefs are as maintainable and explanatory as possible.

I undersign every word! And I tried my best to make everything documented in this PR and in the libdefs sources.

// Here Flow reports that `dispatch1` is missing from props,
// while the real error is still the missing passthroug property,
// giving the passthrough property fixes the wrongly titled error.
//$ExpectError no passthrough

This comment has been minimized.

Copy link
@villesau

villesau Dec 25, 2018

Member

duplicate //$ExpectError no passthrough causes unnecessary errors. Once this is fixed, the PR is good to go!

This comment has been minimized.

Copy link
@peter-leonov

peter-leonov Dec 25, 2018

Author Contributor

Sorry, did not get, what needs to be changed?

This comment has been minimized.

Copy link
@villesau

villesau Dec 25, 2018

Member

@peter-leonov one line below there is second //$ExpectError: This one does not actually match anything.

This comment has been minimized.

Copy link
@peter-leonov

peter-leonov Dec 26, 2018

Author Contributor

Now I see 🤦‍♀️ Fixed!

peter-leonov added some commits Dec 26, 2018

@peter-leonov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 26, 2018

@villesau

One interesting, though super minor, problem I noticed when trying out with reac-tredux todos-flow example was that these does not accept store type.

I've replaced Action type with Dispatch type as this changes nothing for this stage, but gives the opportunity to fix that "super minor problem" you've mentioned above and also makes these libdefs more flexible for redux-thunk users.

I hope, we are totally fine by now 😎

@villesau
Copy link
Member

left a comment

Nice!! Awesome work @peter-leonov, thanks a lot!

@villesau villesau merged commit f8afc4c into flow-typed:master Dec 26, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@peter-leonov peter-leonov deleted the peter-leonov:react-redux-flow-85 branch Dec 26, 2018

@PinkaminaDianePie

This comment has been minimized.

Copy link

commented Dec 26, 2018

Thank you so much, guys! I was checking this thread much more often than my social networks :D

@rkurbatov

This comment has been minimized.

Copy link
Contributor

commented Dec 26, 2018

Awesome work! We have a huge project that uses flow so I have something to do during upcoming holidays :D

@peter-leonov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 26, 2018

Thanks a lot to all of you guys for your help and support! Bug reports are very welcome 😅

P.S. Updated the PR description with two examples.

@peter-leonov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 30, 2018

@villesau

One interesting, though super minor, problem I noticed when trying out with reac-tredux todos-flow example was that these does not accept store type.

I've replaced Action type with Dispatch type as this changes nothing for this stage, but gives the opportunity to fix that "super minor problem" you've mentioned above and also makes these libdefs more flexible for redux-thunk users.

For those who are interested in stricter action type checking or use redux-thunk a lot with the new libdefs, please, also follow the #3035.

@Ashoat

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

Thanks for the new and improved libdef! Unfortunately you stripped away the default exports, which breaks compatibility with the CommonJS distro of react-redux. Since Node still can't handle the ES6 distro of react-redux (since it attempts a named import of react, which is CommonJS-only, and Node can't handle named imports of CommonJS modules), this breaks compatibility of this libdef with Node's experimental ES6 support. More context is available here.

I'll put up a PR momentarily to reintroduce the default export.

@kangax

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

@peter-leonov I'm getting "Missing type annotation for OP. OP is a type parameter declared in function type [1] and was implicitly instantiated at call of connect [2]." with quite a lot of connect()'s in our app. Adding types to MSP (and/or MDP) doesn't change anything. Anything else I can do to fix it?

This is on Flow 0.89 + the redux typedefs from this PR.

@AlimovSV

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

@peter-leonov Will you have the chance to add support of react-redux@6?

@glasserc glasserc referenced this pull request Jan 31, 2019

Merged

Update flow-bin to 0.91.0 #731

@peter-leonov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

@ALL: I'm having an exam later this month so will not be of much help, sorry.

@kangax could you please show some portion of the code producing this error?

@AlimovSV I have not planed it, but will be happy to take a look at your PR ;)

@fabiomcosta

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

@kangax I think this is happening because the new types are less dependent on Flow's inference, and require calling connect with some type annotations.

See https://github.com/flow-typed/flow-typed/blob/master/definitions/npm/react-redux_v5.x.x/flow_v0.89.x-/react-redux_v5.x.x.js#L6-L8

connect<Props, OwnProps, _, _, _, _>(…)

Where you can define Props and OwnProps as:

type OwnProps = {|
  // define props that don't come from your redux store
|};
type Props = {|
  ...OwnProps,
  // define props that come from your redux store
|};

On the link you'll also see that OP -> OwnProps.
I honestly think we should just use the full names on the type definitions, it would make the errors messages a bit more understandable.

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.