Skip to content

Commit

Permalink
Fixes bug when initial mount of a host component is hidden (#12294)
Browse files Browse the repository at this point in the history
`oldProps` was null. This went uncaught by the unit tests because
ReactNoop did not use `oldProps` in either `prepareUpdate` or
`completeUpdate`. I added some invariants so we don't regress in
the future.
  • Loading branch information
acdlite authored Feb 27, 2018
1 parent 2f5eacc commit 8e5f12c
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 3 deletions.
9 changes: 9 additions & 0 deletions packages/react-noop-renderer/src/ReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,12 @@ let SharedHostConfig = {
oldProps: Props,
newProps: Props,
): null | {} {
if (oldProps === null) {
throw new Error('Should have old props');
}
if (newProps === null) {
throw new Error('Should have old props');
}
return UPDATE_SIGNAL;
},

Expand Down Expand Up @@ -196,6 +202,9 @@ const NoopRenderer = ReactFiberReconciler({
oldProps: Props,
newProps: Props,
): void {
if (oldProps === null) {
throw new Error('Should have old props');
}
instance.prop = newProps.prop;
},

Expand Down
14 changes: 13 additions & 1 deletion packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,18 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
// Normally we can bail out on props equality but if context has changed
// we don't do the bailout and we have to reuse existing props instead.
} else if (memoizedProps === nextProps) {
return bailoutOnAlreadyFinishedWork(current, workInProgress);
const isHidden =
workInProgress.mode & AsyncMode &&
shouldDeprioritizeSubtree(type, nextProps);
if (isHidden) {
// Before bailing out, make sure we've deprioritized a hidden component.
workInProgress.expirationTime = Never;
}
if (!isHidden || renderExpirationTime !== Never) {
return bailoutOnAlreadyFinishedWork(current, workInProgress);
}
// If we're rendering a hidden node at hidden priority, don't bailout. The
// parent is complete, but the children may not be.
}

let nextChildren = nextProps.children;
Expand Down Expand Up @@ -503,6 +514,7 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
// Down-prioritize the children.
workInProgress.expirationTime = Never;
// Bailout and come back to this fiber later.
workInProgress.memoizedProps = nextProps;
return null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,7 @@ describe('ReactIncrementalSideEffects', () => {
// items. Including the set state since that is deprioritized.
// TODO: The cycles it takes to do this could be lowered with further
// optimizations.
ReactNoop.flushDeferredPri(60 + 5);
ReactNoop.flushDeferredPri(35);
expect(ReactNoop.getChildren()).toEqual([
div(
// Updated.
Expand Down Expand Up @@ -790,7 +790,7 @@ describe('ReactIncrementalSideEffects', () => {
),
]);

expect(ops).toEqual(['Bar', 'Bar']);
expect(ops).toEqual(['Bar', 'Bar', 'Bar']);
});
// TODO: Test that side-effects are not cut off when a work in progress node
// moves to "current" without flushing due to having lower priority. Does this
Expand Down

0 comments on commit 8e5f12c

Please sign in to comment.