Flow: Issues with react-redux

[maxsalven]

The following works OK and correctly gives me a flow error about data being the wrong type:

//@flow

import React from "react";
import { connect } from "react-redux";
import type { Connector } from "react-redux";

const updateUser = () => ({ type: "UPDATE_USER" });

type ComponentProps = {
  data: boolean
};

const MyComponent = ({ data }: ComponentProps) => <div>{data}</div>;

const mapStateToProps = (state): * => ({
  data: "test"
});

const connector: Connector<{||}, ComponentProps> = connect(mapStateToProps);

export default connector(MyComponent);

However, if I have a second argument to connect to use an action:

const connector: Connector<{||}, ComponentProps> = connect(mapStateToProps, {
  updateUser
});

then Flow no longer gives me an error. Am I doing something wrong?

[julienw]

This looks like a libdef issue, maybe you should file this at https://github.com/flowtype/flow-typed/. I know that the libdef for react-redux is not perfect yet, so it could definitely be an error in it.

[eloytoro]

The reason this happens is because flow has no idea how to handle generic types and closure types mixed together.

In this case the component's resulting prop type should be derived from the type of the return value of mapStateToProps, however flow stops trying to resolve abstract types for generic definitions of curried functions and assigns them a weird mixed type annotation

Here's a simplified version of why this doesn't work

https://flow.org/try/#0PTAEAEDMBsHsHcBQiDGsB2BnALqS7QBeUAHgEEAaUAIQD4AKbAJwEMtJYmBbALlHrIBKIrRrDCo+gDcW0PkL7URoAN6JQoJgFNsAVyYFmbTB27TZgxAF9E2AJ4AHLaAAqjrWSKrQ6Fly18OEwAlugA5qA29k6u7krEKqBh2jqBzKERNmhYuEbsnFxe9Im+-pF8bk4KsU7xkonJWqmgAAYAFlrQcHwAJCqlWlYtkZbZOKBOTJgYFe6eCT5+AaAA5J2wdiuRiPiMrPncgvST0+iCQA

[maxsalven]

Thanks @julienw, I have filed an issue in flow-typed as well.

Note however that the exact code posted above works fine (you correctly get an error in both scenarios) with Flow 0.54, even though the libdef is identical:

https://github.com/flowtype/flow-typed/tree/master/definitions/npm/react-redux_v5.x.x

So whether or not the libdef is broken, I'm not sure, but switching from Flow 0.54 to 0.55 definitely breaks the desired behaviour.

@eloytoro I don't understand enough about the internals of Flow, but my code above works correctly in Flow 0.54.

[eloytoro]

@maxsalven could you post the full code? its missing the Connector type definition.

