Skip to content

add flow type definition file #389

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

Closed
wants to merge 2 commits into from
Closed

add flow type definition file #389

wants to merge 2 commits into from

Conversation

nmn
Copy link

@nmn nmn commented May 24, 2016

This adds a flow type declaration file so that anyone using it from NPM should just get the types 'for free'

It assumes the latest version of flow (0.25, though 0.24 should work too), and adds mostly adequate type definitions for both connect and Provider. Obviously most of the code is there to type connect which can get pretty tricky.

Known Issues: Flow correctly doesn't support stateless React components. The type definition for connect doesn't change that fact. (I tried to do a trick to make such that any stateless function component decorated with connect would just start getting type checked, but that didn't quite work in practice.)

@nmn nmn mentioned this pull request May 24, 2016
options?: {pure?: boolean, withRef?: boolean}
) => (component: (props: P) => any) => Class<React$Component<void, $Diff<P, DP>, void>>

type ConnectDisptch = <D, P, S, C: React$Component<D, P, S>, SP, Dispatch: Function>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be ConnectDispatch?

@nmn
Copy link
Author

nmn commented May 25, 2016

@gaearon I tried to support the case when mapDispatchToProps is an object, but I can't seem to find a way.

So far my entire type definition heavily relies on the magic type $Diff<A, B>. It's something flow uses internally to subtract DefaultProps from Props to get the external requirements.

It turns out, that B has to be a strict subset of A for it to work.
So $Diff<{name: string}, {name: any}> doesn't work.

But to support mapDispatchToProps is an object, I can't just subtract the value returned by mapDispatchToProps from props. I need to map into functions somehow, which I can't find any way to do.

I can still fix the type if you're willing to merge this. It's a little more wordy to write out the function version but I think people who use Flow will trade it for the improved type-safety.

@nmn
Copy link
Author

nmn commented May 25, 2016

Update: One other possibility is to use a more generic type:

type Connect = <D, P, S, C: React$Component<D, P, S>, SP, DP>(
  mapStateToProps?: (state: Object) => SP,
  mapDispatchToProps?: DP | (dispatch: Function) => DP,
  mergeProps?: (stateProps: SP, dispatchProps: DP, props: P) => Object,
  options?: {pure?: boolean, withRef?: boolean}
) => (component: Class<C>) => Class<React$Component<D, $Shape<P>, S>>

This has two downsides:

  1. You have to use undefined to skip arguments instead of null.
  2. The returned type is typed as strictly as the type defs defined in the current PR.

This type only checks that the props you HAVE provided have the right type. (e. g. It won't let you pass a string instead of a number) But it will let you completely skip required props and not warn you.

Do you think that is preferable?

@nmn
Copy link
Author

nmn commented Jun 5, 2016

Update:

  • I updated the type definitions to accept the options only as the 4th argument.
  • I chose to not include the mapDispatchToProps is an object case as after much research, I couldn't find a way to define types for that correctly. I assume people using flow will be willing to write slightly more code to get type-safety.

@agentcooper
Copy link

@nmn can you maybe give an example of the annotated connected component which is compatible with these type definitions?

btw, you can probably use type ConnectOptions = {pure?: boolean, withRef?: boolean} to decrease duplication.

@agentcooper
Copy link

agentcooper commented Jun 6, 2016

@nmn One more thing: you support ownProps param in mergeProps (as origProps), but not in mapStateToProps and mapDispatchToProps.

@nmn
Copy link
Author

nmn commented Jun 8, 2016

@agentcooper

  1. The idea is that if you type your own component you shouldn't need anything extra. So the example is to just use connect like normal!
  2. I think I missed the ownProps arg in the other two args. I'll add it in.

@timdorr timdorr mentioned this pull request Aug 27, 2016
7 tasks
@ccorcos
Copy link

ccorcos commented Aug 29, 2016

I think its quite important to have some typesafey around state and dispatch. State isnt just any object, its a specific object you're expecting within the application. And dispatch isnt just a function, but a function that accepts specific actions. Perhaps connect could have a type signature like Connects<State,Dispatch> so that I can define my state as an object literal type and dispatch as only accepting specific actions.

@agentcooper
Copy link

There are pretty good definitions available at reduxjs/redux#1887, for both redux and react-redux.

@ccorcos
Copy link

ccorcos commented Aug 30, 2016

thanks @agentcooper! those are some great lib defs!

@nmn
Copy link
Author

nmn commented Aug 31, 2016

@agentcooper Those type definitions do look interesting. They are definitely more sound theoretically. But in the past, the most correct definitions have not worked for me in practice.

I'll take these for a spin and report back, now that Flow has been updated a few times.

@timdorr
Copy link
Member

timdorr commented Nov 8, 2016

Same as #538: Can this be rebased against the next branch? I don't think typings showing up in a patch or minor would follow semver, since things might break. Plus, it'll keep the complaints down. Thanks!

@timdorr timdorr added this to the 5.0 milestone Nov 8, 2016
@mull
Copy link

mull commented Jan 11, 2017

Has this been abandoned?

@green-arrow
Copy link

Bump on this.

Also opened up a relevant issue on flow-typed for the shorthand notation of mapDispatchToProps: flow-typed/flow-typed#746

@nmn
Copy link
Author

nmn commented Mar 29, 2017

This PR is on the back-burner as flow-typed already has the type-defs.

There are some problems with function components that I'm trying to get fixed.

@@ -0,0 +1,77 @@
/* @flow */
type ConnectAll = <D, P, S, C: React$Component<D, P, S>, SP, DP, Dispatch: Function>(
mapStateToProps: (state: Object) => SP,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mapStateToProps can have additional parameter - own props of the connected component.

@timdorr
Copy link
Member

timdorr commented Dec 8, 2017

Closing for similar reasons to #814. Maintaining typings in-house is bad for DX.

@timdorr timdorr closed this Dec 8, 2017
@green-arrow
Copy link

@timdorr - I think you want #815 (looks like #814 was closed due to a wrong branch). Thanks for the update on this!

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

Successfully merging this pull request may close these issues.

8 participants