Repository: reduxjs/react-redux
Pull request #1079 (2018-11-11 19:42:27)

Add connectAdvanced() tests
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.

netlify[bot]: Deploy preview for *react-redux-docs* ready!

Built with commit 8fb8a39385f2d91e6249f635945656df58137ea2

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](https://github.com/reduxjs/react-redux/blob/5cdb83a91ca0ad4414a978f56eb34392a1e3f9df/src/components/connectAdvanced.js#L136), [lines 192-196](https://github.com/reduxjs/react-redux/blob/5cdb83a91ca0ad4414a978f56eb34392a1e3f9df/src/components/connectAdvanced.js#L192-L196), and [lines 149-161](https://github.com/reduxjs/react-redux/blob/5cdb83a91ca0ad4414a978f56eb34392a1e3f9df/src/components/connectAdvanced.js#L149-L161).

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](https://github.com/reduxjs/react-redux/pull/1000#issuecomment-436140264), 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.

epeli: 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](https://github.com/reduxjs/react-redux/pull/1079/files#diff-64a5723fe9f86fb29f940a32c5b863e9R80)" -test since that feels the most bug-like to me?

The intention is to test this statement in 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:

```js
        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:

```js
        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` ?

Patch:
diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js
index fee9776..066776e 100644
--- a/src/components/connectAdvanced.js
+++ b/src/components/connectAdvanced.js
@@ -203,11 +203,7 @@ export default function connectAdvanced(
           store
         )
 
-        if (pure) {
-          return this.selectChildElement(derivedProps, forwardedRef)
-        }
-
-        return <FinalWrappedComponent {...derivedProps} ref={forwardedRef} />
+        return this.selectChildElement(derivedProps, forwardedRef)
       }
 
       render() {


Test patch:
diff --git a/test/components/connectAdvanced.spec.js b/test/components/connectAdvanced.spec.js
new file mode 100644
index 0000000..6a47c85
--- /dev/null
+++ b/test/components/connectAdvanced.spec.js
@@ -0,0 +1,183 @@
+import React, { Component } from 'react'
+import * as rtl from 'react-testing-library'
+import { Provider as ProviderMock, connectAdvanced } from '../../src/index.js'
+import { createStore } from 'redux'
+import 'jest-dom/extend-expect'
+
+describe('React', () => {
+  describe('connectAdvanced', () => {
+    it('should map state and render once on mount', () => {
+      const initialState = {
+        foo: 'bar'
+      }
+
+      let mapCount = 0
+      let renderCount = 0
+
+      const store = createStore(() => initialState)
+
+      function Inner(props) {
+        renderCount++
+        return <div data-testid="foo">{JSON.stringify(props)}</div>
+      }
+
+      const Container = connectAdvanced(() => {
+        return state => {
+          mapCount++
+          return state
+        }
+      })(Inner)
+
+      const tester = rtl.render(
+        <ProviderMock store={store}>
+          <Container />
+        </ProviderMock>
+      )
+
+      expect(tester.getByTestId('foo')).toHaveTextContent('bar')
+
+      expect(mapCount).toEqual(1)
+      expect(renderCount).toEqual(1)
+    })
+
+    it('should render on reference change', () => {
+      let mapCount = 0
+      let renderCount = 0
+
+      // force new reference on each dispatch
+      const store = createStore(() => ({
+        foo: 'bar'
+      }))
+
+      function Inner(props) {
+        renderCount++
+        return <div data-testid="foo">{JSON.stringify(props)}</div>
+      }
+
+      const Container = connectAdvanced(() => {
+        return state => {
+          mapCount++
+          return state
+        }
+      })(Inner)
+
+      rtl.render(
+        <ProviderMock store={store}>
+          <Container />
+        </ProviderMock>
+      )
+
+      store.dispatch({ type: 'NEW_REFERENCE' })
+
+      // Should have mapped the state on mount and on the dispatch
+      expect(mapCount).toEqual(2)
+
+      // Should have rendered on mount and after the dispatch bacause the map
+      // state returned new reference
+      expect(renderCount).toEqual(2)
+    })
+
+    it('should not render when the returned reference does not change', () => {
+      const staticReference = {
+        foo: 'bar'
+      }
+
+      let mapCount = 0
+      let renderCount = 0
+
+      // force new reference on each dispatch
+      const store = createStore(() => ({
+        foo: 'bar'
+      }))
+
+      function Inner(props) {
+        renderCount++
+        return <div data-testid="foo">{JSON.stringify(props)}</div>
+      }
+
+      const Container = connectAdvanced(() => {
+        return () => {
+          mapCount++
+          // but return static reference
+          return staticReference
+        }
+      })(Inner)
+
+      const tester = rtl.render(
+        <ProviderMock store={store}>
+          <Container />
+        </ProviderMock>
+      )
+
+      store.dispatch({ type: 'NEW_REFERENCE' })
+
+      expect(tester.getByTestId('foo')).toHaveTextContent('bar')
+
+      // The state should have been mapped twice: on mount and on the dispatch
+      expect(mapCount).toEqual(2)
+
+      // But the render should have been called only on mount since the map state
+      // did not return a new reference
+      expect(renderCount).toEqual(1)
+    })
+
+    it('should map state on own props change but not render when the reference does not change', () => {
+      const staticReference = {
+        foo: 'bar'
+      }
+
+      let mapCount = 0
+      let renderCount = 0
+
+      const store = createStore(() => staticReference)
+
+      function Inner(props) {
+        renderCount++
+        return <div data-testid="foo">{JSON.stringify(props)}</div>
+      }
+
+      const Container = connectAdvanced(() => {
+        return () => {
+          mapCount++
+          // return the static reference
+          return staticReference
+        }
+      })(Inner)
+
+      class OuterComponent extends Component {
+        constructor() {
+          super()
+          this.state = { foo: 'FOO' }
+        }
+
+        setFoo(foo) {
+          this.setState({ foo })
+        }
+
+        render() {
+          return (
+            <div>
+              <Container {...this.state} />
+            </div>
+          )
+        }
+      }
+
+      let outerComponent
+      rtl.render(
+        <ProviderMock store={store}>
+          <OuterComponent ref={c => (outerComponent = c)} />
+        </ProviderMock>
+      )
+
+      outerComponent.setFoo('BAR')
+
+      // map on mount and on prop change
+      expect(mapCount).toEqual(2)
+
+      // render only on mount but skip on prop change because no new
+      // reference was returned
+      expect(renderCount).toEqual(1)
+    })
+  })
+})