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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[react-redux_v5.x.x] Connect the `Dispatch` type with actions in dispatch params #3035

Merged
merged 9 commits into from Jan 4, 2019

Conversation

Projects
None yet
4 participants
@peter-leonov
Copy link
Contributor

commented Dec 30, 2018

Long time no see :)

This PR solves the problem with actions in dispatch props have currently no connection to the Dispatch type argument of the connect() function. In practice this means that Flow will not stop us from returning any random type from an action creator given in mapDispatchToProps. Here is an example:

// first valid action with action creator of our app
type ValidAction1 = {|
  +type: 'VALID_ACTION_1'
|}
const validAction1 = () => ({type 'VALID_ACTION_1'})
// second valid action with action creator of our app
type ValidAction2 = {|
  +type: 'VALID_ACTION_2'
|}
const validAction2 = () => ({type 'VALID_ACTION_1'})

// combined type of all valid actions of our app
type Action = ValidAction1 | ValidAction2

// the dispatch type of our application, which should accept only valid actions of our app
// (vanilla dispatch() returns action as is)
type Dispatch = Action => Action

// here is an example of an invalid action creator (possibly a typo or copy-paste problem)
const invalidAction = () => { foo: 'bar' }

// no own props
type OwnProps = {||}
// this component is going to only dispatch things
type Props = {
  action1: typeof action1,
  action2: typeof action2,
  invalidAction: typeof invalidAction
}

// ... other boilerplate code here ...

// ERROR: currently at this point Flow does not warn us about the `invalidAction`, but is should warn
connect(Props, OwnProps, _, _, _, Dispatch)

We do check the mapStateToProps to get the real state type (if it's present in the code and given or inferable from the call) but fo dispatch the Dispatch type is just bogus and was added to be typed in the bright future. Hopefully, it is now :)

Also, I've fixed some minor things:

  • not passing dispatch to config and mergeProps arguments if no mapStateToProps parameter passed;
  • fixed type arguments order for the mergeProps part of typings (was a typo);
  • pass the empty object type {||} as stateProps to mergeProps and config instead of wrongly inferred empty type for the non present StateProps and DispatchProps.

As this PR makes the typings significantly stricter (and safer) but a bit harder to drop into a random project (especially, after the habits we've developed with the very forgiving mode of Flow <v0.85) we should discuss if and how to make this change smooth 馃槃

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

Merged

[react-redux_v5.x.x] Add support for Flow 0.89+ #3012

7 of 7 tasks complete
@@ -52,6 +52,8 @@ declare module "react-redux" {
// and provide the StateProps type to the SP type parameter.
| ((state: S, ownProps: OP) => (state: S, ownProps: OP) => SP);

declare type Bind<D> = <A, R>((...A) => R) => (...A) => $Call<D, R>;

This comment has been minimized.

Copy link
@peter-leonov

peter-leonov Dec 30, 2018

Author Contributor

This is what bindActionCreators() actually does.

This comment has been minimized.

Copy link
@Hypnosphi

Hypnosphi Feb 20, 2019

Contributor

Should the corresponding change be made to redux libdef as well?

options?: ?Options<S, OP, {||}, {| ...OP, ...DP |}>,
// Got error like inexact OwnProps is incompatible with exact object type?
// Just make your OP parameter an exact object.
): Connector<P, OP, {| ...OP, ...$ObjMap<DP, Bind<D>> |}>;

This comment has been minimized.

Copy link
@peter-leonov

peter-leonov Dec 30, 2018

Author Contributor

Here is the main piece of code in this PR:

$ObjMap<DP, Bind<D>>

In simple case it just make sure that the actions returned by the specified in DP action creators are compatible with the function type in D. That it's a function type is implicitly specified by the usage of $ObjMap.

What's more interesting, in case of redux-thunk the code above transforms the raw thunks of type (...A) => (...B) => R from mapDispatchToProps object to (...A) => R in the resulting Props. This thing makes it possible to use thunks returning something useful:

const thunkB = (id) => async (dispatch: Dispatch) => {
  const data = await fetchTodo(id);
  dispatch(todoLoaded(data));
}

const thunkA = async (dispatch: Dispatch) => {
  // dispatch returns the promise from thunkB
  await dispatch(thunkB('todo123'))
}

class TodoLoader ... {
  componentDidMount() {
    const { thunkA } = this.props;
    // so it does in the props too and the new typings support this
    thunkA.then(trackTodoLoaded);
  }
}
// ... and here the property returns a return value of thunk
// as dispatch calls it for us with `dispatch` and `getState`
thunk: () => Promise<number>,
};

This comment has been minimized.

Copy link
@peter-leonov

peter-leonov Dec 30, 2018

Author Contributor

This is a test case for redux-thunk as described in previous comment. Here you can see that DispatchProps type as returned from mapDispatchToProps is different to what the Props get. This is exactly because of what connect() does: it hides the arguments by wrapping the thunk value.

Btw, can it be considered as a good monad example, dear functional gurus? 馃

)(Com);
e.push(Connected);
<Connected passthrough="foo" />;
}

