react-redux connect revamp

[villesau]

This is still very WIP and the API is not yet complete (API is now decent), so consider this more as a preview. For basic use cases with class based components these types works like a charm already (see tests).

Motivation for the rewrite is that currently connect libdefs are in really bad shape and doesn't catch if some of the required props are missing.

The main problem with this approach is that functional stateless components does not work like expected with this, see: facebook/flow#5652 E: sorted out, now they works!

Second problem is that exact object types are hard to get work with this.

@GAntoine am I missing something or is there a bug in Flow with either in $Diff or react stateless functional component type definitions? E: Needed to use React.ElementConfig

E: This starts to be ready from my side.

[villesau]

My changes starts below

[villesau]

Now when I made a bit of code archeology I found out that these types have been actually using $Diff before too. This PR moved from $Diff to $SuperType: #318
@gcanti do you remember the reasoning behind it?

[villesau]

Now when I made a bit of code archeology I found out that these types have been actually using $Diff before too. This PR moved from $Diff to $SuperType: #318
@gcanti do you remember the reasoning behind it?

[cprodescu]

I'm thinking about taking a stab at this as well. For me react-redux not working consistently is one of the main detractors to recommending flow more widely.
@villesau Are you still working on this? Do you have any updates?

Thank you!

[cprodescu]

I'm thinking about taking a stab at this as well. For me react-redux not working consistently is one of the main detractors to recommending flow more widely.
@villesau Are you still working on this? Do you have any updates?

Thank you!

[villesau]

@cprodescu yes I'm still working on this :) I've been busy with other things so haven't had time to push this forward. Next i'm going to implement mergeProps. Then i think it starts to be in decent shape. Note that the libdefs won't work with stateless functional components due to some Flow bug. I'd hope to get that fixed.

[villesau]

@cprodescu yes I'm still working on this :) I've been busy with other things so haven't had time to push this forward. Next i'm going to implement mergeProps. Then i think it starts to be in decent shape. Note that the libdefs won't work with stateless functional components due to some Flow bug. I'd hope to get that fixed.

[ancestorak]

@villesau What does "won't work" mean? Will it leave things as they are or break them further?

[ancestorak]

@villesau What does "won't work" mean? Will it leave things as they are or break them further?

[villesau]

@ancestorak It means that stateless components will not type check, you don't get any errors. So you can use stateless components but you don't get any help from Flow. See the issue and examples there: facebook/flow#5652

[villesau]

@ancestorak It means that stateless components will not type check, you don't get any errors. So you can use stateless components but you don't get any help from Flow. See the issue and examples there: facebook/flow#5652

[cprodescu]

@villesau Thank you for taking this on!
Out of the following requirements, which ones do you plan to tackle? Sorry for the verbosity, want to get an idea of which cases your solution plans to address.

1. Type-checking works when the class definition and usage are across different files.
   Current react-redux interface fails here, not trigger most ExpectError tests if you just separate current test_connect.js code into 3 files:

• component and prop definitions
• connector definitions
• usage of connected components

2. Type-checking works for class components with non-exact props
3. Type-checking works for class components with non-exact props and default props
4. Type-checking works for class components with exact props
5. Type-checking works for class components with exact props and default props
6. Type-checking works for stateless functions with non-exact props
7. Type-checking works for stateless functions with non-exact props and default props
8. Type-checking works for stateless functions with exact props
9. Type-checking works for stateless functions with exact props and default props

[cprodescu]

@villesau Thank you for taking this on!
Out of the following requirements, which ones do you plan to tackle? Sorry for the verbosity, want to get an idea of which cases your solution plans to address.

1. Type-checking works when the class definition and usage are across different files.
   Current react-redux interface fails here, not trigger most ExpectError tests if you just separate current test_connect.js code into 3 files:

• component and prop definitions
• connector definitions
• usage of connected components

2. Type-checking works for class components with non-exact props
3. Type-checking works for class components with non-exact props and default props
4. Type-checking works for class components with exact props
5. Type-checking works for class components with exact props and default props
6. Type-checking works for stateless functions with non-exact props
7. Type-checking works for stateless functions with non-exact props and default props
8. Type-checking works for stateless functions with exact props
9. Type-checking works for stateless functions with exact props and default props

