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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add connectAdvanced() tests #1079

Merged
merged 2 commits into from Nov 11, 2018

Conversation

esamattis
Copy link
Contributor

@esamattis esamattis commented Nov 11, 2018

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
Copy link

netlify bot commented Nov 11, 2018

Deploy preview for react-redux-docs ready!

Built with commit 8d0cb87

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

@markerikson
Copy link
Contributor

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
Copy link
Contributor Author

esamattis commented Nov 11, 2018

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
Copy link
Contributor

@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
Copy link
Contributor

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 markerikson merged commit dba598b into reduxjs:master Nov 11, 2018
@markerikson
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@timdorr
Copy link
Member

timdorr commented Nov 21, 2018

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

@timdorr
Copy link
Member

timdorr commented Nov 23, 2018

webguru7 pushed a commit to webguru7/react-redux that referenced this pull request Dec 8, 2022
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](https://github.com/epeli/redux-render-prop) module which relies on the connectAdvanced() primitive and I noticed that all the important performance related [tests started failing](https://travis-ci.com/epeli/redux-render-prop/jobs/157787740) when I upgraded to 6 beta. Most importantly [`unrelated state updates don't cause render`](https://github.com/epeli/redux-render-prop/blob/aa2aa9b299e5859f7a79bc1c926ed75907f63dcb/__tests__/redux-render-prop.test.tsx#L284-L349). 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](https://github.com/epeli/redux-render-prop/blob/aa2aa9b299e5859f7a79bc1c926ed75907f63dcb/src/redux-render-prop.ts#L188-L195) 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants