Add connectAdvanced() tests

[esamattis]

Since the connectAdvanced() is a public api I think it should have some tests. Unfortunately these tests are intentionally failing because I'm not sure how connectAdvanved() should really work. This is just my best guess. For example the behaviour is very different between 5.x and 6 beta. Different tests fail between those and in common failures the errors are different.

I have created redux-render-prop module which relies on the connectAdvanced() primitive and I noticed that all the important performance related tests started failing when I upgraded to 6 beta. Most importantly unrelated state updates don't cause render. I've added matching test in this PR called should not render when the returned reference does not change.

I've also observed that in 5.x the state mapping function gets called twice on component mount for which I had to create a very ugly workaround in redux-render-prop to avoid unnecessary rendering. I've added test for that case too (should map state and render once on mount). This seems to be fixed in the 6 beta.

I hope we can have more stable behaviour for the connectAdvanced() in future.

[netlify]

Deploy preview for react-redux-docs ready!

Built with commit 8d0cb87

https://deploy-preview-1079--react-redux-docs.netlify.com

[markerikson]

FWIW, the v5 behavior is actually kind of deliberate. We initialize the selector in the constructor and pass in the current props. The component may or may not actually need to subscribe to the store, so we check in the constructor if it needs to subscribe, and if so, run the selector again with the store state. You can see this in the v5.1.1 tag at line 136, lines 192-196, and lines 149-161.

The other overall issue is the behavior of connectAdvanced, and where and how we do the work.

In v5, each component is a separate subscriber to the store, and immediately runs its memoized selector in the subscriber callback. That means we can bail out immediately without React getting involved if there's not actually a need to update, and in fact, we force React to update by calling this.setState({}).

In v6, we've moved to a single subscriber up top in <Provider>, and are using a <Context.Provider> to make the store state accessible to nested components. That means React now has to walk to the tree to find the matching consumers. It also means that we have to specifically render a <Context.Consumer>, give it a render prop to access the data, and do the bulk of the work in ConnectAdvanced.renderWrappedChild().

However, we then memoize the actual rendered child component based on its props, which will cause React to skip rendering the child if nothing changed.

Overall, I agree that having more tests specifically for connectAdvanced would be a good thing. That said, my first reaction is that you might want to review your own library's tests and make sure that the assumptions there are correct.

[esamattis]

Fair enough about the v5 implementation if it's indeed intentional. I know it was quite complicated feat to get right.

But I'm fairly confident about my libary's tests since they don't test any implementation details and I've gone through its rendering behaviour very rigorously since it's really important to get right.

But can you take a closer look on the "should not render when the returned reference does not change" -test with v6 since that feels the most bug-like to me?

The intention is to test this statement from the api docs: "If a consecutive call to selector returns the same object (===) as its previous call, the component will not be re-rendered"

In the current v6 beta 2 I'm not able to bail out on rendering with connectAdvanced() at all.

[markerikson]

@epeli : okay, you may have a point here.

In the current build, the render logic ends with:

        let derivedProps = this.selectDerivedProps(
          storeState,
          wrapperProps,
          store
        )

        if (pure) {
          return this.selectChildElement(derivedProps, forwardedRef)
        }

        return <FinalWrappedComponent {...derivedProps} ref={forwardedRef} />
      }

Where pure only exists if the main connect function was used, but not with connectAdvanced.

However, yes, that does mean that the child component will end up getting re-rendered every time.

I just tried removing the branch and changing it to:

        let derivedProps = this.selectDerivedProps(
          storeState,
          wrapperProps,
          store
        )

        return this.selectChildElement(derivedProps, forwardedRef)

In other words, always check to see if we can memoize, not just when connectOptions.pure is true.

Looks like all the existing connect tests pass, and now all of your connectAdvanced tests pass.

I'll look these over a bit further, but yeah, I think we'll want to merge the tests and that small behavior change. Thanks - good catch!

[markerikson]

Additional question: how exactly are you actually making use of connectAdvanced, and what does it let you do that you can't do with the standard connect ?

[markerikson]

Merged. @epeli , please do comment when you get a chance with some details on how exactly you're using connectAdvanced. We really don't have many people using this API that I'm aware of, and I'm trying to get some info on how it's being used to decide whether we want to keep it long-term.

[esamattis]

Woo, thank you! V6 now looks much better target for redux-render-prop than V5. Sweet.

I'm actually not completely sure anymore why I "had to". The reason is mainly that it's easier to work with lower level abstraction when you are writing low level code. redux-render-prop works in the same space as connect() and when I tried to write it on top of connect() I felt that I had to fight against it. connectAdvanced() is just a simpler target.

But IRC one technical reason to go for connectAdvanced() was that it has to render when nothing changes (ie. no redux state or own prop changes) because the render prop might have some other reason to render. For example when it references this.state inside it. This situation comes up when class methods are used to flatten render props. This test demonstrates it:

https://github.com/epeli/redux-render-prop/blob/692c208874f947986fc583e15e5c0f3c2d13279d/__tests__/redux-render-prop.test.tsx#L898

and here's how I implement it using connectAdvanced():

https://github.com/epeli/redux-render-prop/blob/692c208874f947986fc583e15e5c0f3c2d13279d/src/redux-render-prop.ts#L171-L186

[markerikson]

@timdorr : can we cut a v6-beta3 release with this?

[timdorr]

Yeah, I'll get it out in a bit.

[timdorr]

Just pushed it out now: https://github.com/reduxjs/react-redux/releases/tag/v6.0.0-beta.3