From 0fc04467987c187d557744f27c8ba90ea0a06cad Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 19 Oct 2018 18:41:47 -0700 Subject: [PATCH] Class component can suspend without losing state outside concurrent mode (#13899) Outside of concurrent mode, schedules a force update on a suspended class component to force it to prevent it from bailing out and reusing the current fiber, which we know to be inconsistent. --- .../src/ReactFiberBeginWork.js | 57 +++++---- .../src/ReactFiberScheduler.js | 19 ++- .../src/ReactFiberUnwindWork.js | 15 +-- .../__tests__/ReactSuspense-test.internal.js | 115 ++++++++++++++++++ 4 files changed, 169 insertions(+), 37 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 0cf92b4b27..1e0b6e1e34 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -401,32 +401,41 @@ function updateClassComponent( } prepareToReadContext(workInProgress, renderExpirationTime); + const instance = workInProgress.stateNode; let shouldUpdate; - if (current === null) { - if (workInProgress.stateNode === null) { - // In the initial pass we might need to construct the instance. - constructClassInstance( - workInProgress, - Component, - nextProps, - renderExpirationTime, - ); - mountClassInstance( - workInProgress, - Component, - nextProps, - renderExpirationTime, - ); - shouldUpdate = true; - } else { - // In a resume, we'll already have an instance we can reuse. - shouldUpdate = resumeMountClassInstance( - workInProgress, - Component, - nextProps, - renderExpirationTime, - ); + if (instance === null) { + if (current !== null) { + // An class component without an instance only mounts if it suspended + // inside a non- concurrent tree, in an inconsistent state. We want to + // tree it like a new mount, even though an empty version of it already + // committed. Disconnect the alternate pointers. + current.alternate = null; + workInProgress.alternate = null; + // Since this is conceptually a new fiber, schedule a Placement effect + workInProgress.effectTag |= Placement; } + // In the initial pass we might need to construct the instance. + constructClassInstance( + workInProgress, + Component, + nextProps, + renderExpirationTime, + ); + mountClassInstance( + workInProgress, + Component, + nextProps, + renderExpirationTime, + ); + shouldUpdate = true; + } else if (current === null) { + // In a resume, we'll already have an instance we can reuse. + shouldUpdate = resumeMountClassInstance( + workInProgress, + Component, + nextProps, + renderExpirationTime, + ); } else { shouldUpdate = updateClassInstance( current, diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index c3a0e983eb..480909ac62 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -111,7 +111,12 @@ import { computeInteractiveExpiration, } from './ReactFiberExpirationTime'; import {ConcurrentMode, ProfileMode, NoContext} from './ReactTypeOfMode'; -import {enqueueUpdate, resetCurrentlyProcessingQueue} from './ReactUpdateQueue'; +import { + enqueueUpdate, + resetCurrentlyProcessingQueue, + ForceUpdate, + createUpdate, +} from './ReactUpdateQueue'; import {createCapturedValue} from './ReactCapturedValue'; import { isContextProvider as isLegacyContextProvider, @@ -1604,6 +1609,18 @@ function retrySuspendedRoot( // fiber, too, since it already committed in an inconsistent state and // therefore does not have any pending work. scheduleWorkToRoot(sourceFiber, retryTime); + const sourceTag = sourceFiber.tag; + if ( + (sourceTag === ClassComponent || sourceFiber === ClassComponentLazy) && + sourceFiber.stateNode !== null + ) { + // When we try rendering again, we should not reuse the current fiber, + // since it's known to be in an inconsistent state. Use a force updte to + // prevent a bail out. + const update = createUpdate(retryTime); + update.tag = ForceUpdate; + enqueueUpdate(sourceFiber, update); + } } const rootExpirationTime = root.expirationTime; diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.js b/packages/react-reconciler/src/ReactFiberUnwindWork.js index b2c8397a76..a39d3d6a9e 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.js @@ -18,7 +18,6 @@ import {unstable_wrap as Schedule_tracing_wrap} from 'scheduler/tracing'; import getComponentName from 'shared/getComponentName'; import warningWithoutStack from 'shared/warningWithoutStack'; import { - FunctionComponent, ClassComponent, ClassComponentLazy, HostRoot, @@ -71,10 +70,6 @@ import { import {findEarliestOutstandingPriorityLevel} from './ReactFiberPendingPriority'; import {reconcileChildren} from './ReactFiberBeginWork'; -function NoopComponent() { - return null; -} - function createRootErrorUpdate( fiber: Fiber, errorInfo: CapturedValue, @@ -262,13 +257,9 @@ function throwException( // 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 function 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 = FunctionComponent; - sourceFiber.type = NoopComponent; + // Set the instance back to null. We use this as a heuristic to + // detect that the fiber mounted in an inconsistent state. + sourceFiber.stateNode = null; } } diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 0a6f6a445a..7386db830d 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -321,4 +321,119 @@ describe('ReactSuspense', () => { ); expect(ReactTestRenderer).toHaveYielded(['Suspend! [Hi]', 'Suspend! [Hi]']); }); + + describe('outside concurrent mode', () => { + it('a mounted class component can suspend without losing state', () => { + class TextWithLifecycle extends React.Component { + componentDidMount() { + ReactTestRenderer.unstable_yield(`Mount [${this.props.text}]`); + } + componentDidUpdate() { + ReactTestRenderer.unstable_yield(`Update [${this.props.text}]`); + } + componentWillUnmount() { + ReactTestRenderer.unstable_yield(`Unmount [${this.props.text}]`); + } + render() { + return ; + } + } + + let instance; + class AsyncTextWithLifecycle extends React.Component { + state = {step: 1}; + componentDidMount() { + ReactTestRenderer.unstable_yield( + `Mount [${this.props.text}:${this.state.step}]`, + ); + } + componentDidUpdate() { + ReactTestRenderer.unstable_yield( + `Update [${this.props.text}:${this.state.step}]`, + ); + } + componentWillUnmount() { + ReactTestRenderer.unstable_yield( + `Unmount [${this.props.text}:${this.state.step}]`, + ); + } + render() { + instance = this; + const text = `${this.props.text}:${this.state.step}`; + const ms = this.props.ms; + try { + TextResource.read(cache, [text, ms]); + ReactTestRenderer.unstable_yield(text); + return text; + } catch (promise) { + if (typeof promise.then === 'function') { + ReactTestRenderer.unstable_yield(`Suspend! [${text}]`); + } else { + ReactTestRenderer.unstable_yield(`Error! [${text}]`); + } + throw promise; + } + } + } + + function App() { + return ( + }> + + + + + ); + } + + const root = ReactTestRenderer.create(); + + expect(ReactTestRenderer).toHaveYielded([ + 'A', + 'Suspend! [B:1]', + 'C', + + 'Mount [A]', + // B's lifecycle should not fire because it suspended + // 'Mount [B]', + 'Mount [C]', + + // In a subsequent commit, render a placeholder + 'Loading...', + 'Mount [Loading...]', + ]); + expect(root).toMatchRenderedOutput('Loading...'); + + jest.advanceTimersByTime(100); + expect(ReactTestRenderer).toHaveYielded([ + 'Promise resolved [B:1]', + 'B:1', + 'Unmount [Loading...]', + // Should be a mount, not an update + 'Mount [B:1]', + ]); + + expect(root).toMatchRenderedOutput('AB:1C'); + + instance.setState({step: 2}); + expect(ReactTestRenderer).toHaveYielded([ + 'Suspend! [B:2]', + 'Loading...', + 'Mount [Loading...]', + ]); + expect(root).toMatchRenderedOutput('Loading...'); + + jest.advanceTimersByTime(100); + + expect(ReactTestRenderer).toHaveYielded([ + 'Promise resolved [B:2]', + 'B:2', + 'Unmount [Loading...]', + 'Update [B:2]', + ]); + expect(root).toMatchRenderedOutput('AB:2C'); + }); + }); });