From 63cb523e264257f45f3ac7603348e2a0a8cc57c9 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 19 Jun 2020 10:11:56 -0700 Subject: [PATCH] Bugfix: Legacy Mode + DevTools "force fallback" DevTools has a feature to force a Suspense boundary to show a fallback. This feature causes us to skip the first render pass (where we render the primary children) and go straight to rendering the fallback. There's a Legacy Mode-only codepath that failed to take this scenario into account, instead assuming that whenever a fallback is being rendered, it was preceded by an attempt to render the primary children. SuspenseList can also cause us to skip the first pass, but the relevant branch is Legacy Mode-only, and SuspenseList is not supported in Legacy Mode. Fixes a test that I had temporarily disabled when upstreaming the Lanes implementation in #19108. --- .../__snapshots__/store-test.js.snap | 76 +++++++++++ .../src/__tests__/store-test.js | 122 ++++++++---------- .../src/ReactFiberBeginWork.new.js | 13 +- .../src/ReactFiberBeginWork.old.js | 13 +- 4 files changed, 155 insertions(+), 69 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/__snapshots__/store-test.js.snap b/packages/react-devtools-shared/src/__tests__/__snapshots__/store-test.js.snap index 7264c7704d7e2..46e5f86aedc07 100644 --- a/packages/react-devtools-shared/src/__tests__/__snapshots__/store-test.js.snap +++ b/packages/react-devtools-shared/src/__tests__/__snapshots__/store-test.js.snap @@ -255,6 +255,82 @@ exports[`Store collapseNodesByDefault:false should support nested Suspense nodes `; +exports[`Store collapseNodesByDefault:false should support nested Suspense nodes: 8: first and third child are suspended 1`] = ` +[root] + ▾ + + ▾ + + ▾ + + ▾ + + ▾ + + +`; + +exports[`Store collapseNodesByDefault:false should support nested Suspense nodes: 9: parent is suspended 1`] = ` +[root] + ▾ + + ▾ + +`; + +exports[`Store collapseNodesByDefault:false should support nested Suspense nodes: 10: parent is suspended 1`] = ` +[root] + ▾ + + ▾ + +`; + +exports[`Store collapseNodesByDefault:false should support nested Suspense nodes: 11: all children are suspended 1`] = ` +[root] + ▾ + + ▾ + + ▾ + + ▾ + + ▾ + + +`; + +exports[`Store collapseNodesByDefault:false should support nested Suspense nodes: 12: all children are suspended 1`] = ` +[root] + ▾ + + ▾ + + ▾ + + ▾ + + ▾ + + +`; + +exports[`Store collapseNodesByDefault:false should support nested Suspense nodes: 13: third child is suspended 1`] = ` +[root] + ▾ + + ▾ + + ▾ + + ▾ + + ▾ + + +`; + exports[`Store collapseNodesByDefault:false should support reordering of children: 1: mount 1`] = ` [root] ▾ diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js index 466c075928f81..d2e95073e8a6d 100644 --- a/packages/react-devtools-shared/src/__tests__/store-test.js +++ b/packages/react-devtools-shared/src/__tests__/store-test.js @@ -285,73 +285,61 @@ describe('Store', () => { ); expect(store).toMatchSnapshot('7: only third child is suspended'); - // FIXME: The rest of the test fails. This was introduced as part of - // the Lanes refactor. I'm fairly certain it's related to the layout of - // the Suspense fiber: we no longer conditionally wrap the primary - // children. They are always wrapped in an extra fiber. - // - // This landed in the new fork without triggering the test run - // because we don't run the DevTools tests against both forks. I only - // discovered the failure once I upstreamed the changes. - // - // Since this has been running in www for weeks without major issues, I'll - // defer fixing this to a follow up. - // - // const rendererID = getRendererID(); - // act(() => - // agent.overrideSuspense({ - // id: store.getElementIDAtIndex(4), - // rendererID, - // forceFallback: true, - // }), - // ); - // expect(store).toMatchSnapshot('8: first and third child are suspended'); - // act(() => - // agent.overrideSuspense({ - // id: store.getElementIDAtIndex(2), - // rendererID, - // forceFallback: true, - // }), - // ); - // expect(store).toMatchSnapshot('9: parent is suspended'); - // act(() => - // ReactDOM.render( - // , - // container, - // ), - // ); - // expect(store).toMatchSnapshot('10: parent is suspended'); - // act(() => - // agent.overrideSuspense({ - // id: store.getElementIDAtIndex(2), - // rendererID, - // forceFallback: false, - // }), - // ); - // expect(store).toMatchSnapshot('11: all children are suspended'); - // act(() => - // agent.overrideSuspense({ - // id: store.getElementIDAtIndex(4), - // rendererID, - // forceFallback: false, - // }), - // ); - // expect(store).toMatchSnapshot('12: all children are suspended'); - // act(() => - // ReactDOM.render( - // , - // container, - // ), - // ); - // expect(store).toMatchSnapshot('13: third child is suspended'); + const rendererID = getRendererID(); + act(() => + agent.overrideSuspense({ + id: store.getElementIDAtIndex(4), + rendererID, + forceFallback: true, + }), + ); + expect(store).toMatchSnapshot('8: first and third child are suspended'); + act(() => + agent.overrideSuspense({ + id: store.getElementIDAtIndex(2), + rendererID, + forceFallback: true, + }), + ); + expect(store).toMatchSnapshot('9: parent is suspended'); + act(() => + ReactDOM.render( + , + container, + ), + ); + expect(store).toMatchSnapshot('10: parent is suspended'); + act(() => + agent.overrideSuspense({ + id: store.getElementIDAtIndex(2), + rendererID, + forceFallback: false, + }), + ); + expect(store).toMatchSnapshot('11: all children are suspended'); + act(() => + agent.overrideSuspense({ + id: store.getElementIDAtIndex(4), + rendererID, + forceFallback: false, + }), + ); + expect(store).toMatchSnapshot('12: all children are suspended'); + act(() => + ReactDOM.render( + , + container, + ), + ); + expect(store).toMatchSnapshot('13: third child is suspended'); }); it('should display a partially rendered SuspenseList', () => { diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js index cee8ae1f0fefe..1dd44dd32f284 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.new.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.new.js @@ -2082,7 +2082,18 @@ function updateSuspenseFallbackChildren( }; let primaryChildFragment; - if ((mode & BlockingMode) === NoMode) { + if ( + // In legacy mode, we commit the primary tree as if it successfully + // completed, even though it's in an inconsistent state. + (mode & BlockingMode) === NoMode && + // Make sure we're on the second pass, i.e. the primary child fragment was + // already cloned. In legacy mode, the only case where this isn't true is + // when DevTools forces us to display a fallback; we skip the first render + // pass entirely and go straight to rendering the fallback. (In Concurrent + // Mode, SuspenseList can also trigger this scenario, but this is a legacy- + // only codepath.) + workInProgress.child !== currentPrimaryChildFragment + ) { // In legacy mode, we commit the primary tree as if it successfully // completed, even though it's in an inconsistent state. const progressedPrimaryFragment: Fiber = (workInProgress.child: any); diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.old.js b/packages/react-reconciler/src/ReactFiberBeginWork.old.js index 97541e23460d7..92c9e87d86067 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.old.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.old.js @@ -2082,7 +2082,18 @@ function updateSuspenseFallbackChildren( }; let primaryChildFragment; - if ((mode & BlockingMode) === NoMode) { + if ( + // In legacy mode, we commit the primary tree as if it successfully + // completed, even though it's in an inconsistent state. + (mode & BlockingMode) === NoMode && + // Make sure we're on the second pass, i.e. the primary child fragment was + // already cloned. In legacy mode, the only case where this isn't true is + // when DevTools forces us to display a fallback; we skip the first render + // pass entirely and go straight to rendering the fallback. (In Concurrent + // Mode, SuspenseList can also trigger this scenario, but this is a legacy- + // only codepath.) + workInProgress.child !== currentPrimaryChildFragment + ) { // In legacy mode, we commit the primary tree as if it successfully // completed, even though it's in an inconsistent state. const progressedPrimaryFragment: Fiber = (workInProgress.child: any);