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 type error when only passing in only mapStateToProps on flow v0.55.0 #1269

Closed
TLadd opened this Issue Sep 21, 2017 · 23 comments

Comments

Projects
None yet
@TLadd
Contributor

TLadd commented Sep 21, 2017

Previously on flow v0.54.1, the following code checked out fine

// @flow
import { connect, type Connector } from 'react-redux'

type State = {
  val: string,
}
type OwnProps = {
  other: string,
}
type Props = {
  other: string,
  val: string,
}

const mapStateToProps = (state: State) => {
  return {
    val: state.val,
  }
}

const connector: Connector<OwnProps, Props> = connect(mapStateToProps)

After upgrading flow to v0.55.0, flow gives an error

Error: src/components/body/custom-events/department-transfer-container.js:29
 29: const connector: Connector<OwnProps, Props> = connect(mapStateToProps)
                                                   ^^^^^^^^^^^^^^^^^^^^^^^^ function call. Could not decide which case to select
 29: const connector: Connector<OwnProps, Props> = connect(mapStateToProps)
                                                   ^^^^^^^ intersection type
  Case 3 may work:
                                 v--------------
   78:   declare function connect<S, A, OP, SP>(
   79:     mapStateToProps: MapStateToProps<S, OP, SP>,
   80:     mapDispatchToProps: Null,
  ...:
   83:   ): Connector<OP, $Supertype<SP & { dispatch: Dispatch<A> } & OP>>
         ----------------------------------------------------------------^ polymorphic type: function type. See lib: flow-typed/npm/react-redux_v5.x.x.js:78
  But if it doesn't, case 5 looks promising too:
                                 v------------------
   92:   declare function connect<S, A, OP, SP, DP>(
   93:     mapStateToProps: MapStateToProps<S, OP, SP>,
   94:     mapDispatchToProps: MapDispatchToProps<A, OP, DP>,
  ...:
   97:   ): Connector<OP, $Supertype<SP & DP & OP>>
         -----------------------------------------^ polymorphic type: function type. See lib: flow-typed/npm/react-redux_v5.x.x.js:92
  Please provide additional annotation(s) to determine whether case 3 works (or consider merging it with case 5):
   14: const mapStateToProps = (state: Object, ownProps: OwnProps) => {
                                                                     ^ return

I can resolve the error by passing in an empty mapDispatchToProps like so

const mapDispatchToProps = (dispatch: Dispatch<*>) => {
  return {}
}
const connector: Connector<OwnProps, Props> = connect(
  mapStateToProps,
  mapDispatchToProps
)

but hoping there might be a way to adjust the libdef to support not passing in mapDispatchToProps like before.

@TLadd TLadd changed the title from react-redux type error when only passing in mapStateToProps on flow v0.55.0 to react-redux type error when only passing in only mapStateToProps on flow v0.55.0 Sep 21, 2017

@echenley

This comment has been minimized.

Show comment
Hide comment
@echenley

echenley Sep 21, 2017

Contributor

To expand, setting a default type in the generic seems to work for declare type ..., but throws an Unexpected Token error when used for declare function.

// ideally this would work, but causes an Unexpected Token error at the =
declare function connect<S, A: {} = {}, OP, SP>(
  mapStateToProps: MapStateToProps<S, OP, SP>,
  mapDispatchToProps: Null,
  mergeProps: Null,
  options?: ConnectOptions
): Connector<OP, $Supertype<SP & { dispatch: Dispatch<A> } & OP>>;
Contributor

echenley commented Sep 21, 2017

To expand, setting a default type in the generic seems to work for declare type ..., but throws an Unexpected Token error when used for declare function.

// ideally this would work, but causes an Unexpected Token error at the =
declare function connect<S, A: {} = {}, OP, SP>(
  mapStateToProps: MapStateToProps<S, OP, SP>,
  mapDispatchToProps: Null,
  mergeProps: Null,
  options?: ConnectOptions
): Connector<OP, $Supertype<SP & { dispatch: Dispatch<A> } & OP>>;
@magopian

This comment has been minimized.

Show comment
Hide comment
@magopian

magopian Sep 22, 2017

To have the same functionality as when you're not providing a mapDispatchToProps (or using null) to have dispatch added to the props for you, here's what you can do to work around this bug:

export const ConnectedApp = connect(                                                                                                                                                                                                      
  mapStateToProps,                                                                                                                                                                                                                                     
  (dispatch: Dispatch) => ({dispatch: dispatch}),                                                                                                                                                                                         
)(App);

magopian commented Sep 22, 2017

To have the same functionality as when you're not providing a mapDispatchToProps (or using null) to have dispatch added to the props for you, here's what you can do to work around this bug:

export const ConnectedApp = connect(                                                                                                                                                                                                      
  mapStateToProps,                                                                                                                                                                                                                                     
  (dispatch: Dispatch) => ({dispatch: dispatch}),                                                                                                                                                                                         
)(App);
@sporto

This comment has been minimized.

Show comment
Hide comment
@sporto

sporto Sep 26, 2017

Annotating mapStateToProps has worked for me.

import type { MapStateToProps } from "react-redux"

const mapStateToProps: MapStateToProps<*, *, *> = (state: StateType) => {
  return ...
}

export default connect(mapStateToProps)(...)

Flow infers what version of connect to use this way.

sporto commented Sep 26, 2017

Annotating mapStateToProps has worked for me.

import type { MapStateToProps } from "react-redux"

const mapStateToProps: MapStateToProps<*, *, *> = (state: StateType) => {
  return ...
}

export default connect(mapStateToProps)(...)

Flow infers what version of connect to use this way.

magopian added a commit to mozilla/delivery-dashboard that referenced this issue Sep 26, 2017

magopian added a commit to mozilla/delivery-dashboard that referenced this issue Sep 26, 2017

@leethree

This comment has been minimized.

Show comment
Hide comment
@leethree

leethree Oct 11, 2017

Contributor

I'm seeing the same errors with Flow 0.56.0. I found that the | DP part will lead to Flow allowing empty mapDispatchToProps to be considered, thus lead Flow to think "case 5 looks promising".

  declare type MapDispatchToProps<A, OP: Object, DP: Object> =
    | ((dispatch: Dispatch<A>, ownProps: OP) => DP)
    | DP;

Removing | DP seems to work but it will likely lead to other issues:

  declare type MapDispatchToProps<A, OP: Object, DP: Object> =
    (dispatch: Dispatch<A>, ownProps: OP) => DP;
Contributor

leethree commented Oct 11, 2017

I'm seeing the same errors with Flow 0.56.0. I found that the | DP part will lead to Flow allowing empty mapDispatchToProps to be considered, thus lead Flow to think "case 5 looks promising".

  declare type MapDispatchToProps<A, OP: Object, DP: Object> =
    | ((dispatch: Dispatch<A>, ownProps: OP) => DP)
    | DP;

Removing | DP seems to work but it will likely lead to other issues:

  declare type MapDispatchToProps<A, OP: Object, DP: Object> =
    (dispatch: Dispatch<A>, ownProps: OP) => DP;
@adamesque

This comment has been minimized.

Show comment
Hide comment
@adamesque

adamesque Oct 18, 2017

We were wrestling with this issue, and it turned out the problem was that we weren't declaring a return type for our mapStateToProps function. Adding a signature like

function mapStateToProps(state: State): StateProps {
    return …;
}

resolved the type error. It was just an oversight on our part.

adamesque commented Oct 18, 2017

We were wrestling with this issue, and it turned out the problem was that we weren't declaring a return type for our mapStateToProps function. Adding a signature like

function mapStateToProps(state: State): StateProps {
    return …;
}

resolved the type error. It was just an oversight on our part.

@oreqizer

This comment has been minimized.

Show comment
Hide comment
@oreqizer

oreqizer Oct 22, 2017

Contributor

those of you battling the issue, this worked like a charm:

// BEFORE
const connector: Connector<OwnProps, Props> = connect((state: ReduxState, props: OwnProps) => ({
  fmt: dateFormatter(state, props),
}));

// AFTER
// just add ': *' to your map state to props function as the return type
const connector: Connector<OwnProps, Props> = connect((state: ReduxState, props: OwnProps): * => ({
  fmt: dateFormatter(state, props),
}));
Contributor

oreqizer commented Oct 22, 2017

those of you battling the issue, this worked like a charm:

// BEFORE
const connector: Connector<OwnProps, Props> = connect((state: ReduxState, props: OwnProps) => ({
  fmt: dateFormatter(state, props),
}));

// AFTER
// just add ': *' to your map state to props function as the return type
const connector: Connector<OwnProps, Props> = connect((state: ReduxState, props: OwnProps): * => ({
  fmt: dateFormatter(state, props),
}));
@igorz-

This comment has been minimized.

Show comment
Hide comment
@igorz-

igorz- Nov 14, 2017

Can someone explain, please, how existential type works and how it fixes the problem with a return type of mapStateToProps function?

The manual says:

The * can be thought of as an “auto” instruction to Flow, telling it to fill in the type from context.

But flow infers return type of function automatically even if it's not provided. Therefore I conclude that flow has two kinds of type inference and they work differently.

What's the difference between:

(state) => ({prop: state.someProp})

and:

(state): * => ({prop: state.someProp})

In first case the return type of function will be something like {prop: any}? And what return type will be in second case?

igorz- commented Nov 14, 2017

Can someone explain, please, how existential type works and how it fixes the problem with a return type of mapStateToProps function?

The manual says:

The * can be thought of as an “auto” instruction to Flow, telling it to fill in the type from context.

But flow infers return type of function automatically even if it's not provided. Therefore I conclude that flow has two kinds of type inference and they work differently.

What's the difference between:

(state) => ({prop: state.someProp})

and:

(state): * => ({prop: state.someProp})

In first case the return type of function will be something like {prop: any}? And what return type will be in second case?

@julienw

This comment has been minimized.

Show comment
Hide comment
@julienw

julienw Nov 14, 2017

Contributor

I believe this is because of the order Flow does things. From @calebmer on Twitter, Flow could infer without * and make the example work but the current code makes it difficult to do it.

Contributor

julienw commented Nov 14, 2017

I believe this is because of the order Flow does things. From @calebmer on Twitter, Flow could infer without * and make the example work but the current code makes it difficult to do it.

@igorz-

This comment has been minimized.

Show comment
Hide comment
@igorz-

igorz- Nov 14, 2017

I believe this is because of the order Flow does things. From @calebmer on Twitter, Flow could infer without * and make the example work but the current code makes it difficult to do it.

How does it work from technical point of view? Does * tells flow to do some "delayed" type inference (till there be more usage for inference to work properly)?

igorz- commented Nov 14, 2017

I believe this is because of the order Flow does things. From @calebmer on Twitter, Flow could infer without * and make the example work but the current code makes it difficult to do it.

How does it work from technical point of view? Does * tells flow to do some "delayed" type inference (till there be more usage for inference to work properly)?

@julienw

This comment has been minimized.

Show comment
Hide comment
@julienw

julienw Nov 14, 2017

Contributor

I really don't know -- I need to learn some CAML ;)

Contributor

julienw commented Nov 14, 2017

I really don't know -- I need to learn some CAML ;)

@igorz-

This comment has been minimized.

Show comment
Hide comment
@igorz-

igorz- Nov 14, 2017

I really don't know -- I need to learn some CAML ;)

Ok, thanks :) Maybe someone else could explain.