Also the generic type you're using for the first parameter here Connector<{}, ComponentProps> will allow _any_ object, so if this parameter refers to the allowed component props, you're basically telling flow your component could have _any_ props (Remember that the connected component's prop types does not inherit the wrapped component prop types, you must define it)

[maxsalven]

@eloytoro Have updated the example to be complete code.

I changed the first parameter (which is the props received by the container itself) to be a sealed empty object, but it made no difference to the problem.

[eloytoro]

Does it compile? data should be a boolean, not a string. If it compiles flow is doing something wrong.

type ComponentProps = {
  data: boolean
}

[maxsalven]

Yes, either Flow or the libdef is incorrect.

Flow correctly gives me an error about data being the wrong type if I only call connect(mapStateToProps).

If I call connect(mapStateToProps, { updateUser }), then Flow incorrectly says there are no errors, but clearly there are (data is still the wrong type).

[maxsalven]

I imagine Redux is one of the most used third party libraries to React. Any ideas on how to fix this?

[julienw]

@maxsalven could you please share a full code showing your issue ? Maybe in a separate github repository ?

[maxsalven]

@julienw

https://github.com/maxsalven/flow-issue

This file correctly reports a flow error due to data being the wrong type:
https://github.com/maxsalven/flow-issue/blob/master/has-errors-reports-errors.js

This file incorrectly reports no errors:
https://github.com/maxsalven/flow-issue/blob/master/has-errors-reports-no-errors.js

[eloytoro]

Yes, either Flow or the libdef is incorrect.

Both are. Flow doesn't have the tools to properly type this definition right now, making it impossible to have an accurate libdef, which kinda voids the point of having a type definitions at all.

@maxsalven I would like you to try having flow check the props of connected components when you're rendering, flow will fail to do proper typechecking for me

[villesau]

There are multiple issues about react-redux types at the moment. Not sure if all are related, but that points out that they're currently badly broken and widely used.

https://github.com/facebook/flow/issues/4939
https://github.com/facebook/flow/issues/4909
https://github.com/flowtype/flow-typed/pull/1317
https://github.com/flowtype/flow-typed/issues/1538
https://github.com/flowtype/flow-typed/issues/1498
https://github.com/flowtype/flow-typed/issues/1432
https://github.com/flowtype/flow-typed/issues/1269

[maxsalven]

@eloytoro

Flow doesn't have the tools to properly type this definition right now, making it impossible to have an accurate libdef, which kinda voids the point of having a type definitions at all.

The code example I provided works perfectly fine in Flow 0.54, so I'm not sure I understand your comment. Flow clearly has the tools to provide a more accurate definition that the current one.

[julienw]

@maxsalven I couldn't look at your example yet. I just want to give my 2 cents that I know Flow has a weakness when analyzing a file by itself. Usually it works better when analyzing a whole program, including the calling site. In your case here, there is no calling site so I believe some paths in Flow are not called. Again, I haven't tried your code and haven't tried to actually use the connected components, so I don't know if this would actually work here.

I believe this could be fixed, but is it worth it ? A file usually works in connection with other files, inside a bigger program.

(note: I'm not a Flow developer, merely a Flow user)

[maxsalven]

@julienw The pattern used in my example repo is an extremely common one in the React + Redux world. Pretty much every Redux connected component will be coded in a near identical manner to what I've done in the example.

I would say 100% this needs to be fixed, it's a huge issue in one of the most popular libraries for React.

Even the canonical example (https://github.com/reactjs/redux/tree/master/examples/todos-flow) is broken in the current version of Flow (but I'll reiterate, it works fine in Flow 0.54).

As one example of these issues: If you change this line to a different prop name (e.g. todas instead of todos), you get zero error in the latest Flow even though a required prop in the referenced component (todos in https://github.com/reactjs/redux/blob/master/examples/todos-flow/src/components/TodoList.js) is not provided! You correctly get an error in Flow 0.54:

https://github.com/reactjs/redux/blob/master/examples/todos-flow/src/containers/VisibleTodoList.js#L16

[maxsalven]

Opened an issue on the Redux repo as well, as the example code in there is also currently broken.

https://github.com/reactjs/redux/issues/2717

[AndrewSouthpaw]

Okay, there's a bunch of different open issues, I think I have a working solution, so I'll redirect most issues over to here. It'd be nice to consolidate the discussion. :)

As far as I can tell, both Flow and the libdef are fine, we just need to write annotations a bit differently.

Here is a demo of a Redux connected component working with v0.59. (Please forgive the library definitions at the top, it seemed easier to copy/paste than to try to simplify and possible cause inaccuracies. Just scroll down.)

Things of note, and how it differs from the canonical Flow example:

1. As discovered elsewhere, using * as the return type for mapStateToProps deals with the issue where flow can't decide which branch of connect to use.
2. The error for a bad mapStateToProps function is woefully verbose. It can be fixed by giving by actually typing it: const mapStateToProps: ReduxProps = ({ foo: 1 }) where type ReduxProps = { foo: number }. I'll probably start doing that.
3. If you are providing ownProps and props from redux state, you'll want to separate them into OwnProps and then Props which intersects OwnProps and anything passed in from the redux connector.

@maxsalven I tried out the example you provided at the top and didn't have any trouble reproducing the error, see here.

EDIT 11/20/2017 @ 17:49 PST

Ideally, I'd like to not have to change anything about my annotations, i.e. adding a return type to mapStateToProps. It should be able to infer it as far as I can tell. It appears that dropping | DP from the type MapDispatchToProps in the libdef resolves the problem, as in:

  declare type MapDispatchToProps<A, OP: {}, DP: {}> = ((dispatch: Dispatch<A>, ownProps: OP) => DP)

But unfortunately that's not correct, MDTP should allow an object to be passed in... It's like flow can't distinguish between {} and void | null, which doesn't make any sense.

[maxsalven]

@maxsalven I tried out the example you provided at the top and didn't have any trouble reproducing the error, see here.

ComponentProps needs to include updateUser in this example, at which point your code breaks (you've set ComponentProps as the return type of mapStateToProps, which is incorrect, ComponentProps can also receive props from ContainerProps and the result of mapDispatchToProps).

So this code is 'correct', but Flow will now incorrectly report an error:

const updateUser = () => ({ type: "UPDATE_USER" });

type ComponentProps = {
  data: string,
  updateUser: any,
};

const MyComponent = ({ data, updateUser }: ComponentProps) => <div>{data}</div>;

const mapStateToProps = (state): ComponentProps => ({
  data: "test"
});

const connector: Connector<{}, ComponentProps> = connect(mapStateToProps, { updateUser });

export default connector(MyComponent);

To re-iterate, this code (where the data prop type is 'wrong'), will incorrectly pass in 0.59, but correctly fail in 0.54:

const updateUser = () => ({ type: "UPDATE_USER" });

type ComponentProps = {
  data: boolean,
  updateUser: any,
};

const MyComponent = ({ data, updateUser }: ComponentProps) => <div>{data}</div>;

const mapStateToProps = (state): * => ({
  data: "test"
});

const connector: Connector<{}, ComponentProps> = connect(mapStateToProps, { updateUser });

export default connector(MyComponent);

[bdrobinson]

@maxsalven The behaviour in Flow itself definitely changed between 0.54 and 0.55. I did some research and found that it is commit https://github.com/facebook/flow/commit/bd938abfb8970972d512847415d0f68563120480 that broke the react-redux libdefs (presumably either by introducing a bug, or changing the API somehow). I posted it on https://github.com/facebook/flow/issues/4939 but haven't heard any more about it.

[maxsalven]

Thanks @bdrobinson. I'm very worried that this has been broken for almost three months now. Any thoughts on how we can engage with a Flow core team member to help get this sorted? It's beyond my ability level to help engineer a fix.

@calebmer, any chance of some assistance for an issue that impacts a library as important as Redux?

[villesau]

Also @xdissent has made some findings related to usage of $SubType and $SuperType in react-redux libdefs: https://github.com/flowtype/flow-typed/pull/1317#issuecomment-340305043

That might not be strictly related to this specific problem though, but in the condition of react-redux libdefs in overall.

[AndrewSouthpaw]

ComponentProps needs to include updateUser in this example, at which point your code breaks (you've set ComponentProps as the return type of mapStateToProps, which is incorrect, ComponentProps can also receive props from ContainerProps and the result of mapDispatchToProps).

Oh, I didn't realize that, thanks for pointing it out @maxsalven.

What's also interesting is that, using your second example, it does detect an error in v0.59 when only mapStateToProps is passed:

import React from 'react'              
const updateUser = () => ({ type: "UPDATE_USER" });

type ComponentProps = {
  data: boolean,
  updateUser: any,
};

const MyComponent = ({ data, updateUser }: ComponentProps) => <div>{data}</div>;

const mapStateToProps = (state): * => ({
  data: "test"
});

const connector: Connector<{}, ComponentProps> = connect(mapStateToProps);

export default connector(MyComponent);

[maxsalven]

@AndrewSouthpaw

What's also interesting is that, using your second example, it does detect an error in v0.59 when only mapStateToProps is passed:

Yep, that’s mentioned in the very first post in this issue :)

[AndrewSouthpaw]

Quite so. There's so many open issues on it, I've lost track of what's already been said. 😆 Sorry for the redundant findings.

[TLadd]

Just want to point this out because it was a great source of confusion to me; I had module.system.node.resolve_dirname=src in my .flowconfig and so was being affected by https://github.com/facebook/flow/issues/5180 when testing out upgrading flow versions. From what I was seeing, I was thinking I was completely losing type coverage on the output of connect, which I assumed had to do with an incompatibility between flow and the libdef. It appears like a lot of what I was seeing was really caused by https://github.com/facebook/flow/issues/5180.

With flow v0.61, removing module.system.node.resolve_dirname=src (few workarounds discussed in #5180), and the current react-redux flow-typed libdef, it is at least mostly working again. I think there are still legitimate issues with the flow/react-redux libdef, but wanted to point this out in case it helps someone.

[AndrewSouthpaw]

I haven't looked thoroughly, but came upon the HigherOrderComponent type def in react-flow-types. Maybe that could give us some help.

[pigulla]

Any updates on the issue?

[villesau]

@pigulla I'm working on better react-redux connect types, see: https://github.com/flowtype/flow-typed/pull/1731 Mostly just mergeProps is missing and maybe some fine tuning like where to pass dispatch and where not.

The issue with approach in my PR is that it won't work with stateless functional components which I assume to be a Flow bug.

[pigulla]

Thanks for the update, much appreciated.

[julienw]

In our project, we rewrote them like this: https://github.com/devtools-html/perf.html/blob/master/src/utils/connect.js

We decided to be very much more explicit than what the standard connect does but we found it a lot easier to deal with.

[Mr-Wallet]

Since this seems to be the focal point of current react-redux flow-typed issues:

Something missing from the discussions I've found so far that I feel bears mention is that the docs say:

in advanced scenarios where you need more control over the rendering performance, mapStateToProps() can also return a function. In this case, that function will be used as mapStateToProps() for a particular component instance.

This also applies to mapDispatchToProps. In my team's case this feature is vital for performance. In our case we solved it by modifying the function cases to have a union of return types:
((state: S, ownProps: OP) => (SP | ((state: S, ownProps: OP) => SP)))
((dispatch: Dispatch<A>, ownProps: OP) => (DP | ((dispatch: Dispatch<A>, ownProps: OP) => DP)))

This may be a separate issue, but it does seem related, and hopefully a PR for the issue at hand could holistically consider it (as in, at least not make things any worse for us).

[villesau]

react-redux libdefs have been rewritten twice since this, so I assume this is no longer valid.