Fix bailout broken in lazy components due to default props resolving (#18539)
* Add failing tests for lazy components * Fix bailout broken in lazy components due to default props resolving We should never compare unresolved props with resolved props. Since comparing resolved props by reference doesn't make sense, we use unresolved props in that case. Otherwise, resolved props are used. * Avoid reassigning props warning when we bailout
jddxf committed
Apr 8, 2020 at 17:58 UTC
241103a6fb8544077579c4c1458f367510d92934
3 files changed
+107
-11
packages/react-reconciler/src/ReactFiberBeginWork.js
+1
-1
@@ -856,7 +856,7 @@ function updateClassComponent(
856
);
857
if (__DEV__) {
858
const inst = workInProgress.stateNode;
859
- if (inst.props !== nextProps) {
859
+ if (shouldUpdate && inst.props !== nextProps) {
860
if (!didWarnAboutReassigningProps) {
861
console.error(
862
'It looks like %s is reassigning its own `this.props` while rendering. ' +
packages/react-reconciler/src/ReactFiberClassComponent.js
+15
-10
@@ -997,11 +997,13 @@ function updateClassInstance(
997
998
cloneUpdateQueue(current, workInProgress);
999
1000
- const oldProps = workInProgress.memoizedProps;
1001
- instance.props =
1000
+ const unresolvedOldProps = workInProgress.memoizedProps;
1001
+ const oldProps =
1002
workInProgress.type === workInProgress.elementType
1003
- ? oldProps
1004
- : resolveDefaultProps(workInProgress.type, oldProps);
1003
+ ? unresolvedOldProps
1004
+ : resolveDefaultProps(workInProgress.type, unresolvedOldProps);
1005
+ instance.props = oldProps;
1006
+ const unresolvedNewProps = workInProgress.pendingProps;
1007
1008
const oldContext = instance.context;
1009
const contextType = ctor.contextType;
@@ -1029,7 +1031,10 @@ function updateClassInstance(
1031
(typeof instance.UNSAFE_componentWillReceiveProps === 'function' ||
1032
typeof instance.componentWillReceiveProps === 'function')
1033
) {
1032
- if (oldProps !== newProps || oldContext !== nextContext) {
1034
+ if (
1035
+ unresolvedOldProps !== unresolvedNewProps ||
1036
+ oldContext !== nextContext
1037
+ ) {
1038
callComponentWillReceiveProps(
1039
workInProgress,
1040
instance,
@@ -1047,7 +1052,7 @@ function updateClassInstance(
1052
newState = workInProgress.memoizedState;
1053
1054
if (
1050
- oldProps === newProps &&
1055
+ unresolvedOldProps === unresolvedNewProps &&
1056
oldState === newState &&
1057
!hasContextChanged() &&
1058
!checkHasForceUpdateAfterProcessing()
@@ -1056,7 +1061,7 @@ function updateClassInstance(
1061
// effect even though we're bailing out, so that cWU/cDU are called.
1062
if (typeof instance.componentDidUpdate === 'function') {
1063
if (
1059
- oldProps !== current.memoizedProps ||
1064
+ unresolvedOldProps !== current.memoizedProps ||
1065
oldState !== current.memoizedState
1066
) {
1067
workInProgress.effectTag |= Update;
@@ -1064,7 +1069,7 @@ function updateClassInstance(
1069
}
1070
if (typeof instance.getSnapshotBeforeUpdate === 'function') {
1071
if (
1067
- oldProps !== current.memoizedProps ||
1072
+ unresolvedOldProps !== current.memoizedProps ||
1073
oldState !== current.memoizedState
1074
) {
1075
workInProgress.effectTag |= Snapshot;
@@ -1121,7 +1126,7 @@ function updateClassInstance(
1126
// effect even though we're bailing out, so that cWU/cDU are called.
1127
if (typeof instance.componentDidUpdate === 'function') {
1128
if (
1124
- oldProps !== current.memoizedProps ||
1129
+ unresolvedOldProps !== current.memoizedProps ||
1130
oldState !== current.memoizedState
1131
) {
1132
workInProgress.effectTag |= Update;
@@ -1129,7 +1134,7 @@ function updateClassInstance(
1134
}
1135
if (typeof instance.getSnapshotBeforeUpdate === 'function') {
1136
if (
1132
- oldProps !== current.memoizedProps ||
1137
+ unresolvedOldProps !== current.memoizedProps ||
1138
oldState !== current.memoizedState
1139
) {
1140
workInProgress.effectTag |= Snapshot;
packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js
+91
@@ -343,6 +343,97 @@ describe('ReactLazy', () => {
343
expect(root).toMatchRenderedOutput('SiblingB');
344
});
345
346
+ it('resolves defaultProps without breaking bailout due to unchanged props and state, #17151', async () => {
347
+ class LazyImpl extends React.Component {
348
+ static defaultProps = {value: 0};
349
+
350
+ render() {
351
+ const text = `${this.props.label}: ${this.props.value}`;
352
+ return <Text text={text} />;
353
+ }
354
+ }
355
+
356
+ const Lazy = lazy(() => fakeImport(LazyImpl));
357
+
358
+ const instance1 = React.createRef(null);
359
+ const instance2 = React.createRef(null);
360
+
361
+ const root = ReactTestRenderer.create(
362
+ <>
363
+ <LazyImpl ref={instance1} label="Not lazy" />
364
+ <Suspense fallback={<Text text="Loading..." />}>
365
+ <Lazy ref={instance2} label="Lazy" />
366
+ </Suspense>
367
+ </>,
368
+ {
369
+ unstable_isConcurrent: true,
370
+ },
371
+ );
372
+ expect(Scheduler).toFlushAndYield(['Not lazy: 0', 'Loading...']);
373
+ expect(root).not.toMatchRenderedOutput('Not lazy: 0Lazy: 0');
374
+
375
+ await Promise.resolve();
376
+
377
+ expect(Scheduler).toFlushAndYield(['Lazy: 0']);
378
+ expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0');
379
+
380
+ // Should bailout due to unchanged props and state
381
+ instance1.current.setState(null);
382
+ expect(Scheduler).toFlushAndYield([]);
383
+ expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0');
384
+
385
+ // Should bailout due to unchanged props and state
386
+ instance2.current.setState(null);
387
+ expect(Scheduler).toFlushAndYield([]);
388
+ expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0');
389
+ });
390
+
391
+ it('resolves defaultProps without breaking bailout in PureComponent, #17151', async () => {
392
+ class LazyImpl extends React.PureComponent {
393
+ static defaultProps = {value: 0};
394
+ state = {};
395
+
396
+ render() {
397
+ const text = `${this.props.label}: ${this.props.value}`;
398
+ return <Text text={text} />;
399
+ }
400
+ }
401
+
402
+ const Lazy = lazy(() => fakeImport(LazyImpl));
403
+
404
+ const instance1 = React.createRef(null);
405
+ const instance2 = React.createRef(null);
406
+
407
+ const root = ReactTestRenderer.create(
408
+ <>
409
+ <LazyImpl ref={instance1} label="Not lazy" />
410
+ <Suspense fallback={<Text text="Loading..." />}>
411
+ <Lazy ref={instance2} label="Lazy" />
412
+ </Suspense>
413
+ </>,
414
+ {
415
+ unstable_isConcurrent: true,
416
+ },
417
+ );
418
+ expect(Scheduler).toFlushAndYield(['Not lazy: 0', 'Loading...']);
419
+ expect(root).not.toMatchRenderedOutput('Not lazy: 0Lazy: 0');
420
+
421
+ await Promise.resolve();
422
+
423
+ expect(Scheduler).toFlushAndYield(['Lazy: 0']);
424
+ expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0');
425
+
426
+ // Should bailout due to shallow equal props and state
427
+ instance1.current.setState({});
428
+ expect(Scheduler).toFlushAndYield([]);
429
+ expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0');
430
+
431
+ // Should bailout due to shallow equal props and state
432
+ instance2.current.setState({});
433
+ expect(Scheduler).toFlushAndYield([]);
434
+ expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0');
435
+ });
436
+
437
it('sets defaultProps for modern lifecycles', async () => {
438
class C extends React.Component {
439
static defaultProps = {text: 'A'};