igorz- commented Nov 14, 2017

I really don't know -- I need to learn some CAML ;)

Ok, thanks :) Maybe someone else could explain.

@maxsalven

This comment has been minimized.

Show comment
Hide comment
@maxsalven

maxsalven Nov 15, 2017

I have a somewhat similar issue. My flow code type checks correctly when I don't use the second argument of connect, but doesn't catch errors when I do. Any ideas?

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


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?

maxsalven commented Nov 15, 2017

I have a somewhat similar issue. My flow code type checks correctly when I don't use the second argument of connect, but doesn't catch errors when I do. Any ideas?

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


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 comment has been minimized.

Show comment
Hide comment
@julienw

julienw Nov 15, 2017

Contributor

@maxsalven I believe this is a separate error, could you file a separate issue please ? :)

Contributor

julienw commented Nov 15, 2017

@maxsalven I believe this is a separate error, could you file a separate issue please ? :)

@ryami333

This comment has been minimized.

Show comment
Hide comment
@ryami333

ryami333 Nov 23, 2017

Undocumented as yet, but it may be pertinent to point out that Flow has now implemented a native $Connect utility type, which may or may not help resolve these HOC-related issues.

ryami333 commented Nov 23, 2017

Undocumented as yet, but it may be pertinent to point out that Flow has now implemented a native $Connect utility type, which may or may not help resolve these HOC-related issues.

