From d251b4d2a8b846024b3dd144f3227218dd63a886 Mon Sep 17 00:00:00 2001 From: Esa-Matti Suuronen Date: Sun, 11 Nov 2018 23:42:06 +0200 Subject: [PATCH] Add connectAdvanced() tests (#1079) 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. --- src/components/connectAdvanced.js | 6 +- test/components/connectAdvanced.spec.js | 183 ++++++++++++++++++++++++ 2 files changed, 184 insertions(+), 5 deletions(-) create mode 100644 test/components/connectAdvanced.spec.js diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index fee9776df..066776e6d 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 + return this.selectChildElement(derivedProps, forwardedRef) } render() { diff --git a/test/components/connectAdvanced.spec.js b/test/components/connectAdvanced.spec.js new file mode 100644 index 000000000..6a47c8579 --- /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
{JSON.stringify(props)}
+ } + + const Container = connectAdvanced(() => { + return state => { + mapCount++ + return state + } + })(Inner) + + const tester = rtl.render( + + + + ) + + 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
{JSON.stringify(props)}
+ } + + const Container = connectAdvanced(() => { + return state => { + mapCount++ + return state + } + })(Inner) + + rtl.render( + + + + ) + + 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
{JSON.stringify(props)}
+ } + + const Container = connectAdvanced(() => { + return () => { + mapCount++ + // but return static reference + return staticReference + } + })(Inner) + + const tester = rtl.render( + + + + ) + + 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
{JSON.stringify(props)}
+ } + + 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 ( +
+ +
+ ) + } + } + + let outerComponent + rtl.render( + + (outerComponent = c)} /> + + ) + + 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) + }) + }) +})