From 38b680eaaa9ebd7c9d44e5b15dc2573969a77125 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 12 Jul 2018 10:12:33 -0700 Subject: [PATCH 1/2] Suspending inside a constructor outside of strict mode Outside of strict mode, suspended components commit in an incomplete state, then are synchronously deleted in a subsequent commit. If a component suspends inside the constructor, it mounts without an instance. This breaks at least one invariant: during deletion, we assume that every mounted component has an instance, and check the instance for the existence of `componentWillUnmount`. Rather than add a redundant check to the deletion of every class component, components that suspend inside their constructor and outside of strict mode are turned into empty functional components before they are mounted. This is a bit weird, but it's an edge case, and the empty component will be synchronously unmounted regardless. --- .../src/ReactFiberUnwindWork.js | 17 ++++++++++ .../__tests__/ReactSuspense-test.internal.js | 32 +++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.js b/packages/react-reconciler/src/ReactFiberUnwindWork.js index 192d083c17f8..ca63ac421016 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.js @@ -71,6 +71,10 @@ import { import {findEarliestOutstandingPriorityLevel} from './ReactFiberPendingPriority'; import {reconcileChildrenAtExpirationTime} from './ReactFiberBeginWork'; +function NoopComponent() { + return null; +} + function createRootErrorUpdate( fiber: Fiber, errorInfo: CapturedValue, @@ -246,6 +250,19 @@ function throwException( sourceFiber.tag = FunctionalComponent; } + if ( + sourceFiber.tag === ClassComponent && + workInProgress.stateNode === null + ) { + // We're about to mount a class component that doesn't have an + // instance. Turn this into a dummy functional component instead, + // to prevent type errors. This is a bit weird but it's an edge + // case and we're about to synchronously delete this + // component, anyway. + sourceFiber.tag = FunctionalComponent; + sourceFiber.type = NoopComponent; + } + // Exit without suspending. return; } diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 12b4ce001b0e..9146ae8e63b2 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -1282,6 +1282,38 @@ describe('ReactSuspense', () => { span('C'), ]); }); + + it('suspends inside constructor', async () => { + class AsyncTextInConstructor extends React.Component { + constructor(props) { + super(props); + const text = props.text; + try { + TextResource.read(cache, [props.text, props.ms]); + this.state = {text}; + } catch (promise) { + if (typeof promise.then === 'function') { + ReactNoop.yield(`Suspend! [${text}]`); + } else { + ReactNoop.yield(`Error! [${text}]`); + } + throw promise; + } + } + render() { + ReactNoop.yield(this.state.text); + return ; + } + } + + ReactNoop.renderLegacySyncRoot( + }> + + , + ); + + expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); + }); }); }); From 5714bb93e1c4aca0fba31981e67dac605fad99fb Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 13 Jul 2018 10:55:03 -0700 Subject: [PATCH 2/2] Do not fire lifecycles of a suspended component In non-strict mode, suspended components commit, but their lifecycles should not fire. --- .../src/ReactFiberUnwindWork.js | 26 +++--- .../__tests__/ReactSuspense-test.internal.js | 84 +++++++++++++++++++ packages/shared/ReactTypeOfSideEffect.js | 3 + 3 files changed, 102 insertions(+), 11 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.js b/packages/react-reconciler/src/ReactFiberUnwindWork.js index ca63ac421016..432c30d4423d 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.js @@ -30,6 +30,7 @@ import { NoEffect, ShouldCapture, Update as UpdateEffect, + LifecycleEffectMask, } from 'shared/ReactTypeOfSideEffect'; import { enableGetDerivedStateFromCatch, @@ -250,17 +251,20 @@ function throwException( sourceFiber.tag = FunctionalComponent; } - if ( - sourceFiber.tag === ClassComponent && - workInProgress.stateNode === null - ) { - // We're about to mount a class component that doesn't have an - // instance. Turn this into a dummy functional component instead, - // to prevent type errors. This is a bit weird but it's an edge - // case and we're about to synchronously delete this - // component, anyway. - sourceFiber.tag = FunctionalComponent; - sourceFiber.type = NoopComponent; + if (sourceFiber.tag === ClassComponent) { + // We're going to commit this fiber even though it didn't + // complete. But we shouldn't call any lifecycle methods or + // callbacks. Remove all lifecycle effect tags. + sourceFiber.effectTag &= ~LifecycleEffectMask; + if (sourceFiber.alternate === null) { + // We're about to mount a class component that doesn't have an + // instance. Turn this into a dummy functional component instead, + // to prevent type errors. This is a bit weird but it's an edge + // case and we're about to synchronously delete this + // component, anyway. + sourceFiber.tag = FunctionalComponent; + sourceFiber.type = NoopComponent; + } } // Exit without suspending. diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 9146ae8e63b2..14678d160ea9 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -1315,6 +1315,90 @@ describe('ReactSuspense', () => { expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); }); }); + + it('does not call lifecycles of a suspended component', async () => { + class TextWithLifecycle extends React.Component { + componentDidMount() { + ReactNoop.yield(`Mount [${this.props.text}]`); + } + componentDidUpdate() { + ReactNoop.yield(`Update [${this.props.text}]`); + } + componentWillUnmount() { + ReactNoop.yield(`Unmount [${this.props.text}]`); + } + render() { + return ; + } + } + + class AsyncTextWithLifecycle extends React.Component { + componentDidMount() { + ReactNoop.yield(`Mount [${this.props.text}]`); + } + componentDidUpdate() { + ReactNoop.yield(`Update [${this.props.text}]`); + } + componentWillUnmount() { + ReactNoop.yield(`Unmount [${this.props.text}]`); + } + render() { + const text = this.props.text; + const ms = this.props.ms; + try { + TextResource.read(cache, [text, ms]); + ReactNoop.yield(text); + return ; + } catch (promise) { + if (typeof promise.then === 'function') { + ReactNoop.yield(`Suspend! [${text}]`); + } else { + ReactNoop.yield(`Error! [${text}]`); + } + throw promise; + } + } + } + + function App() { + return ( + }> + + + + + ); + } + + ReactNoop.renderLegacySyncRoot(, () => + ReactNoop.yield('Commit root'), + ); + expect(ReactNoop.clearYields()).toEqual([ + 'A', + 'Suspend! [B]', + 'C', + + 'Mount [A]', + // B's lifecycle should not fire because it suspended + // 'Mount [B]', + 'Mount [C]', + 'Commit root', + + // In a subsequent commit, render a placeholder + 'Loading...', + + // A, B, and C are unmounted, but we skip calling B's componentWillUnmount + 'Unmount [A]', + 'Unmount [C]', + + // Force delete all the existing children when switching to the + // placeholder. This should be a mount, not an update. + 'Mount [Loading...]', + ]); + expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); + }); }); // TODO: diff --git a/packages/shared/ReactTypeOfSideEffect.js b/packages/shared/ReactTypeOfSideEffect.js index 27d6aa6090e4..2a5732c385c6 100644 --- a/packages/shared/ReactTypeOfSideEffect.js +++ b/packages/shared/ReactTypeOfSideEffect.js @@ -24,6 +24,9 @@ export const DidCapture = /* */ 0b00001000000; export const Ref = /* */ 0b00010000000; export const Snapshot = /* */ 0b00100000000; +// Update & Callback & Ref & Snapshot +export const LifecycleEffectMask = /* */ 0b00110100100; + // Union of all host effects export const HostEffectMask = /* */ 0b00111111111;