React-redux: Use transactional setState() inside connect()

[gaearon]

We currently delay calling mapStateToProps and mapDispatchToProps in the common case because it is too early to supply ownProps inside handleChange, as those props may themselves be obtained from the previous version of the state by the parent component, and may not have been received as the new props just yet, so we bump into issues with staleness (#86).

Right now we call them inside render() so ownProps are always up-to-date but this is a bit odd, makes the code complicated and in some cases robs us of possible performance improvements, as caching the element isn’t quite as good as providing shouldComponentUpdate (#366).

One possible solution to this would be to use the transactional setState() overlord overload.
It is described in the documentation:

It's also possible to pass a function with the signature function(state, props). This can be useful in some cases when you want to enqueue an atomic update that consults the previous value of state+props before setting any values. For instance, suppose we wanted to increment a value in state:

setState(function(previousState, currentProps) {
  return {myInteger: previousState.myInteger + 1};
});

This looks pretty much exactly like our use case, and I have a feeling that this would let us use setState normally without having to resort to calling mapStateToProps and friends in render(). In this case, we would probably keep the merged props rather than the store state, in the local state.

We still want to keep the “fast path” that avoids setState altogether in handleChange, but other than that, the rest of the code should be vastly simplified by this, or at least become more conventional.

I’m still not 100% sure this is going to work, but I encourage anyone who wants to dig deeper into how React and Redux work together to give this a try. We have an extensive test suite that should cover any call count regressions. If you are working on this, please leave a comment in this issue.

[amccloud]

Looking into it! I'm trying to wrap my head around what would no longer be necessary. Looks like this could clean up quite a bit of hand holding around haveStatePropsBeenPrecalculated and stateProps in render().

[amccloud]

Making progress. 8 failing test mostly around invocation counts. Will work on this more tomorrow.

[gaearon]

Awesome! If you get stuck please feel free to share your work in progress so somebody can pick it up later.

[slorber]

Really happy to see all those red lines on first commits :)

[udnisap]

Approach

My approach was to use haveMergedPropsChanged as the flag for shouldComponentUpdate and call
updateDispatchPropsIfNeeded updateStatePropsIfNeeded based on the requirement.

I also added the following lines which needs to be run if required.

componentWillMount(){
        this.updateDispatchPropsIfNeeded();
        this.updateStatePropsIfNeeded();
        this.haveMergedPropsChanged = this.updateMergedPropsIfNeeded();
      }

Whats pending

conditionally run updateDispatchPropsIfNeeded updateStatePropsIfNeeded based on the requirement.
Currently 8 tests are failing

1) should ignore deep mutations in props
2) should invoke mapState every time props are changed if it has zero arguments
3) should invoke mapState every time props are changed if it has a second argument
4) should shallowly compare the merged state to prevent unnecessary updates
5) should recalculate the state and rebind the actions on hot update
6) should not render the wrapped component when mapState does not produce change
7) should bail out early if mapState does not depend on props
8) should allow providing a factory function to mapStateToProps

I can work on it tomorrow. If someone wants to continue feel free to use what I have done.

[amccloud]

https://github.com/amccloud/react-redux/commit/d2a1fe3bc317e39ed49ffedb53a9835896a5015b

@gaearon Now i'm stuck! I've gotten all but 3 test to pass:

1) should recalculate the state and rebind the actions on hot update
2) calls mapState and mapDispatch for impure components
3) should pass state consistently to mapState

I had to update one test now that re-rendering is now driven by setState. I think test failures 2 and 3 are similar issues. Test failure 1 is a bit different and @gaearon I was hoping you can look at it for me.

My notes: https://gist.github.com/amccloud/327a9d7b7dc97eaf6bb3f1f3da9e2d5e

[slorber]

@gaearon is there a reason why the test calls mapState and mapDispatch for impure components expect 2 calls to mapState/mapDispatch on mount? For me only one should be required.

[slorber]

@gaearon it was quite hard and the code is not clean yet but I made all tests pass!

You can take a look at [#373]

At first I thought that I would never be able to make pass the last test related to the stale props when both parent/child are connected, because the transactionnal setState callback can be called multiple times in a single batch.

I had to use a trick like that:

        this.lastSetStateFunction = setStateFunction
        this.setState((previousState, currentProps) => {
          if ( this.lastSetStateFunction === setStateFunction ) {
            return setStateFunction(previousState,currentProps)
          }
          else {
            return previousState
          }
        });

@tgriesser I also succeeded make all the tests pass without using checkMergedEquals. Do you think there might be a missing test case?