[villesau]

@cprodescu I'm sorry but type definitions with exact props won't work. That's a Flow limitation: spreading exact props doesn't work right now, see facebook/flow#2405 . And also intersection between exact is impossible, e.g: {|a: number|} & {|b: number|} is impossible type: It can't match two exact types simultaneously. Thus, exact typing will not be supported any time soon at least. My approach is using intersection at least for now.

Stateless functional components will not give any errors until the bug in Flow is fixed. That's very unfortunate and my major concern with the approach. I still hope that when the types are in good enough shape otherwise, the bug will gain some traction on Flow side.

So:

1. Type-checking works when the class definition and usage are across different files.
   Current react-redux interface fails here, not trigger most ExpectError tests if you just separate current test_connect.js code into 3 files:

• component and prop definitions
• connector definitions
• usage of connected components
  • This is my number 1 goal. To get type checking working across different files by typing your functions and components props you pass to connect. And for class based components it's already the case, feel free to copy & paste the types and try out. Only mergeProps argument is missing. You can also check out the tests I've written so far, I hope they are more readable and understandable than the previous tests.

2. Type-checking works for class components with non-exact props
   • This works
3. Type-checking works for class components with non-exact props and default props
   • Not sure, can you give an example of this? E: works.
4. Type-checking works for class components with exact props
   • Exact props will not work E: Works and there is a test to demonstrate
5. Type-checking works for class components with exact props and default props
   • Exact props with default props will not work
6. Type-checking works for stateless functions with non-exact props
   • I hope to get the bug fixed once the types are in good shape. These should work exactly like with class based components. This works
7. Type-checking works for stateless functions with non-exact props and default props
   • example needed This works
8. Type-checking works for stateless functions with exact props
   • Exact props will not work E: Works and there is a test to demonstrate
9. Type-checking works for stateless functions with exact props and default props
   • Exact props with default props will not work

Would be really good if you could try out the definitions so that I get more feedback what works and what doesn't. I've tested this against our own large redux code base, but different teams might have different conventions so my current solution is aiming to solve our own use cases at first, which i hope to help others as much as possible too.

[villesau]

@cprodescu I'm sorry but type definitions with exact props won't work. That's a Flow limitation: spreading exact props doesn't work right now, see facebook/flow#2405 . And also intersection between exact is impossible, e.g: {|a: number|} & {|b: number|} is impossible type: It can't match two exact types simultaneously. Thus, exact typing will not be supported any time soon at least. My approach is using intersection at least for now.

Stateless functional components will not give any errors until the bug in Flow is fixed. That's very unfortunate and my major concern with the approach. I still hope that when the types are in good enough shape otherwise, the bug will gain some traction on Flow side.

So:

1. Type-checking works when the class definition and usage are across different files.
   Current react-redux interface fails here, not trigger most ExpectError tests if you just separate current test_connect.js code into 3 files:

• component and prop definitions
• connector definitions
• usage of connected components
  • This is my number 1 goal. To get type checking working across different files by typing your functions and components props you pass to connect. And for class based components it's already the case, feel free to copy & paste the types and try out. Only mergeProps argument is missing. You can also check out the tests I've written so far, I hope they are more readable and understandable than the previous tests.

2. Type-checking works for class components with non-exact props
   • This works
3. Type-checking works for class components with non-exact props and default props
   • Not sure, can you give an example of this? E: works.
4. Type-checking works for class components with exact props
   • Exact props will not work E: Works and there is a test to demonstrate
5. Type-checking works for class components with exact props and default props
   • Exact props with default props will not work
6. Type-checking works for stateless functions with non-exact props
   • I hope to get the bug fixed once the types are in good shape. These should work exactly like with class based components. This works
7. Type-checking works for stateless functions with non-exact props and default props
   • example needed This works
8. Type-checking works for stateless functions with exact props
   • Exact props will not work E: Works and there is a test to demonstrate
