From 41aa345d2bde261c2fb4a4ef89e379640c88be67 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 3 Apr 2019 13:21:27 -0700 Subject: [PATCH 1/2] Fix a crash in Suspense with findDOMNode --- .../ReactDOMSuspensePlaceholder-test.js | 44 +++++++++++++++++++ .../src/ReactFiberTreeReflection.js | 25 ++++++++++- 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js b/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js index de43fd44274c3..45fec880410c4 100644 --- a/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js @@ -233,4 +233,48 @@ describe('ReactDOMSuspensePlaceholder', () => { await Lazy; expect(log).toEqual(['cDU first', 'cDU second']); }); + + // Regression test for https://github.com/facebook/react/issues/14188 + it('can call findDOMNode() in a suspended component commit phase (#2)', () => { + let suspendOnce = Promise.resolve(); + function Suspend() { + if (suspendOnce) { + let promise = suspendOnce; + suspendOnce = null; + throw promise; + } + return null; + } + + const log = []; + class Child extends React.Component { + componentDidMount() { + log.push('cDM'); + ReactDOM.findDOMNode(this); + } + + componentDidUpdate() { + log.push('cDU'); + ReactDOM.findDOMNode(this); + } + + render() { + return null; + } + } + + function App() { + return ( + + + + + ); + } + + ReactDOM.render(, container); + expect(log).toEqual(['cDM']); + ReactDOM.render(, container); + expect(log).toEqual(['cDM', 'cDU']); + }); }); diff --git a/packages/react-reconciler/src/ReactFiberTreeReflection.js b/packages/react-reconciler/src/ReactFiberTreeReflection.js index f2c3e37b699ec..150e8200aed40 100644 --- a/packages/react-reconciler/src/ReactFiberTreeReflection.js +++ b/packages/react-reconciler/src/ReactFiberTreeReflection.js @@ -21,6 +21,8 @@ import { HostRoot, HostPortal, HostText, + Fragment, + SuspenseComponent, } from 'shared/ReactWorkTags'; import {NoEffect, Placement} from 'shared/ReactSideEffectTags'; @@ -119,8 +121,27 @@ export function findCurrentFiberUsingSlowPath(fiber: Fiber): Fiber | null { let parentA = a.return; let parentB = parentA ? parentA.alternate : null; if (!parentA || !parentB) { - // We're at the root. - break; + // We're either at the root, or we're in a special Fragment + // with no alternate, which is how Suspense (un)hiding works. + let maybeSuspenseFragment = parentA || parentB; + if (maybeSuspenseFragment && maybeSuspenseFragment.tag === Fragment) { + const maybeSuspense = maybeSuspenseFragment.return; + if ( + maybeSuspense && + maybeSuspense.tag === SuspenseComponent && + // If state isn't null, it timed out and we have two Fragment children. + maybeSuspense.memoizedState !== null + ) { + parentA = maybeSuspense; + parentB = maybeSuspense; + a = maybeSuspenseFragment; + b = maybeSuspenseFragment; + } else { + break; + } + } else { + break; + } } // If both copies of the parent fiber point to the same child, we can From 43b1f74c88d986c88623412be7b1d65a6e271779 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 3 Apr 2019 15:07:09 -0700 Subject: [PATCH 2/2] Alternate fix for #14198 This doesn't rely on checking the tag. When the alternate of a parent is missing, it assumes it's a fragment indirection and moves onto the next parent fiber. --- .../src/ReactFiberTreeReflection.js | 44 ++++++++----------- 1 file changed, 18 insertions(+), 26 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberTreeReflection.js b/packages/react-reconciler/src/ReactFiberTreeReflection.js index 150e8200aed40..734dead68cf6f 100644 --- a/packages/react-reconciler/src/ReactFiberTreeReflection.js +++ b/packages/react-reconciler/src/ReactFiberTreeReflection.js @@ -21,8 +21,6 @@ import { HostRoot, HostPortal, HostText, - Fragment, - SuspenseComponent, } from 'shared/ReactWorkTags'; import {NoEffect, Placement} from 'shared/ReactSideEffectTags'; @@ -115,33 +113,27 @@ export function findCurrentFiberUsingSlowPath(fiber: Fiber): Fiber | null { // If we have two possible branches, we'll walk backwards up to the root // to see what path the root points to. On the way we may hit one of the // special cases and we'll deal with them. - let a = fiber; - let b = alternate; + let a: Fiber = fiber; + let b: Fiber = alternate; while (true) { let parentA = a.return; - let parentB = parentA ? parentA.alternate : null; - if (!parentA || !parentB) { - // We're either at the root, or we're in a special Fragment - // with no alternate, which is how Suspense (un)hiding works. - let maybeSuspenseFragment = parentA || parentB; - if (maybeSuspenseFragment && maybeSuspenseFragment.tag === Fragment) { - const maybeSuspense = maybeSuspenseFragment.return; - if ( - maybeSuspense && - maybeSuspense.tag === SuspenseComponent && - // If state isn't null, it timed out and we have two Fragment children. - maybeSuspense.memoizedState !== null - ) { - parentA = maybeSuspense; - parentB = maybeSuspense; - a = maybeSuspenseFragment; - b = maybeSuspenseFragment; - } else { - break; - } - } else { - break; + if (parentA === null) { + // We're at the root. + break; + } + let parentB = parentA.alternate; + if (parentB === null) { + // There is no alternate. This is an unusual case. Currently, it only + // happens when a Suspense component is hidden. An extra fragment fiber + // is inserted in between the Suspense fiber and its children. Skip + // over this extra fragment fiber and proceed to the next parent. + const nextParent = parentA.return; + if (nextParent !== null) { + a = b = nextParent; + continue; } + // If there's no parent, we're at the root. + break; } // If both copies of the parent fiber point to the same child, we can