From f9f28f3854c23d4067495fee5b556fa391900533 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 6 Nov 2018 19:54:14 +0000 Subject: [PATCH] Fix lazy() with defaultProps (#14112) * Resolve defaultProps for Lazy components * Make test fail again * Undo the partial fix * Make test output more compact * Add a separate failing test for sync mode * Clean up tests * Add another update to both tests * Resolve props for commit phase lifecycles * Resolve prevProps for begin phase lifecycles * Resolve prevProps for pre-commit lifecycles * Only resolve props if element type differs * Fix Flow * Don't set instance.props/state during commit phase This is an optimization. I'm not sure it's entirely safe. It's probably worth running internal tests and see if we can ever trigger a case where they're different. This can mess with resuming. * Keep setting instance.props/state before unmounting This reverts part of the previous commit. It broke a test that verifies we use current props in componentWillUnmount if the fiber unmounts due to an error. --- .../src/ReactFiberBeginWork.js | 20 +- .../src/ReactFiberClassComponent.js | 6 +- .../src/ReactFiberCommitWork.js | 30 ++- .../src/ReactFiberLazyComponent.js | 15 ++ .../src/__tests__/ReactLazy-test.internal.js | 179 ++++++++++++++++++ 5 files changed, 223 insertions(+), 27 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 67808381bf06a..5849d84ff1f84 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -103,7 +103,10 @@ import { resumeMountClassInstance, updateClassInstance, } from './ReactFiberClassComponent'; -import {readLazyComponentType} from './ReactFiberLazyComponent'; +import { + readLazyComponentType, + resolveDefaultProps, +} from './ReactFiberLazyComponent'; import { resolveLazyComponentTag, createFiberFromTypeAndProps, @@ -729,21 +732,6 @@ function updateHostText(current, workInProgress) { return null; } -function resolveDefaultProps(Component, baseProps) { - if (Component && Component.defaultProps) { - // Resolve default props. Taken from ReactElement - const props = Object.assign({}, baseProps); - const defaultProps = Component.defaultProps; - for (let propName in defaultProps) { - if (props[propName] === undefined) { - props[propName] = defaultProps[propName]; - } - } - return props; - } - return baseProps; -} - function mountLazyComponent( _current, workInProgress, diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 9a167e6f4c8ca..abfa43e1ea552 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -28,6 +28,7 @@ import warningWithoutStack from 'shared/warningWithoutStack'; import {REACT_CONTEXT_TYPE} from 'shared/ReactSymbols'; import {startPhaseTimer, stopPhaseTimer} from './ReactDebugFiberPerf'; +import {resolveDefaultProps} from './ReactFiberLazyComponent'; import {StrictMode} from './ReactTypeOfMode'; import { @@ -987,7 +988,10 @@ function updateClassInstance( const instance = workInProgress.stateNode; const oldProps = workInProgress.memoizedProps; - instance.props = oldProps; + instance.props = + workInProgress.type === workInProgress.elementType + ? oldProps + : resolveDefaultProps(workInProgress.type, oldProps); const oldContext = instance.context; const contextType = ctor.contextType; diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 138267f3fdd91..9a0ef664b68fd 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -60,6 +60,7 @@ import {onCommitUnmount} from './ReactFiberDevToolsHook'; import {startPhaseTimer, stopPhaseTimer} from './ReactDebugFiberPerf'; import {getStackByFiberInDevAndProd} from './ReactCurrentFiber'; import {logCapturedError} from './ReactFiberErrorLogger'; +import {resolveDefaultProps} from './ReactFiberLazyComponent'; import {getCommitTime} from './ReactProfilerTimer'; import {commitUpdateQueue} from './ReactUpdateQueue'; import { @@ -228,10 +229,13 @@ function commitBeforeMutationLifeCycles( const prevState = current.memoizedState; startPhaseTimer(finishedWork, 'getSnapshotBeforeUpdate'); const instance = finishedWork.stateNode; - instance.props = finishedWork.memoizedProps; - instance.state = finishedWork.memoizedState; + // We could update instance props and state here, + // but instead we rely on them being set during last render. + // TODO: revisit this when we implement resuming. const snapshot = instance.getSnapshotBeforeUpdate( - prevProps, + finishedWork.elementType === finishedWork.type + ? prevProps + : resolveDefaultProps(finishedWork.type, prevProps), prevState, ); if (__DEV__) { @@ -345,16 +349,21 @@ function commitLifeCycles( if (finishedWork.effectTag & Update) { if (current === null) { startPhaseTimer(finishedWork, 'componentDidMount'); - instance.props = finishedWork.memoizedProps; - instance.state = finishedWork.memoizedState; + // We could update instance props and state here, + // but instead we rely on them being set during last render. + // TODO: revisit this when we implement resuming. instance.componentDidMount(); stopPhaseTimer(); } else { - const prevProps = current.memoizedProps; + const prevProps = + finishedWork.elementType === finishedWork.type + ? current.memoizedProps + : resolveDefaultProps(finishedWork.type, current.memoizedProps); const prevState = current.memoizedState; startPhaseTimer(finishedWork, 'componentDidUpdate'); - instance.props = finishedWork.memoizedProps; - instance.state = finishedWork.memoizedState; + // We could update instance props and state here, + // but instead we rely on them being set during last render. + // TODO: revisit this when we implement resuming. instance.componentDidUpdate( prevProps, prevState, @@ -365,8 +374,9 @@ function commitLifeCycles( } const updateQueue = finishedWork.updateQueue; if (updateQueue !== null) { - instance.props = finishedWork.memoizedProps; - instance.state = finishedWork.memoizedState; + // We could update instance props and state here, + // but instead we rely on them being set during last render. + // TODO: revisit this when we implement resuming. commitUpdateQueue( finishedWork, updateQueue, diff --git a/packages/react-reconciler/src/ReactFiberLazyComponent.js b/packages/react-reconciler/src/ReactFiberLazyComponent.js index 8a002946ac076..cae98e4a1f599 100644 --- a/packages/react-reconciler/src/ReactFiberLazyComponent.js +++ b/packages/react-reconciler/src/ReactFiberLazyComponent.js @@ -12,6 +12,21 @@ import type {LazyComponent, Thenable} from 'shared/ReactLazyComponent'; import {Resolved, Rejected, Pending} from 'shared/ReactLazyComponent'; import warning from 'shared/warning'; +export function resolveDefaultProps(Component: any, baseProps: Object): Object { + if (Component && Component.defaultProps) { + // Resolve default props. Taken from ReactElement + const props = Object.assign({}, baseProps); + const defaultProps = Component.defaultProps; + for (let propName in defaultProps) { + if (props[propName] === undefined) { + props[propName] = defaultProps[propName]; + } + } + return props; + } + return baseProps; +} + export function readLazyComponentType(lazyComponent: LazyComponent): T { const status = lazyComponent._status; const result = lazyComponent._result; diff --git a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js index 22155831fbef8..afe24fe794cbf 100644 --- a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js @@ -282,6 +282,185 @@ describe('ReactLazy', () => { expect(root).toMatchRenderedOutput('SiblingB'); }); + it('sets defaultProps for modern lifecycles', async () => { + class C extends React.Component { + static defaultProps = {text: 'A'}; + state = {}; + + static getDerivedStateFromProps(props) { + ReactTestRenderer.unstable_yield( + `getDerivedStateFromProps: ${props.text}`, + ); + return null; + } + + constructor(props) { + super(props); + ReactTestRenderer.unstable_yield(`constructor: ${this.props.text}`); + } + + componentDidMount() { + ReactTestRenderer.unstable_yield( + `componentDidMount: ${this.props.text}`, + ); + } + + componentDidUpdate(prevProps) { + ReactTestRenderer.unstable_yield( + `componentDidUpdate: ${prevProps.text} -> ${this.props.text}`, + ); + } + + componentWillUnmount() { + ReactTestRenderer.unstable_yield( + `componentWillUnmount: ${this.props.text}`, + ); + } + + shouldComponentUpdate(nextProps) { + ReactTestRenderer.unstable_yield( + `shouldComponentUpdate: ${this.props.text} -> ${nextProps.text}`, + ); + return true; + } + + getSnapshotBeforeUpdate(prevProps) { + ReactTestRenderer.unstable_yield( + `getSnapshotBeforeUpdate: ${prevProps.text} -> ${this.props.text}`, + ); + return null; + } + + render() { + return ; + } + } + + const LazyClass = lazy(() => fakeImport(C)); + + const root = ReactTestRenderer.create( + }> + + , + { + unstable_isConcurrent: true, + }, + ); + + expect(root).toFlushAndYield(['Loading...']); + expect(root).toMatchRenderedOutput(null); + + await Promise.resolve(); + + expect(root).toFlushAndYield([ + 'constructor: A', + 'getDerivedStateFromProps: A', + 'A1', + 'componentDidMount: A', + ]); + + root.update( + }> + + , + ); + expect(root).toFlushAndYield([ + 'getDerivedStateFromProps: A', + 'shouldComponentUpdate: A -> A', + 'A2', + 'getSnapshotBeforeUpdate: A -> A', + 'componentDidUpdate: A -> A', + ]); + expect(root).toMatchRenderedOutput('A2'); + + root.update( + }> + + , + ); + expect(root).toFlushAndYield([ + 'getDerivedStateFromProps: A', + 'shouldComponentUpdate: A -> A', + 'A3', + 'getSnapshotBeforeUpdate: A -> A', + 'componentDidUpdate: A -> A', + ]); + expect(root).toMatchRenderedOutput('A3'); + }); + + it('sets defaultProps for legacy lifecycles', async () => { + class C extends React.Component { + static defaultProps = {text: 'A'}; + state = {}; + + UNSAFE_componentWillMount() { + ReactTestRenderer.unstable_yield( + `UNSAFE_componentWillMount: ${this.props.text}`, + ); + } + + UNSAFE_componentWillUpdate(nextProps) { + ReactTestRenderer.unstable_yield( + `UNSAFE_componentWillUpdate: ${this.props.text} -> ${nextProps.text}`, + ); + } + + UNSAFE_componentWillReceiveProps(nextProps) { + ReactTestRenderer.unstable_yield( + `UNSAFE_componentWillReceiveProps: ${this.props.text} -> ${ + nextProps.text + }`, + ); + } + + render() { + return ; + } + } + + const LazyClass = lazy(() => fakeImport(C)); + + const root = ReactTestRenderer.create( + }> + + , + ); + + expect(ReactTestRenderer).toHaveYielded(['Loading...']); + expect(root).toFlushAndYield([]); + expect(root).toMatchRenderedOutput('Loading...'); + + await Promise.resolve(); + + root.update( + }> + + , + ); + expect(ReactTestRenderer).toHaveYielded([ + 'UNSAFE_componentWillMount: A', + 'A1', + 'UNSAFE_componentWillReceiveProps: A -> A', + 'UNSAFE_componentWillUpdate: A -> A', + 'A2', + ]); + expect(root).toFlushAndYield([]); + expect(root).toMatchRenderedOutput('A2'); + + root.update( + }> + + , + ); + expect(ReactTestRenderer).toHaveYielded([ + 'UNSAFE_componentWillReceiveProps: A -> A', + 'UNSAFE_componentWillUpdate: A -> A', + 'A3', + ]); + expect(root).toFlushAndYield([]); + expect(root).toMatchRenderedOutput('A3'); + }); + it('includes lazy-loaded component in warning stack', async () => { const LazyFoo = lazy(() => { ReactTestRenderer.unstable_yield('Started loading');