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);