9. Type-checking works for stateless functions with exact props and default props
   • Exact props with default props will not work

Would be really good if you could try out the definitions so that I get more feedback what works and what doesn't. I've tested this against our own large redux code base, but different teams might have different conventions so my current solution is aiming to solve our own use cases at first, which i hope to help others as much as possible too.

[cprodescu]

This does not match the function you are passing in mapDispatchToProps: () => {}, you probably want () => void here since you are not returning an object from dispatch1

[villesau]

good catch! Seems that Flow cannot handle the return value of the function. Fortunately it understands what needs to be passed in, i'll add test for that.

[cprodescu]

state.a is number, forMapStateToProps is string

[villesau]

that's very true, thanks for pointing out! This had totally slipped trough my eyes. Seems that the inference chain gets broken here. Same with props. If state.a is replaced with e.g 'abc', tests starts to fail like they should.

This is one of the worst parts of Flow: It's really hard to know when Flow infers something as any

Interestingly, Flow versions prior to 0.57.1 catches the error, so it's clearly a regression in Flow.

[villesau]

wrote a bug report about this: facebook/flow#5792

[villesau]

I've now implemented first version of mergeProps. Seems to work decently.

Flow complains Cannot create MySFC element because props [1] is incompatible with empty [2].

when using stateless functional components and references to