@kusmierz

This comment has been minimized.

Show comment
Hide comment
@kusmierz

kusmierz Nov 27, 2017

Contributor

@ryami333 could you provide some POC?

Contributor

kusmierz commented Nov 27, 2017

@ryami333 could you provide some POC?

@ryami333

This comment has been minimized.

Show comment
Hide comment
@ryami333

ryami333 Nov 27, 2017

No sorry, I discovered its existence while digging about about the recompose lib-defs, but I don't really know much about it. It's sad that it's not documented either, despite having been in source for a few months.

ryami333 commented Nov 27, 2017

No sorry, I discovered its existence while digging about about the recompose lib-defs, but I don't really know much about it. It's sad that it's not documented either, despite having been in source for a few months.

@xdissent

This comment has been minimized.

Show comment
Hide comment
@xdissent

xdissent Nov 27, 2017

Contributor

@ryami333 do you mean $Compose?

Contributor

xdissent commented Nov 27, 2017

@ryami333 do you mean $Compose?

@ryami333

This comment has been minimized.

Show comment
Hide comment
@ryami333

ryami333 Nov 27, 2017

Yes - sorry! (It's still not documented though)

ryami333 commented Nov 27, 2017

Yes - sorry! (It's still not documented though)

@julienw julienw referenced this issue Dec 5, 2017

Closed

Remove PropTypes #661

@villesau

This comment has been minimized.

Show comment
Hide comment
@villesau

villesau Jan 14, 2018

Contributor

I'm rewriting the connect types here: #1731
Good: No any additional types like Connector etc required :)
Bad: It does not work with stateless functional components, see facebook/flow#5652 :(

Contributor

villesau commented Jan 14, 2018

I'm rewriting the connect types here: #1731
Good: No any additional types like Connector etc required :)
Bad: It does not work with stateless functional components, see facebook/flow#5652 :(