This comment has been minimized.

Copy link
@peter-leonov

peter-leonov Dec 30, 2018

Author Contributor

What do you think, is it time to split the tests for connect in multiple files by topic? Like test_connectMergeProps.js, test_connectStateOnly.js, test_connectConfig.js.

This comment has been minimized.

Copy link
@villesau

villesau Jan 4, 2019

Member

I think that could make sense! Good idea 馃憤

@EugeneZ

This comment has been minimized.

Copy link
Contributor

commented Dec 31, 2018

This looks great. Massive thanks for all the work you've been doing on these typings! I think there is one overload of connect missing, the one where mapStateToProps is provided, and the mapDispatchToProps is a map. (The corresponding overload where the third and fourth arguments are provided is present, however.)

In our code, at least, this seems to be the most commonly used overload.

I've taken a shot at typing these in our code based on your work and they seem to work well so far:

  // this difference is not important in the vanila redux,
  // but in case of usage with redux-thunk, the return type may differ.
  declare export function connect<-P, -OP, -SP, -DP, S, D>(
    // If you get error here try adding return type to your mapStateToProps function
    mapStateToProps: MapStateToProps<S, OP, SP>,
    mapDispatchToProps: DP,
    mergeProps?: null | void,
    options?: ?Options<S, OP, SP, {| ...OP, ...SP, ...DP |}>,
    // Got error like inexact OwnProps is incompatible with exact object type?
    // Just make your OP parameter an exact object.
  ): Connector<P, OP, {| ...OP, ...SP, ...$ObjMap<DP, Bind<D>> |}>;
@peter-leonov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 31, 2018

@EugeneZ 馃う鈥嶁檧锔 yes, you're right. It was not exactly forgotten, but embedded into the function case and thus typed incorrectly in case of thunks. Thanks a lot for trying out the yet very raw material! And happy new year 馃巹馃巺馃崑!

@villesau
Copy link
Member

left a comment

Good idea to split to files, and one (apparently?) typo :)

@@ -548,4 +548,140 @@ function testHoistConnectedComponent() {
<Connected passthrough={123} passthroughWithDefaultProp={456} forMapStateToProps={'data'}/>;
// OK with declared static property
Connected.myStatic;
//$ExpectError property `myStatic1` is missing in statics

This comment has been minimized.

Copy link
@villesau

villesau Jan 4, 2019

Member

you mean notStatic? :)

This comment has been minimized.

Copy link
@peter-leonov

peter-leonov Jan 4, 2019

Author Contributor

Eagle eye achievement unlocked!

)(Com);
e.push(Connected);
<Connected passthrough="foo" />;
}

This comment has been minimized.

Copy link
@villesau

villesau Jan 4, 2019

Member

I think that could make sense! Good idea 馃憤

@peter-leonov peter-leonov force-pushed the peter-leonov:react-redux-type-dispatch branch from a1d6edc to 9ea1430 Jan 4, 2019

@peter-leonov

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2019

Rebased to hopefully get green tests. Will split into files asap.

peter-leonov added some commits Jan 4, 2019

remove the duplicated annotated tests
Also removed type parameters and added a clarifying comment.

@villesau villesau merged commit 5848599 into flow-typed:master Jan 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@villesau

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

Nice, GJ!

@peter-leonov peter-leonov deleted the peter-leonov:react-redux-type-dispatch branch Jan 5, 2019

@glasserc glasserc referenced this pull request Jan 31, 2019

Merged

Update flow-bin to 0.91.0 #731

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.