Fix lazy() with defaultProps (#14112)

gaearon · web-flow · commit f777d196e094 · 2018-11-06T19:54:14.000Z
* 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.
diff --git 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
@@ -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
@@ -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
@@ -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<T>(lazyComponent: LazyComponent<T>): 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
@@ -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 <Text text={this.props.text + this.props.num} />;
+      }
+    }
+
+    const LazyClass = lazy(() => fakeImport(C));
+
+    const root = ReactTestRenderer.create(
+      <Suspense fallback={<Text text="Loading..." />}>
+        <LazyClass num={1} />
+      </Suspense>,
+      {
+        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(
+      <Suspense fallback={<Text text="Loading..." />}>
+        <LazyClass num={2} />
+      </Suspense>,
+    );
+    expect(root).toFlushAndYield([
+      'getDerivedStateFromProps: A',
+      'shouldComponentUpdate: A -> A',
+      'A2',
+      'getSnapshotBeforeUpdate: A -> A',
+      'componentDidUpdate: A -> A',
+    ]);
+    expect(root).toMatchRenderedOutput('A2');
+
+    root.update(
+      <Suspense fallback={<Text text="Loading..." />}>
+        <LazyClass num={3} />
+      </Suspense>,
+    );
+    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 <Text text={this.props.text + this.props.num} />;
+      }
+    }
+
+    const LazyClass = lazy(() => fakeImport(C));
+
+    const root = ReactTestRenderer.create(
+      <Suspense fallback={<Text text="Loading..." />}>
+        <LazyClass num={1} />
+      </Suspense>,
+    );
+
+    expect(ReactTestRenderer).toHaveYielded(['Loading...']);
+    expect(root).toFlushAndYield([]);
+    expect(root).toMatchRenderedOutput('Loading...');
+
+    await Promise.resolve();
+
+    root.update(
+      <Suspense fallback={<Text text="Loading..." />}>
+        <LazyClass num={2} />
+      </Suspense>,
+    );
+    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(
+      <Suspense fallback={<Text text="Loading..." />}>
+        <LazyClass num={3} />
+      </Suspense>,
+    );
+    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');