79│ declare function connect<P: Object, R: Object, PR: Object>(

So basically Flow infers stateless functional components as empty (?) with these libdefs.

Workaround for this is to write SFC like: const MySFC: React.ComponentType<Props> = (props: Props) => <div /> which is suboptimal. The fact that Flow complains about this is better than failing silently though. But ideally I wouldn't need to write a type for stateless functional components.

Otherwise I would be OK with having to type SFCs, but it causes huge noise when taking the libdefs into use in large codebase.

Any ideas?

[villesau]

I've now implemented first version of mergeProps. Seems to work decently.

Flow complains Cannot create MySFC element because props [1] is incompatible with empty [2].

when using stateless functional components and references to

79│ declare function connect<P: Object, R: Object, PR: Object>(

So basically Flow infers stateless functional components as empty (?) with these libdefs.

Workaround for this is to write SFC like: const MySFC: React.ComponentType<Props> = (props: Props) => <div /> which is suboptimal. The fact that Flow complains about this is better than failing silently though. But ideally I wouldn't need to write a type for stateless functional components.

Otherwise I would be OK with having to type SFCs, but it causes huge noise when taking the libdefs into use in large codebase.

Any ideas?

[villesau]

Some known issues:

1. If your component is connected in different file where you pass mapStateToProps etc, libdefs doesn't work. You get Error:(40, 4) Cannot create 'MyComponent' element because props [1] is incompatible with empty [2]. error. Flow bug? ( #1331 )
2. In some rare cases where property is missing, error is shown in wrong place. Instead of component usage the error is shown where mapStateToProps etc are defined.
3. StatelessFunctionalComponents need to be separately typed like const MySFC: React.ComponentType<Props> = (props: Props) => <div /> ( facebook/flow#5652 ?) EDIT: Fixed with slight libdef changes!
4. exact types does not work ( facebook/flow#2405 )
5. There is a case where inference chain gets broken when passing variable straight from state to return props: ( #1731 (comment) )

But regardless of the issues, I've been able to take the current version of libdefs into use for rather large React & redux code base. Of course, that codebase has more or less unified way of using Redux with React so might be that I haven't been able to cover your use case. It's also unfortunate that SFCs needs to be typed, but on the other hand with these libdefs i've been able to catch good amount of real issues. If you write your react & redux like explained in the first list item, you might or might not be able to already get some benefit of this.

[villesau]

Some known issues:

1. If your component is connected in different file where you pass mapStateToProps etc, libdefs doesn't work. You get Error:(40, 4) Cannot create 'MyComponent' element because props [1] is incompatible with empty [2]. error. Flow bug? ( #1331 )
2. In some rare cases where property is missing, error is shown in wrong place. Instead of component usage the error is shown where mapStateToProps etc are defined.
3. StatelessFunctionalComponents need to be separately typed like const MySFC: React.ComponentType<Props> = (props: Props) => <div /> ( facebook/flow#5652 ?) EDIT: Fixed with slight libdef changes!
4. exact types does not work ( facebook/flow#2405 )
5. There is a case where inference chain gets broken when passing variable straight from state to return props: ( #1731 (comment) )

But regardless of the issues, I've been able to take the current version of libdefs into use for rather large React & redux code base. Of course, that codebase has more or less unified way of using Redux with React so might be that I haven't been able to cover your use case. It's also unfortunate that SFCs needs to be typed, but on the other hand with these libdefs i've been able to catch good amount of real issues. If you write your react & redux like explained in the first list item, you might or might not be able to already get some benefit of this.

[ryami333]

Just here to say that this refactor was well overdue and I'm very grateful that someone has decided to take this on - I've found the existing connect types almost entirely useless and the type integrity of all of my apps break down at the boundary between components and containers.

[ryami333]

Just here to say that this refactor was well overdue and I'm very grateful that someone has decided to take this on - I've found the existing connect types almost entirely useless and the type integrity of all of my apps break down at the boundary between components and containers.

[ryami333]

@villesau I just smoke-tested this type-def in my current project and it works 1000x better than the existing type defs. It's uncovered dozens of potential defects I wouldn't have caught otherwise. I can't wait until these are merged! 🍾 🎉

[ryami333]

@villesau I just smoke-tested this type-def in my current project and it works 1000x better than the existing type defs. It's uncovered dozens of potential defects I wouldn't have caught otherwise. I can't wait until these are merged! 🍾 🎉

[harjis]

I tested these typedefs in our project and they seemed to work a lot better than the previous ones. Thanks a lot for the effort! I can try to help out with these if I can some time next week.

ConnectOptions does not seem to be used. I think it is needed for usage like connect(mapStateToProps, null, null, { pure: false })(Component);

[villesau]

Good point about options! Our project does not use them so lack of that didn't got caught :) Fortunately that should be very trivial to implement!

[villesau]

@ryami333 Thanks for the encouraging feedback! To me these are now good enough at least for now, and we at Smartly are already using exactly this version of the libdefs :) For other usages i've been waiting for feedback and e.g options is pretty good catch already!

The test errors comes from previous libdefs, as some PRs with issues were merged when there was a period that flow-typed test suite didn't check for unused suppression.

From my side, I think I'll do some cleanup and then these should be good to go.

[villesau]

@ryami333 Thanks for the encouraging feedback! To me these are now good enough at least for now, and we at Smartly are already using exactly this version of the libdefs :) For other usages i've been waiting for feedback and e.g options is pretty good catch already!

The test errors comes from previous libdefs, as some PRs with issues were merged when there was a period that flow-typed test suite didn't check for unused suppression.

From my side, I think I'll do some cleanup and then these should be good to go.

[villesau]

@AndrewSouthpaw could you go trough this? The interface is now decent for at least my and apparently as well for @ryami333 s use case.

The test failures comes from previous libdefs. There was a period when $ExpectError cases were not checked and some pull requests slipped trough broken.

[villesau]

@AndrewSouthpaw could you go trough this? The interface is now decent for at least my and apparently as well for @ryami333 s use case.

The test failures comes from previous libdefs. There was a period when $ExpectError cases were not checked and some pull requests slipped trough broken.

[sibelius]

what is missing here?

[sibelius]

what is missing here?

[villesau]

@sibelius For my use case the libdefs are working decently, although there are some caveats e.g with SFC:s. Can you give them a try for the feedback?

[villesau]

@sibelius For my use case the libdefs are working decently, although there are some caveats e.g with SFC:s. Can you give them a try for the feedback?

[villesau]

@ahem Actually intersection is semantically more correct than spreading in this case. If you define exact types, you should define identical (same) types for all the parts which receives the props except with mergeProps where component receives result of mergeProps only. mapStateToProps, mapDispatchToProps and component all receives the same props, with an exception that component also receives pros from mapXtoProps functions. This is already supported, you can use exact types as long as they are identical, there is also tests for that. I think exact props should represent real, exact props, when inexact props might only represent what you are interested in.

@AndrewSouthpaw I wouldn't want to add anything else into scope at this point. Any possible problems can be handled by opening a new PR. This is already much more validated PR than most of others.

[villesau]

@ahem Actually intersection is semantically more correct than spreading in this case. If you define exact types, you should define identical (same) types for all the parts which receives the props except with mergeProps where component receives result of mergeProps only. mapStateToProps, mapDispatchToProps and component all receives the same props, with an exception that component also receives pros from mapXtoProps functions. This is already supported, you can use exact types as long as they are identical, there is also tests for that. I think exact props should represent real, exact props, when inexact props might only represent what you are interested in.

@AndrewSouthpaw I wouldn't want to add anything else into scope at this point. Any possible problems can be handled by opening a new PR. This is already much more validated PR than most of others.

[AndrewSouthpaw]

@villesau I think you misunderstood me, I'm with you in not wanting to add scope. :) I was making a note for others who happen to bump into the problem.

[AndrewSouthpaw]

@villesau I think you misunderstood me, I'm with you in not wanting to add scope. :) I was making a note for others who happen to bump into the problem.

[AndreasDahl]

Love the work in this PR so far, but the following code types, which i think i shouldn't

import React from 'react';

type OwnProps = {
    a: ?string,
}
                                      
type Props = {
    b: string,
}
                                      
class C extends React.Component<Props> {}
      
connect((state, ownProps : OwnProps) => ({
    b: ownProps.a,
}))(C);

[AndreasDahl]

Love the work in this PR so far, but the following code types, which i think i shouldn't

import React from 'react';

type OwnProps = {
    a: ?string,
}
                                      
type Props = {
    b: string,
}
                                      
class C extends React.Component<Props> {}
      
connect((state, ownProps : OwnProps) => ({
    b: ownProps.a,
}))(C);

[AndrewSouthpaw]

@AndreasDahl You can simply type the result of MSTP though. It's not perfect, but I think it's fair to keep it out of scope here. Feel free to raise a separate issue.

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

type OwnProps = {
  a: ?string,
}

type Props = {
  b: string,
}

class C extends React.Component<Props> {}

const mapStateToProps = (state, ownProps: OwnProps): Props => ({
  b: ownProps.a,
})

const ConnectedC = connect(mapStateToProps)(C)

const a = <ConnectedC a="foo" />

[AndrewSouthpaw]

@AndreasDahl You can simply type the result of MSTP though. It's not perfect, but I think it's fair to keep it out of scope here. Feel free to raise a separate issue.

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

type OwnProps = {
  a: ?string,
}

type Props = {
  b: string,
}

class C extends React.Component<Props> {}

const mapStateToProps = (state, ownProps: OwnProps): Props => ({
  b: ownProps.a,
})

const ConnectedC = connect(mapStateToProps)(C)

const a = <ConnectedC a="foo" />

[AndreasDahl]

@AndrewSouthpaw it looks to me like the only reason you get an error from your example is because you explicitly type return of mapStateToProps. If you instead write mapStateToProps as

const mapStateToProps = (state, ownProps: OwnProps) => ({
  b: ownProps.a,
})

Do you still get an error?

[AndreasDahl]

@AndrewSouthpaw it looks to me like the only reason you get an error from your example is because you explicitly type return of mapStateToProps. If you instead write mapStateToProps as

const mapStateToProps = (state, ownProps: OwnProps) => ({
  b: ownProps.a,
})

Do you still get an error?

[villesau]

@AndreasDahl @AndrewSouthpaw I think that's a bug in Flow it self. That specific case worked before 0.57.1: facebook/flow#5792

to my knowledge, there is not much what can be done on libdef side to tackle that problem :/

[villesau]

@AndreasDahl @AndrewSouthpaw I think that's a bug in Flow it self. That specific case worked before 0.57.1: facebook/flow#5792

to my knowledge, there is not much what can be done on libdef side to tackle that problem :/

[AndrewSouthpaw]

Merged. I think we've explored this PR long enough. @villesau, great work, really. You've tackled a critical and buggy part of Flow/React/Redux that many people could not wrap their heads around (including myself). I hope to grok Flow as deeply as you some day. 😅

[AndrewSouthpaw]

Merged. I think we've explored this PR long enough. @villesau, great work, really. You've tackled a critical and buggy part of Flow/React/Redux that many people could not wrap their heads around (including myself). I hope to grok Flow as deeply as you some day. 😅

[Ashoat]

As @AndrewSouthpaw suggested I opened an issue regarding the removal of the type parameterization in the return of MapStateToProps: #1934

@villesau, would really appreciate if you could just elaborate on your explanation in that issue. Totally buy that it's a Flow issue, just curious what the issue is specifically, and having an issue to track it won't hurt.

[Ashoat]

As @AndrewSouthpaw suggested I opened an issue regarding the removal of the type parameterization in the return of MapStateToProps: #1934

@villesau, would really appreciate if you could just elaborate on your explanation in that issue. Totally buy that it's a Flow issue, just curious what the issue is specifically, and having an issue to track it won't hurt.

[williamboman]

This is so awesome, thanks!!

I'm having issues getting this to work using the class decorator syntax. Is it known whether using decorators is supported or not? Currently working around this by:

class _Foo extends PureComponent<Props> {...}
export default connect(mapStateToProps)(_Foo)

[williamboman]

This is so awesome, thanks!!

I'm having issues getting this to work using the class decorator syntax. Is it known whether using decorators is supported or not? Currently working around this by:

class _Foo extends PureComponent<Props> {...}
export default connect(mapStateToProps)(_Foo)

[AndrewSouthpaw]

That's what my team does as well. I don't think class decorator syntax is supported, though I never tried. That'd be a question for the flow team.

[AndrewSouthpaw]

That's what my team does as well. I don't think class decorator syntax is supported, though I never tried. That'd be a question for the flow team.

[marcelmokos]

We were using Connector in our project. If the component is not directly in a function call will it type, how should we adjust the types?

const withSearchFormConnect: Connector<*, SearchFormProps> = connect(
 // mapStateToProps
  (state, ownProps) => ({
      ...ownProps,
     search: state.search,
  }),
  // mapDispatchToProps
  {
      setLocation: location => duck.actionCreator.setLocation({location}),
  }
);


const SearchForm = compose(withSearchFormConnect, withSomethigElse)(
  SearchFormPresenter,
);

[marcelmokos]

We were using Connector in our project. If the component is not directly in a function call will it type, how should we adjust the types?

const withSearchFormConnect: Connector<*, SearchFormProps> = connect(
 // mapStateToProps
  (state, ownProps) => ({
      ...ownProps,
     search: state.search,
  }),
  // mapDispatchToProps
  {
      setLocation: location => duck.actionCreator.setLocation({location}),
  }
);


const SearchForm = compose(withSearchFormConnect, withSomethigElse)(
  SearchFormPresenter,
);

[bgergen]

I am having a similar issue as @marcelmokos. We currently use Connector as well. I would like to update, but not sure what adjustments to make after removing Connector.

@villesau @AndrewSouthpaw Is there a recommended migration path to using the new libdef?

[bgergen]

I am having a similar issue as @marcelmokos. We currently use Connector as well. I would like to update, but not sure what adjustments to make after removing Connector.

@villesau @AndrewSouthpaw Is there a recommended migration path to using the new libdef?

[AndrewSouthpaw]

You don't need to add anything once you've removed Connector, you can look at the tests for an example. If it's not making sense, LMK, and I can post an example.

For migration, I did a few regex find/replaces with my IDE, it wasn't too painful, took maybe 15-30 minutes. Find/replace hit 95% of cases, then a few with custom syntax I cleaned up manually.

[AndrewSouthpaw]

You don't need to add anything once you've removed Connector, you can look at the tests for an example. If it's not making sense, LMK, and I can post an example.

For migration, I did a few regex find/replaces with my IDE, it wasn't too painful, took maybe 15-30 minutes. Find/replace hit 95% of cases, then a few with custom syntax I cleaned up manually.

[Ashoat]

Try just using the inferred type (*) instead of Connector

[Ashoat]

Try just using the inferred type (*) instead of Connector

[bgergen]

@AndrewSouthpaw Thanks, I looked through the tests and adjusted my code accordingly. It seems that I lose typechecking in the passing of props into the component being wrapped by connect. Previously, I could type my connect function as Connector<OwnProps, WrappedComponentProps>, with WrappedComponentProps having been imported from the wrapped component's file. With this configuration, if I tried to return an incorrect type from my mergeProps function, flow would catch it and give me an error. Now, without the Connector type, and without WrappedComponentProps explicitly imported, flow no longer catches these errors.

The tests make it seem like flow should be catching these errors. Got any ideas? I would say that I could still import WrappedComponentProps and use that as the return type from mergeProps, but that leaves me in a strange spot in components that don't use a mergeProps function. Is this possibly an issue that flow has with HOCs in general?

Edit: Using WrappedComponentProps as the return type from mergeProps doesn't seem to work consistently either.

[bgergen]

@AndrewSouthpaw Thanks, I looked through the tests and adjusted my code accordingly. It seems that I lose typechecking in the passing of props into the component being wrapped by connect. Previously, I could type my connect function as Connector<OwnProps, WrappedComponentProps>, with WrappedComponentProps having been imported from the wrapped component's file. With this configuration, if I tried to return an incorrect type from my mergeProps function, flow would catch it and give me an error. Now, without the Connector type, and without WrappedComponentProps explicitly imported, flow no longer catches these errors.

The tests make it seem like flow should be catching these errors. Got any ideas? I would say that I could still import WrappedComponentProps and use that as the return type from mergeProps, but that leaves me in a strange spot in components that don't use a mergeProps function. Is this possibly an issue that flow has with HOCs in general?

Edit: Using WrappedComponentProps as the return type from mergeProps doesn't seem to work consistently either.

[ryami333]

@bgergen are you using a recent release of Flow? I know that perhaps 3-4 months ago there was a bit of an overhaul to error handling and it fixed a bunch of errors being located in the wrong place (or not at all).

[ryami333]

@bgergen are you using a recent release of Flow? I know that perhaps 3-4 months ago there was a bit of an overhaul to error handling and it fixed a bunch of errors being located in the wrong place (or not at all).

[bgergen]

@ryami333 I'm on the newest version I believe: 0.67.1

I ran into a different issue. Props were being checked, but defaultProps in my wrapped components were causing all sorts of conflicts with the ElementConfig type in the libdef. Changing ElementConfig to ElementProps in the libdef seems to have fixed the issue.

@AndrewSouthpaw @villesau Does any of that make sense?

[bgergen]

@ryami333 I'm on the newest version I believe: 0.67.1

I ran into a different issue. Props were being checked, but defaultProps in my wrapped components were causing all sorts of conflicts with the ElementConfig type in the libdef. Changing ElementConfig to ElementProps in the libdef seems to have fixed the issue.

@AndrewSouthpaw @villesau Does any of that make sense?

[villesau]

@bgergen Can you provide an example code where libdefs fails? Without example it's not possible to try to figure out what is wrong.

[villesau]

@bgergen Can you provide an example code where libdefs fails? Without example it's not possible to try to figure out what is wrong.

[stryju]

Seems that this introduces regressions, like lack of getWrappedInstance() for connect-ed component.

Also, mapStateTo* is missing factory annotation:
edit: Ok, this seems to work if annotated properly :)

function mapStateToProps(initialState, initialOwnProps) => {
  // some per component instance optimization
  return (state, ownProps) => ({
    /* ... */
  });
}

[stryju]

Seems that this introduces regressions, like lack of getWrappedInstance() for connect-ed component.

Also, mapStateTo* is missing factory annotation:
edit: Ok, this seems to work if annotated properly :)

function mapStateToProps(initialState, initialOwnProps) => {
  // some per component instance optimization
  return (state, ownProps) => ({
    /* ... */
  });
}

[mike1808]

please add support for factory mapStateToProps

[mike1808]

please add support for factory mapStateToProps

[goodmind]

It infers empty even for class components. What am I missing?
Can't do $Diff on React.ElementConfig

[goodmind]

It infers empty even for class components. What am I missing?
Can't do $Diff on React.ElementConfig

[Hypnosphi]

Here, the component is expected to receive MDP as part of props shape. This assumes that the dispatch signature is A => A which is not true if you use almost any middleware (e.g. redux-thunk)

[stryju]

same goes for

declare export class Provider<S, A> extends React$Component<{
  store: Store<S, A>,
  children?: any
}> {}

should probably be

declare export class Provider<S, A, D = Dispatch<A>> extends React$Component<{
  store: Store<S, A, D>,
  children?: any
}> {}

[abeforgit]

I'm not sure if this is the appropriate place for it, but I'm not sure where else to mention this, it might save someone else the 5 hours I just lost.

While I haven't had any issues in getting type checking to work, the connect function provided here broke intellisense on every editor I've tried (webstorm, atom+nuclide, vscode). The solution I've found is to simply cast the connect call to the appropriate ComponentType, like so:

import type {ComponentType} from 'react'
import type {Props} from "./MyComponent"
import {MyComponent} from "./MyComponent"

// ...

export default (connect(mapStateToProps, mapDispatchToProps)(MyComponent): ComponentType<Props>)

Note it has to be ComponentType imported as above, React.Component imported from import * as React from 'react' will not work.

Edit: just realised, ComponentType is still available when using import * as React from 'react' just use as React.ComponentType

[abeforgit]

I'm not sure if this is the appropriate place for it, but I'm not sure where else to mention this, it might save someone else the 5 hours I just lost.

While I haven't had any issues in getting type checking to work, the connect function provided here broke intellisense on every editor I've tried (webstorm, atom+nuclide, vscode). The solution I've found is to simply cast the connect call to the appropriate ComponentType, like so:

import type {ComponentType} from 'react'
import type {Props} from "./MyComponent"
import {MyComponent} from "./MyComponent"

// ...

export default (connect(mapStateToProps, mapDispatchToProps)(MyComponent): ComponentType<Props>)

Note it has to be ComponentType imported as above, React.Component imported from import * as React from 'react' will not work.

Edit: just realised, ComponentType is still available when using import * as React from 'react' just use as React.ComponentType

[stryju]

@abeforgit I think you'll want to have a $Diff of your comopnent's config and store props, like:

type ConnectedComponent = ComponentType<$Diff<ElementConfig<ComponentType<Props>>, StoreProps>>;
export default (connect(mapStateToProps, mapDispatchToProps)(MyComponent): ConnectedComponent);

[stryju]

@abeforgit I think you'll want to have a $Diff of your comopnent's config and store props, like:

type ConnectedComponent = ComponentType<$Diff<ElementConfig<ComponentType<Props>>, StoreProps>>;
export default (connect(mapStateToProps, mapDispatchToProps)(MyComponent): ConnectedComponent);

[abeforgit]

@stryju Strangely enough, while your suggestion does provide intellisense, it stops the flow typechecking (as in it does not error when props are missing). However the below does work:

type connectedComponent = React.ComponentType<$Diff<React.ElementConfig<typeof MyComponent>, StoreProps>>

Which is weird since the type of the component should be ComponentType<Props>... which is the same as your suggestion...

Ill use this for now as it is better than my original solution, but it still feels like there should be a better way

[abeforgit]

@stryju Strangely enough, while your suggestion does provide intellisense, it stops the flow typechecking (as in it does not error when props are missing). However the below does work:

type connectedComponent = React.ComponentType<$Diff<React.ElementConfig<typeof MyComponent>, StoreProps>>

Which is weird since the type of the component should be ComponentType<Props>... which is the same as your suggestion...

Ill use this for now as it is better than my original solution, but it still feels like there should be a better way