From c4cc734b32f1c6c02a052ebdd19be550eb7afc1d Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 23 Oct 2018 11:25:36 -0700 Subject: [PATCH 1/2] [Synchronous Suspense] Suspending a class outside concurrent mode When a class component suspends during mount outside concurrent mode, change the tag so it's not mistaken for a completed component. For example, we should not call componentWillUnmount if it is deleted. --- .../src/ReactFiberBeginWork.js | 72 ++++++++++++++++++- .../src/ReactFiberCommitWork.js | 15 +++- .../src/ReactFiberCompleteWork.js | 10 +++ .../src/ReactFiberUnwindWork.js | 11 +-- .../__tests__/ReactSuspense-test.internal.js | 39 ++++++++++ .../src/ReactTestRenderer.js | 2 + packages/shared/ReactWorkTags.js | 1 + 7 files changed, 144 insertions(+), 6 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 21c8370d7c2..31560bc6aee 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -33,6 +33,7 @@ import { MemoComponent, SimpleMemoComponent, LazyComponent, + IncompleteComponent, } from 'shared/ReactWorkTags'; import { NoEffect, @@ -762,7 +763,7 @@ function mountLazyComponent( renderExpirationTime, ) { if (_current !== null) { - // An indeterminate component only mounts if it suspended inside a non- + // An lazy component 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. @@ -840,6 +841,68 @@ function mountLazyComponent( return child; } +function mountIncompleteComponent( + _current, + workInProgress, + renderExpirationTime, +) { + if (_current !== null) { + // An incomplete component 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; + } + + // Promote the fiber to a class and try rendering again. + workInProgress.tag = ClassComponent; + const Component = workInProgress.type; + const unresolvedProps = workInProgress.pendingProps; + const resolvedProps = + workInProgress.elementType === Component + ? unresolvedProps + : resolveDefaultProps(Component, unresolvedProps); + + // The rest of this function is a fork of `updateClassComponent` + + // Push context providers early to prevent context stack mismatches. + // During mounting we don't know the child context yet as the instance doesn't exist. + // We will invalidate the child context in finishClassComponent() right after rendering. + let hasContext; + if (isLegacyContextProvider(Component)) { + hasContext = true; + pushLegacyContextProvider(workInProgress); + } else { + hasContext = false; + } + prepareToReadContext(workInProgress, renderExpirationTime); + + constructClassInstance( + workInProgress, + Component, + resolvedProps, + renderExpirationTime, + ); + mountClassInstance( + workInProgress, + Component, + resolvedProps, + renderExpirationTime, + ); + + return finishClassComponent( + null, + workInProgress, + Component, + true, + hasContext, + renderExpirationTime, + ); +} + function mountIndeterminateComponent( _current, workInProgress, @@ -1654,6 +1717,13 @@ function beginWork( renderExpirationTime, ); } + case IncompleteComponent: { + return mountIncompleteComponent( + current, + workInProgress, + renderExpirationTime, + ); + } default: invariant( false, diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index b79c36f6481..6cf1913672f 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -32,6 +32,7 @@ import { HostPortal, Profiler, SuspenseComponent, + IncompleteComponent, } from 'shared/ReactWorkTags'; import { invokeGuardedCallback, @@ -221,6 +222,7 @@ function commitBeforeMutationLifeCycles( case HostComponent: case HostText: case HostPortal: + case IncompleteComponent: // Nothing to do for these component types return; default: { @@ -392,6 +394,8 @@ function commitLifeCycles( } return; } + case IncompleteComponent: + break; default: { invariant( false, @@ -495,7 +499,13 @@ function commitUnmount(current: Fiber): void { case ClassComponent: { safelyDetachRef(current); const instance = current.stateNode; - if (typeof instance.componentWillUnmount === 'function') { + if ( + // Typically, a component that mounted will have an instance. However, + // outside of concurrent mode, a suspended component may commit without + // an instance, so we need to check whether it exists. + instance !== null && + typeof instance.componentWillUnmount === 'function' + ) { safelyCallComponentWillUnmount(current, instance); } return; @@ -924,6 +934,9 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { case SuspenseComponent: { return; } + case IncompleteComponent: { + return; + } default: { invariant( false, diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index c0657661a12..a84d868f5a7 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -37,6 +37,7 @@ import { MemoComponent, SimpleMemoComponent, LazyComponent, + IncompleteComponent, } from 'shared/ReactWorkTags'; import {Placement, Ref, Update} from 'shared/ReactSideEffectTags'; import invariant from 'shared/invariant'; @@ -717,6 +718,15 @@ function completeWork( break; case MemoComponent: break; + case IncompleteComponent: { + // Same as class component case. I put it down here so that the tags are + // sequential to ensure this switch is compiled to a jump table. + const Component = workInProgress.type; + if (isLegacyContextProvider(Component)) { + popLegacyContext(workInProgress); + } + break; + } default: invariant( false, diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.js b/packages/react-reconciler/src/ReactFiberUnwindWork.js index 44f5a20ee84..929037f5396 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.js @@ -25,6 +25,7 @@ import { HostPortal, ContextProvider, SuspenseComponent, + IncompleteComponent, } from 'shared/ReactWorkTags'; import { DidCapture, @@ -252,10 +253,12 @@ function throwException( // But we shouldn't call any lifecycle methods or callbacks. Remove // all lifecycle effect tags. sourceFiber.effectTag &= ~LifecycleEffectMask; - if (sourceFiber.alternate === null) { - // 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; + const current = sourceFiber.alternate; + if (current === null) { + // This is a new mount. Change the tag so it's not mistaken for a + // completed component. For example, we should not call + // componentWillUnmount if it is deleted. + sourceFiber.tag = IncompleteComponent; } } diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 8362ee523d2..305b7408745 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -492,6 +492,45 @@ describe('ReactSuspense', () => { expect(root).toMatchRenderedOutput('Stateful: 2B'); }); + it('suspends in a class that has componentWillUnmount and is then deleted', () => { + class AsyncTextWithUnmount extends React.Component { + componentWillUnmount() { + ReactTestRenderer.unstable_yield('will unmount'); + } + render() { + const text = this.props.text; + 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({text}) { + return ( + }> + + + ); + } + + const root = ReactTestRenderer.create(); + expect(ReactTestRenderer).toHaveYielded(['Suspend! [A]', 'Loading...']); + root.update(); + // Should not fire componentWillUnmount + expect(ReactTestRenderer).toHaveYielded(['B']); + expect(root).toMatchRenderedOutput('B'); + }); + it('retries when an update is scheduled on a timed out tree', () => { let instance; class Stateful extends React.Component { diff --git a/packages/react-test-renderer/src/ReactTestRenderer.js b/packages/react-test-renderer/src/ReactTestRenderer.js index 6a8996c609c..4e1efe2ab03 100644 --- a/packages/react-test-renderer/src/ReactTestRenderer.js +++ b/packages/react-test-renderer/src/ReactTestRenderer.js @@ -29,6 +29,7 @@ import { Profiler, MemoComponent, SimpleMemoComponent, + IncompleteComponent, } from 'shared/ReactWorkTags'; import invariant from 'shared/invariant'; import ReactVersion from 'shared/ReactVersion'; @@ -193,6 +194,7 @@ function toTree(node: ?Fiber) { case Profiler: case ForwardRef: case MemoComponent: + case IncompleteComponent: return childrenToTree(node.child); default: invariant( diff --git a/packages/shared/ReactWorkTags.js b/packages/shared/ReactWorkTags.js index bb7bee12d9d..d39d4d87cd4 100644 --- a/packages/shared/ReactWorkTags.js +++ b/packages/shared/ReactWorkTags.js @@ -45,3 +45,4 @@ export const SuspenseComponent = 13; export const MemoComponent = 14; export const SimpleMemoComponent = 15; export const LazyComponent = 16; +export const IncompleteComponent = 17; From e1300effca35446b0fd6b7a3407274bcbf0890db Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 23 Oct 2018 11:34:12 -0700 Subject: [PATCH 2/2] PR nits --- .../src/ReactFiberBeginWork.js | 28 +++++++++++-------- .../src/ReactFiberCommitWork.js | 8 +++--- .../src/ReactFiberCompleteWork.js | 4 +-- .../src/ReactFiberUnwindWork.js | 4 +-- .../src/ReactTestRenderer.js | 4 +-- packages/shared/ReactWorkTags.js | 2 +- 6 files changed, 27 insertions(+), 23 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 31560bc6aee..5358a693100 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -33,7 +33,7 @@ import { MemoComponent, SimpleMemoComponent, LazyComponent, - IncompleteComponent, + IncompleteClassComponent, } from 'shared/ReactWorkTags'; import { NoEffect, @@ -841,9 +841,11 @@ function mountLazyComponent( return child; } -function mountIncompleteComponent( +function mountIncompleteClassComponent( _current, workInProgress, + Component, + nextProps, renderExpirationTime, ) { if (_current !== null) { @@ -859,12 +861,6 @@ function mountIncompleteComponent( // Promote the fiber to a class and try rendering again. workInProgress.tag = ClassComponent; - const Component = workInProgress.type; - const unresolvedProps = workInProgress.pendingProps; - const resolvedProps = - workInProgress.elementType === Component - ? unresolvedProps - : resolveDefaultProps(Component, unresolvedProps); // The rest of this function is a fork of `updateClassComponent` @@ -883,13 +879,13 @@ function mountIncompleteComponent( constructClassInstance( workInProgress, Component, - resolvedProps, + nextProps, renderExpirationTime, ); mountClassInstance( workInProgress, Component, - resolvedProps, + nextProps, renderExpirationTime, ); @@ -1717,10 +1713,18 @@ function beginWork( renderExpirationTime, ); } - case IncompleteComponent: { - return mountIncompleteComponent( + case IncompleteClassComponent: { + const Component = workInProgress.type; + const unresolvedProps = workInProgress.pendingProps; + const resolvedProps = + workInProgress.elementType === Component + ? unresolvedProps + : resolveDefaultProps(Component, unresolvedProps); + return mountIncompleteClassComponent( current, workInProgress, + Component, + resolvedProps, renderExpirationTime, ); } diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 6cf1913672f..1efd1fcf8df 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -32,7 +32,7 @@ import { HostPortal, Profiler, SuspenseComponent, - IncompleteComponent, + IncompleteClassComponent, } from 'shared/ReactWorkTags'; import { invokeGuardedCallback, @@ -222,7 +222,7 @@ function commitBeforeMutationLifeCycles( case HostComponent: case HostText: case HostPortal: - case IncompleteComponent: + case IncompleteClassComponent: // Nothing to do for these component types return; default: { @@ -394,7 +394,7 @@ function commitLifeCycles( } return; } - case IncompleteComponent: + case IncompleteClassComponent: break; default: { invariant( @@ -934,7 +934,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { case SuspenseComponent: { return; } - case IncompleteComponent: { + case IncompleteClassComponent: { return; } default: { diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index a84d868f5a7..79c36c1a774 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -37,7 +37,7 @@ import { MemoComponent, SimpleMemoComponent, LazyComponent, - IncompleteComponent, + IncompleteClassComponent, } from 'shared/ReactWorkTags'; import {Placement, Ref, Update} from 'shared/ReactSideEffectTags'; import invariant from 'shared/invariant'; @@ -718,7 +718,7 @@ function completeWork( break; case MemoComponent: break; - case IncompleteComponent: { + case IncompleteClassComponent: { // Same as class component case. I put it down here so that the tags are // sequential to ensure this switch is compiled to a jump table. const Component = workInProgress.type; diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.js b/packages/react-reconciler/src/ReactFiberUnwindWork.js index 929037f5396..40c814a37d7 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.js @@ -25,7 +25,7 @@ import { HostPortal, ContextProvider, SuspenseComponent, - IncompleteComponent, + IncompleteClassComponent, } from 'shared/ReactWorkTags'; import { DidCapture, @@ -258,7 +258,7 @@ function throwException( // This is a new mount. Change the tag so it's not mistaken for a // completed component. For example, we should not call // componentWillUnmount if it is deleted. - sourceFiber.tag = IncompleteComponent; + sourceFiber.tag = IncompleteClassComponent; } } diff --git a/packages/react-test-renderer/src/ReactTestRenderer.js b/packages/react-test-renderer/src/ReactTestRenderer.js index 4e1efe2ab03..21f007525f4 100644 --- a/packages/react-test-renderer/src/ReactTestRenderer.js +++ b/packages/react-test-renderer/src/ReactTestRenderer.js @@ -29,7 +29,7 @@ import { Profiler, MemoComponent, SimpleMemoComponent, - IncompleteComponent, + IncompleteClassComponent, } from 'shared/ReactWorkTags'; import invariant from 'shared/invariant'; import ReactVersion from 'shared/ReactVersion'; @@ -194,7 +194,7 @@ function toTree(node: ?Fiber) { case Profiler: case ForwardRef: case MemoComponent: - case IncompleteComponent: + case IncompleteClassComponent: return childrenToTree(node.child); default: invariant( diff --git a/packages/shared/ReactWorkTags.js b/packages/shared/ReactWorkTags.js index d39d4d87cd4..aed81502179 100644 --- a/packages/shared/ReactWorkTags.js +++ b/packages/shared/ReactWorkTags.js @@ -45,4 +45,4 @@ export const SuspenseComponent = 13; export const MemoComponent = 14; export const SimpleMemoComponent = 15; export const LazyComponent = 16; -export const IncompleteComponent = 17; +export const IncompleteClassComponent = 17;