Skip to content

Commit 1a039f2

Browse files
committed
Fixes bug when initial mount of a host component is hidden
`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.
1 parent 6d7c847 commit 1a039f2

File tree

3 files changed

+26
-3
lines changed

3 files changed

+26
-3
lines changed

packages/react-noop-renderer/src/ReactNoop.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,12 @@ let SharedHostConfig = {
130130
oldProps: Props,
131131
newProps: Props,
132132
): null | {} {
133+
if (oldProps === null) {
134+
throw new Error('Should have old props');
135+
}
136+
if (newProps === null) {
137+
throw new Error('Should have old props');
138+
}
133139
return UPDATE_SIGNAL;
134140
},
135141

@@ -196,6 +202,9 @@ const NoopRenderer = ReactFiberReconciler({
196202
oldProps: Props,
197203
newProps: Props,
198204
): void {
205+
if (oldProps === null) {
206+
throw new Error('Should have old props');
207+
}
199208
instance.prop = newProps.prop;
200209
},
201210

packages/react-reconciler/src/ReactFiberBeginWork.js

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -417,7 +417,20 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
417417
// Normally we can bail out on props equality but if context has changed
418418
// we don't do the bailout and we have to reuse existing props instead.
419419
} else if (memoizedProps === nextProps) {
420-
return bailoutOnAlreadyFinishedWork(current, workInProgress);
420+
if (
421+
workInProgress.mode & AsyncMode &&
422+
shouldDeprioritizeSubtree(type, nextProps)
423+
) {
424+
if (renderExpirationTime === Never) {
425+
// Don't bailout.
426+
} else {
427+
// Bailout and deprioritize.
428+
workInProgress.expirationTime = Never;
429+
return bailoutOnAlreadyFinishedWork(current, workInProgress);
430+
}
431+
} else {
432+
return bailoutOnAlreadyFinishedWork(current, workInProgress);
433+
}
421434
}
422435

423436
let nextChildren = nextProps.children;
@@ -446,6 +459,7 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
446459
// Down-prioritize the children.
447460
workInProgress.expirationTime = Never;
448461
// Bailout and come back to this fiber later.
462+
workInProgress.memoizedProps = nextProps;
449463
return null;
450464
}
451465

packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.internal.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -758,7 +758,7 @@ describe('ReactIncrementalSideEffects', () => {
758758
// items. Including the set state since that is deprioritized.
759759
// TODO: The cycles it takes to do this could be lowered with further
760760
// optimizations.
761-
ReactNoop.flushDeferredPri(60 + 5);
761+
ReactNoop.flushDeferredPri(35);
762762
expect(ReactNoop.getChildren()).toEqual([
763763
div(
764764
// Updated.
@@ -790,7 +790,7 @@ describe('ReactIncrementalSideEffects', () => {
790790
),
791791
]);
792792

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

0 commit comments

Comments
 (0)