From 299ca45451655012e29486415d7e566cbc622fcb Mon Sep 17 00:00:00 2001 From: jddxf <740531372@qq.com> Date: Sat, 4 Apr 2020 23:58:34 +0800 Subject: [PATCH 1/3] Add failing tests for lazy components --- .../src/__tests__/ReactLazy-test.internal.js | 91 +++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js index d542173fce30e..33533a7545861 100644 --- a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js @@ -343,6 +343,97 @@ describe('ReactLazy', () => { expect(root).toMatchRenderedOutput('SiblingB'); }); + it('resolves defaultProps without breaking bailout due to unchanged props and state, #17151', async () => { + class LazyImpl extends React.Component { + static defaultProps = {value: 0}; + + render() { + const text = `${this.props.label}: ${this.props.value}`; + return ; + } + } + + const Lazy = lazy(() => fakeImport(LazyImpl)); + + const instance1 = React.createRef(null); + const instance2 = React.createRef(null); + + const root = ReactTestRenderer.create( + <> + + }> + + + , + { + unstable_isConcurrent: true, + }, + ); + expect(Scheduler).toFlushAndYield(['Not lazy: 0', 'Loading...']); + expect(root).not.toMatchRenderedOutput('Not lazy: 0Lazy: 0'); + + await Promise.resolve(); + + expect(Scheduler).toFlushAndYield(['Lazy: 0']); + expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0'); + + // Should bailout due to unchanged props and state + instance1.current.setState(null); + expect(Scheduler).toFlushAndYield([]); + expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0'); + + // Should bailout due to unchanged props and state + instance2.current.setState(null); + expect(Scheduler).toFlushAndYield([]); + expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0'); + }); + + it('resolves defaultProps without breaking bailout in PureComponent, #17151', async () => { + class LazyImpl extends React.PureComponent { + static defaultProps = {value: 0}; + state = {}; + + render() { + const text = `${this.props.label}: ${this.props.value}`; + return ; + } + } + + const Lazy = lazy(() => fakeImport(LazyImpl)); + + const instance1 = React.createRef(null); + const instance2 = React.createRef(null); + + const root = ReactTestRenderer.create( + <> + + }> + + + , + { + unstable_isConcurrent: true, + }, + ); + expect(Scheduler).toFlushAndYield(['Not lazy: 0', 'Loading...']); + expect(root).not.toMatchRenderedOutput('Not lazy: 0Lazy: 0'); + + await Promise.resolve(); + + expect(Scheduler).toFlushAndYield(['Lazy: 0']); + expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0'); + + // Should bailout due to shallow equal props and state + instance1.current.setState({}); + expect(Scheduler).toFlushAndYield([]); + expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0'); + + // Should bailout due to shallow equal props and state + instance2.current.setState({}); + expect(Scheduler).toFlushAndYield([]); + expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0'); + }); + it('sets defaultProps for modern lifecycles', async () => { class C extends React.Component { static defaultProps = {text: 'A'}; From 26c716f2ba54609fd749268e20c99a6622bc521c Mon Sep 17 00:00:00 2001 From: jddxf <740531372@qq.com> Date: Sun, 5 Apr 2020 00:41:51 +0800 Subject: [PATCH 2/3] 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. --- .../src/ReactFiberClassComponent.js | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 7b9be3544e713..dce415db1a24d 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -997,11 +997,13 @@ function updateClassInstance( cloneUpdateQueue(current, workInProgress); - const oldProps = workInProgress.memoizedProps; - instance.props = + const unresolvedOldProps = workInProgress.memoizedProps; + const oldProps = workInProgress.type === workInProgress.elementType - ? oldProps - : resolveDefaultProps(workInProgress.type, oldProps); + ? unresolvedOldProps + : resolveDefaultProps(workInProgress.type, unresolvedOldProps); + instance.props = oldProps; + const unresolvedNewProps = workInProgress.pendingProps; const oldContext = instance.context; const contextType = ctor.contextType; @@ -1029,7 +1031,10 @@ function updateClassInstance( (typeof instance.UNSAFE_componentWillReceiveProps === 'function' || typeof instance.componentWillReceiveProps === 'function') ) { - if (oldProps !== newProps || oldContext !== nextContext) { + if ( + unresolvedOldProps !== unresolvedNewProps || + oldContext !== nextContext + ) { callComponentWillReceiveProps( workInProgress, instance, @@ -1047,7 +1052,7 @@ function updateClassInstance( newState = workInProgress.memoizedState; if ( - oldProps === newProps && + unresolvedOldProps === unresolvedNewProps && oldState === newState && !hasContextChanged() && !checkHasForceUpdateAfterProcessing() @@ -1056,7 +1061,7 @@ function updateClassInstance( // effect even though we're bailing out, so that cWU/cDU are called. if (typeof instance.componentDidUpdate === 'function') { if ( - oldProps !== current.memoizedProps || + unresolvedOldProps !== current.memoizedProps || oldState !== current.memoizedState ) { workInProgress.effectTag |= Update; @@ -1064,12 +1069,13 @@ function updateClassInstance( } if (typeof instance.getSnapshotBeforeUpdate === 'function') { if ( - oldProps !== current.memoizedProps || + unresolvedOldProps !== current.memoizedProps || oldState !== current.memoizedState ) { workInProgress.effectTag |= Snapshot; } } + instance.props = workInProgress.memoizedProps = newProps; return false; } @@ -1121,7 +1127,7 @@ function updateClassInstance( // effect even though we're bailing out, so that cWU/cDU are called. if (typeof instance.componentDidUpdate === 'function') { if ( - oldProps !== current.memoizedProps || + unresolvedOldProps !== current.memoizedProps || oldState !== current.memoizedState ) { workInProgress.effectTag |= Update; @@ -1129,7 +1135,7 @@ function updateClassInstance( } if (typeof instance.getSnapshotBeforeUpdate === 'function') { if ( - oldProps !== current.memoizedProps || + unresolvedOldProps !== current.memoizedProps || oldState !== current.memoizedState ) { workInProgress.effectTag |= Snapshot; From 4e984ffd8a5f886a0134c4fc772a65ffb4ec77b3 Mon Sep 17 00:00:00 2001 From: jddxf <740531372@qq.com> Date: Wed, 8 Apr 2020 14:00:28 +0800 Subject: [PATCH 3/3] Avoid reassigning props warning when we bailout --- packages/react-reconciler/src/ReactFiberBeginWork.js | 2 +- packages/react-reconciler/src/ReactFiberClassComponent.js | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 3ad78e2a79269..f08189c3ef01b 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -856,7 +856,7 @@ function updateClassComponent( ); if (__DEV__) { const inst = workInProgress.stateNode; - if (inst.props !== nextProps) { + if (shouldUpdate && inst.props !== nextProps) { if (!didWarnAboutReassigningProps) { console.error( 'It looks like %s is reassigning its own `this.props` while rendering. ' + diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index dce415db1a24d..a688b891fa515 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -1075,7 +1075,6 @@ function updateClassInstance( workInProgress.effectTag |= Snapshot; } } - instance.props = workInProgress.memoizedProps = newProps; return false; }