Skip to content

Commit

Permalink
fix timing issue in component updates due to consecutive dispatches (r…
Browse files Browse the repository at this point in the history
  • Loading branch information
MrWolfZ authored and timdorr committed Apr 26, 2019
1 parent 21c61ad commit 7da000b
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 1 deletion.
7 changes: 6 additions & 1 deletion src/components/connectAdvanced.js
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ export default function connectAdvanced(
const lastChildProps = useRef()
const lastWrapperProps = useRef(wrapperProps)
const childPropsFromStoreUpdate = useRef()
const renderIsScheduled = useRef(false)

const actualChildProps = usePureOnlyMemo(() => {
// Tricky logic here:
Expand Down Expand Up @@ -282,6 +283,7 @@ export default function connectAdvanced(
// We want to capture the wrapper props and child props we used for later comparisons
lastWrapperProps.current = wrapperProps
lastChildProps.current = actualChildProps
renderIsScheduled.current = false

// If the render was from a store update, clear out that reference and cascade the subscriber update
if (childPropsFromStoreUpdate.current) {
Expand Down Expand Up @@ -328,14 +330,17 @@ export default function connectAdvanced(

// If the child props haven't changed, nothing to do here - cascade the subscription update
if (newChildProps === lastChildProps.current) {
notifyNestedSubs()
if (!renderIsScheduled.current) {
notifyNestedSubs()
}
} else {
// Save references to the new child props. Note that we track the "child props from store update"
// as a ref instead of a useState/useReducer because we need a way to determine if that value has
// been processed. If this went into useState/useReducer, we couldn't clear out the value without
// forcing another re-render, which we don't want.
lastChildProps.current = newChildProps
childPropsFromStoreUpdate.current = newChildProps
renderIsScheduled.current = true

// If the child props _did_ change (or we caught an error), this wrapper component needs to re-render
forceComponentUpdateDispatch({
Expand Down
48 changes: 48 additions & 0 deletions test/components/connect.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -3182,6 +3182,54 @@ describe('React', () => {

expect(reduxCountPassedToMapState).toEqual(3)
})

it('should ensure top-down updates for consecutive batched updates', () => {
const INC = 'INC'
const reducer = (c = 0, { type }) => (type === INC ? c + 1 : c)
const store = createStore(reducer)

let executionOrder = []
let expectedExecutionOrder = [
'parent map',
'parent render',
'child map',
'child render'
]

const ChildImpl = () => {
executionOrder.push('child render')
return <div>child</div>
}

const Child = connect(state => {
executionOrder.push('child map')
return { state }
})(ChildImpl)

const ParentImpl = () => {
executionOrder.push('parent render')
return <Child />
}

const Parent = connect(state => {
executionOrder.push('parent map')
return { state }
})(ParentImpl)

rtl.render(
<ProviderMock store={store}>
<Parent />
</ProviderMock>
)

executionOrder = []
rtl.act(() => {
store.dispatch({ type: INC })
store.dispatch({ type: '' })
})

expect(executionOrder).toEqual(expectedExecutionOrder)
})
})

it("should enforce top-down updates to ensure a deleted child's mapState doesn't throw errors", () => {
Expand Down

0 comments on commit 7da000b

Please sign in to comment.