From 43ffae2d1750ee32409e140e41ea562ff46d2516 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 13 Jul 2018 11:24:03 -0700 Subject: [PATCH] Suspending inside a constructor outside of strict mode (#13200) * 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. * Do not fire lifecycles of a suspended component In non-strict mode, suspended components commit, but their lifecycles should not fire. --- .../src/ReactFiberUnwindWork.js | 21 ++++ .../__tests__/ReactSuspense-test.internal.js | 116 ++++++++++++++++++ packages/shared/ReactTypeOfSideEffect.js | 3 + 3 files changed, 140 insertions(+) diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.js b/packages/react-reconciler/src/ReactFiberUnwindWork.js index 192d083c17..432c30d442 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, @@ -71,6 +72,10 @@ import { import {findEarliestOutstandingPriorityLevel} from './ReactFiberPendingPriority'; import {reconcileChildrenAtExpirationTime} from './ReactFiberBeginWork'; +function NoopComponent() { + return null; +} + function createRootErrorUpdate( fiber: Fiber, errorInfo: CapturedValue, @@ -246,6 +251,22 @@ function throwException( sourceFiber.tag = FunctionalComponent; } + 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. return; } diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 12b4ce001b..14678d160e 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -1282,6 +1282,122 @@ 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...')]); + }); + }); + + 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...')]); }); }); diff --git a/packages/shared/ReactTypeOfSideEffect.js b/packages/shared/ReactTypeOfSideEffect.js index 27d6aa6090..2a5732c385 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;