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

fix timing issue in component updates due to consecutive dispatches #1263

Merged
merged 1 commit into from Apr 26, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/components/connectAdvanced.js
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
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