Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
esamattis authored and markerikson committed Nov 11, 2018
1 parent 1dc56ca commit dba598b
Show file tree
Hide file tree
Showing 2 changed files with 184 additions and 5 deletions.
6 changes: 1 addition & 5 deletions src/components/connectAdvanced.js
Expand Up @@ -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() {
Expand Down
183 changes: 183 additions & 0 deletions 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)
})
})
})

0 comments on commit dba598b

Please sign in to comment.