@julienw

This comment has been minimized.

Show comment
Hide comment
@julienw

julienw Jan 15, 2018

Contributor

My coworker did something here: https://github.com/gregtatum/perf.html/blob/97e8cdf328aed4452d88393ee48a07406b7bd8dd/src/utils/connect.js
(I still have to review it, so it may have errors)

Contributor

julienw commented Jan 15, 2018

My coworker did something here: https://github.com/gregtatum/perf.html/blob/97e8cdf328aed4452d88393ee48a07406b7bd8dd/src/utils/connect.js
(I still have to review it, so it may have errors)

@jedwards1211

This comment has been minimized.

Show comment
Hide comment
@jedwards1211

jedwards1211 Jan 23, 2018

Contributor

@adamesque not declaring the types for your mapStateToProps shouldn't confuse flow into considering cases with more than one argument though.

Contributor

jedwards1211 commented Jan 23, 2018

@adamesque not declaring the types for your mapStateToProps shouldn't confuse flow into considering cases with more than one argument though.

@jedwards1211

This comment has been minimized.

Show comment
Hide comment
@jedwards1211

jedwards1211 Jan 23, 2018

Contributor

Wouldn't it be possible to revert back to intersecting more function declarations until Flow bugs with Null arguments are fixed?

Contributor

jedwards1211 commented Jan 23, 2018

Wouldn't it be possible to revert back to intersecting more function declarations until Flow bugs with Null arguments are fixed?

@villesau

This comment has been minimized.

Show comment
Hide comment
@villesau

villesau Feb 18, 2018

Contributor

Anybody wants to give feedback about current state of this PR: #1731 ? How does the libdefs work in your code base? What works, what does not work? I've tested with large code base with decent results but every project has their own way of using (react-)redux so needs some testers.

Contributor

villesau commented Feb 18, 2018

Anybody wants to give feedback about current state of this PR: #1731 ? How does the libdefs work in your code base? What works, what does not work? I've tested with large code base with decent results but every project has their own way of using (react-)redux so needs some testers.

@GAntoine GAntoine added libdef bug and removed libdef-issue labels Mar 21, 2018

@GAntoine GAntoine closed this May 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment