diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzSuspenseList-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzSuspenseList-test.js index 74db51d2429..0e2fff5cd45 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzSuspenseList-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzSuspenseList-test.js @@ -134,7 +134,7 @@ describe('ReactDOMFizzSuspenseList', () => { } // @gate enableSuspenseList - it('shows content independently by default', async () => { + it('shows content forwards by default', async () => { const A = createAsyncText('A'); const B = createAsyncText('B'); const C = createAsyncText('C'); @@ -157,31 +157,38 @@ describe('ReactDOMFizzSuspenseList', () => { ); } - await A.resolve(); + await C.resolve(); await serverAct(async () => { const {pipe} = ReactDOMFizzServer.renderToPipeableStream(); pipe(writable); }); - assertLog(['A', 'Suspend! [B]', 'Suspend! [C]', 'Loading B', 'Loading C']); + assertLog([ + 'Suspend! [A]', + 'Suspend! [B]', // TODO: Defer rendering the content after fallback if previous suspended, + 'C', + 'Loading A', + 'Loading B', + 'Loading C', + ]); expect(getVisibleChildren(container)).toEqual(
- A + Loading A Loading B Loading C
, ); - await serverAct(() => C.resolve()); - assertLog(['C']); + await serverAct(() => A.resolve()); + assertLog(['A']); expect(getVisibleChildren(container)).toEqual(
A Loading B - C + Loading C
, ); @@ -649,6 +656,77 @@ describe('ReactDOMFizzSuspenseList', () => { ); }); + // @gate enableSuspenseList + it('displays each items in "backwards" mount order', async () => { + const A = createAsyncText('A'); + const B = createAsyncText('B'); + const C = createAsyncText('C'); + + function Foo() { + return ( +
+ + }> + + + }> + + + }> + + + +
+ ); + } + + await A.resolve(); + + await serverAct(async () => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream(); + pipe(writable); + }); + + assertLog([ + 'Suspend! [C]', + 'Suspend! [B]', // TODO: Defer rendering the content after fallback if previous suspended, + 'A', + 'Loading C', + 'Loading B', + 'Loading A', + ]); + + expect(getVisibleChildren(container)).toEqual( +
+ Loading A + Loading B + Loading C +
, + ); + + await serverAct(() => C.resolve()); + assertLog(['C']); + + expect(getVisibleChildren(container)).toEqual( +
+ Loading A + Loading B + C +
, + ); + + await serverAct(() => B.resolve()); + assertLog(['B']); + + expect(getVisibleChildren(container)).toEqual( +
+ A + B + C +
, + ); + }); + // @gate enableSuspenseList it('displays each items in "backwards" order in legacy mode', async () => { const A = createAsyncText('A'); @@ -730,15 +808,13 @@ describe('ReactDOMFizzSuspenseList', () => { return (
- - }> - - + }> + }> + + }> diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index f0d9c2e012e..8daf7fde4b4 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -2104,7 +2104,8 @@ export function validateSuspenseListChildren( ) { if (__DEV__) { if ( - (revealOrder === 'forwards' || + (revealOrder == null || + revealOrder === 'forwards' || revealOrder === 'backwards' || revealOrder === 'unstable_legacy-backwards') && children !== undefined && diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 7251e52dd61..33fadfd5396 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -3245,25 +3245,16 @@ function validateRevealOrder(revealOrder: SuspenseListRevealOrder) { if (__DEV__) { const cacheKey = revealOrder == null ? 'null' : revealOrder; if ( + revealOrder != null && revealOrder !== 'forwards' && + revealOrder !== 'backwards' && revealOrder !== 'unstable_legacy-backwards' && revealOrder !== 'together' && revealOrder !== 'independent' && !didWarnAboutRevealOrder[cacheKey] ) { didWarnAboutRevealOrder[cacheKey] = true; - if (revealOrder == null) { - console.error( - 'The default for the prop is changing. ' + - 'To be future compatible you must explictly specify either ' + - '"independent" (the current default), "together", "forwards" or "legacy_unstable-backwards".', - ); - } else if (revealOrder === 'backwards') { - console.error( - 'The rendering order of is changing. ' + - 'To be future compatible you must specify revealOrder="legacy_unstable-backwards" instead.', - ); - } else if (typeof revealOrder === 'string') { + if (typeof revealOrder === 'string') { switch (revealOrder.toLowerCase()) { case 'together': case 'forwards': @@ -3314,18 +3305,7 @@ function validateTailOptions( const cacheKey = tailMode == null ? 'null' : tailMode; if (!didWarnAboutTailOptions[cacheKey]) { if (tailMode == null) { - if ( - revealOrder === 'forwards' || - revealOrder === 'backwards' || - revealOrder === 'unstable_legacy-backwards' - ) { - didWarnAboutTailOptions[cacheKey] = true; - console.error( - 'The default for the prop is changing. ' + - 'To be future compatible you must explictly specify either ' + - '"visible" (the current default), "collapsed" or "hidden".', - ); - } + // The default tail is now "hidden". } else if ( tailMode !== 'visible' && tailMode !== 'collapsed' && @@ -3338,6 +3318,7 @@ function validateTailOptions( tailMode, ); } else if ( + revealOrder != null && revealOrder !== 'forwards' && revealOrder !== 'backwards' && revealOrder !== 'unstable_legacy-backwards' @@ -3345,7 +3326,7 @@ function validateTailOptions( didWarnAboutTailOptions[cacheKey] = true; console.error( ' is only valid if revealOrder is ' + - '"forwards" or "backwards". ' + + '"forwards" (default) or "backwards". ' + 'Did you mean to specify revealOrder="forwards"?', tailMode, ); @@ -3386,6 +3367,17 @@ function initSuspenseListRenderState( } } +function reverseChildren(fiber: Fiber): void { + let row = fiber.child; + fiber.child = null; + while (row !== null) { + const nextRow = row.sibling; + row.sibling = fiber.child; + fiber.child = row; + row = nextRow; + } +} + // This can end up rendering this component multiple passes. // The first pass splits the children fibers into two sets. A head and tail. // We first render the head. If anything is in fallback state, we do another @@ -3424,7 +3416,16 @@ function updateSuspenseListComponent( validateTailOptions(tailMode, revealOrder); validateSuspenseListChildren(newChildren, revealOrder); - reconcileChildren(current, workInProgress, newChildren, renderLanes); + if (revealOrder === 'backwards' && current !== null) { + // For backwards the current mounted set will be backwards. Reconciling against it + // will lead to mismatches and reorders. We need to swap the original set first + // and then restore it afterwards. + reverseChildren(current); + reconcileChildren(current, workInProgress, newChildren, renderLanes); + reverseChildren(current); + } else { + reconcileChildren(current, workInProgress, newChildren, renderLanes); + } // Read how many children forks this set pushed so we can push it every time we retry. const treeForkCount = getIsHydrating() ? getForksAtLevel(workInProgress) : 0; @@ -3449,31 +3450,37 @@ function updateSuspenseListComponent( workInProgress.memoizedState = null; } else { switch (revealOrder) { - case 'forwards': { + case 'backwards': { + // We're going to find the first row that has existing content. + // We are also going to reverse the order of anything in the existing content + // since we want to actually render them backwards from the reconciled set. + // The tail is left in order, because it'll be added to the front as we + // complete each item. const lastContentRow = findLastContentRow(workInProgress.child); let tail; if (lastContentRow === null) { // The whole list is part of the tail. - // TODO: We could fast path by just rendering the tail now. tail = workInProgress.child; workInProgress.child = null; } else { // Disconnect the tail rows after the content row. - // We're going to render them separately later. + // We're going to render them separately later in reverse order. tail = lastContentRow.sibling; lastContentRow.sibling = null; + // We have to now reverse the main content so it renders backwards too. + reverseChildren(workInProgress); } + // TODO: If workInProgress.child is null, we can continue on the tail immediately. initSuspenseListRenderState( workInProgress, - false, // isBackwards + true, // isBackwards tail, - lastContentRow, + null, // last tailMode, treeForkCount, ); break; } - case 'backwards': case 'unstable_legacy-backwards': { // We're going to find the first row that has existing content. // At the same time we're going to reverse the list of everything @@ -3517,10 +3524,37 @@ function updateSuspenseListComponent( ); break; } - default: { - // The default reveal order is the same as not having + case 'independent': { + // The "independent" reveal order is the same as not having // a boundary. workInProgress.memoizedState = null; + break; + } + // The default is now forwards. + case 'forwards': + default: { + const lastContentRow = findLastContentRow(workInProgress.child); + let tail; + if (lastContentRow === null) { + // The whole list is part of the tail. + // TODO: We could fast path by just rendering the tail now. + tail = workInProgress.child; + workInProgress.child = null; + } else { + // Disconnect the tail rows after the content row. + // We're going to render them separately later. + tail = lastContentRow.sibling; + lastContentRow.sibling = null; + } + initSuspenseListRenderState( + workInProgress, + false, // isBackwards + tail, + lastContentRow, + tailMode, + treeForkCount, + ); + break; } } } diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index d3abcb466e5..0b1f1e4366e 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -698,7 +698,11 @@ function cutOffTailIfNeeded( return; } switch (renderState.tailMode) { - case 'hidden': { + case 'visible': { + // Everything should remain as it was. + break; + } + case 'collapsed': { // Any insertions at the end of the tail list after this point // should be invisible. If there are already mounted boundaries // anything before them are not considered for collapsing. @@ -716,7 +720,13 @@ function cutOffTailIfNeeded( // last rendered item. if (lastTailNode === null) { // All remaining items in the tail are insertions. - renderState.tail = null; + if (!hasRenderedATailFallback && renderState.tail !== null) { + // We suspended during the head. We want to show at least one + // row at the tail. So we'll keep on and cut off the rest. + renderState.tail.sibling = null; + } else { + renderState.tail = null; + } } else { // Detach the insertion after the last node that was already // inserted. @@ -724,7 +734,9 @@ function cutOffTailIfNeeded( } break; } - case 'collapsed': { + // Hidden is now the default. + case 'hidden': + default: { // Any insertions at the end of the tail list after this point // should be invisible. If there are already mounted boundaries // anything before them are not considered for collapsing. @@ -742,13 +754,7 @@ function cutOffTailIfNeeded( // last rendered item. if (lastTailNode === null) { // All remaining items in the tail are insertions. - if (!hasRenderedATailFallback && renderState.tail !== null) { - // We suspended during the head. We want to show at least one - // row at the tail. So we'll keep on and cut off the rest. - renderState.tail.sibling = null; - } else { - renderState.tail = null; - } + renderState.tail = null; } else { // Detach the insertion after the last node that was already // inserted. @@ -1795,7 +1801,8 @@ function completeWork( // This might have been modified. if ( renderState.tail === null && - renderState.tailMode === 'hidden' && + renderState.tailMode !== 'collapsed' && + renderState.tailMode !== 'visible' && !renderedTail.alternate && !getIsHydrating() // We don't cut it if we're hydrating. ) { @@ -1831,10 +1838,6 @@ function completeWork( } } if (renderState.isBackwards) { - // The effect list of the backwards tail will have been added - // to the end. This breaks the guarantee that life-cycles fire in - // sibling order but that isn't a strong guarantee promised by React. - // Especially since these might also just pop in during future commits. // Append to the beginning of the list. renderedTail.sibling = workInProgress.child; workInProgress.child = renderedTail; diff --git a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js index 64ab4f29fcd..695c3697154 100644 --- a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js +++ b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js @@ -79,10 +79,7 @@ export function findFirstSuspended(row: Fiber): null | Fiber { node.tag === SuspenseListComponent && // Independent revealOrder can't be trusted because it doesn't // keep track of whether it suspended or not. - (node.memoizedProps.revealOrder === 'forwards' || - node.memoizedProps.revealOrder === 'backwards' || - node.memoizedProps.revealOrder === 'unstable_legacy-backwards' || - node.memoizedProps.revealOrder === 'together') + node.memoizedProps.revealOrder !== 'independent' ) { const didSuspend = (node.flags & DidCapture) !== NoFlags; if (didSuspend) { diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.js index bfbf165dbab..ba5b668652f 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.js @@ -218,7 +218,7 @@ describe('ReactSuspenseList', () => { }); // @gate enableSuspenseList - it('warns if no revealOrder is specified', async () => { + it('behaves as revealOrder=forwards by default', async () => { const A = createAsyncText('A'); const B = createAsyncText('B'); const C = createAsyncText('C'); @@ -239,54 +239,36 @@ describe('ReactSuspenseList', () => { ); } + ReactNoop.render(); + + await waitForAll(['Suspend! [A]', 'Loading A']); + + expect(ReactNoop).toMatchRenderedOutput(null); + await A.resolve(); - ReactNoop.render(); + await waitForAll(['A', 'Suspend! [B]', 'Loading B']); - await waitForAll([ - 'A', - 'Suspend! [B]', - 'Loading B', - 'Suspend! [C]', - 'Loading C', - // pre-warming - 'Suspend! [B]', - 'Suspend! [C]', - ]); + // Incremental loading is suspended. + jest.advanceTimersByTime(500); - assertConsoleErrorDev([ - 'The default for the prop is changing. ' + - 'To be future compatible you must explictly specify either ' + - '"independent" (the current default), "together", "forwards" or "legacy_unstable-backwards".' + - '\n in SuspenseList (at **)' + - '\n in Foo (at **)', - ]); + expect(ReactNoop).toMatchRenderedOutput(A); - expect(ReactNoop).toMatchRenderedOutput( - <> - A - Loading B - Loading C - , - ); + await act(() => B.resolve()); + assertLog(['B', 'Suspend! [C]', 'Loading C']); - await act(() => C.resolve()); - assertLog( - gate('alwaysThrottleRetries') - ? ['Suspend! [B]', 'C', 'Suspend! [B]'] - : ['C'], - ); + // Incremental loading is suspended. + jest.advanceTimersByTime(500); expect(ReactNoop).toMatchRenderedOutput( <> A - Loading B - C + B , ); - await act(() => B.resolve()); - assertLog(['B']); + await act(() => C.resolve()); + assertLog(['C']); expect(ReactNoop).toMatchRenderedOutput( <> @@ -1040,7 +1022,7 @@ describe('ReactSuspenseList', () => { }); // @gate enableSuspenseList - it('warns if revealOrder="backwards" is specified', async () => { + it('displays each items in "backwards" order', async () => { const A = createAsyncText('A'); const B = createAsyncText('B'); const C = createAsyncText('C'); @@ -1048,14 +1030,14 @@ describe('ReactSuspenseList', () => { function Foo() { return ( - }> - + }> + }> - }> - + }> + ); @@ -1074,14 +1056,6 @@ describe('ReactSuspenseList', () => { 'Suspend! [C]', ]); - assertConsoleErrorDev([ - 'The rendering order of is changing. ' + - 'To be future compatible you must specify ' + - 'revealOrder="legacy_unstable-backwards" instead.' + - '\n in SuspenseList (at **)' + - '\n in Foo (at **)', - ]); - expect(ReactNoop).toMatchRenderedOutput( <> Loading A @@ -1119,7 +1093,7 @@ describe('ReactSuspenseList', () => { }); // @gate enableSuspenseList - it('displays each items in "backwards" order', async () => { + it('displays each items in "backwards" order (legacy)', async () => { const A = createAsyncText('A'); const B = createAsyncText('B'); const C = createAsyncText('C'); @@ -1699,26 +1673,65 @@ describe('ReactSuspenseList', () => { }); // @gate enableSuspenseList - it('warns if no tail option is specified', async () => { + it('behaves as tail=hidden if no tail option is specified', async () => { + const A = createAsyncText('A'); + const B = createAsyncText('B'); + const C = createAsyncText('C'); + function Foo() { return ( - A - B + }> + + + }> + + + }> + + ); } - await act(() => { - ReactNoop.render(); - }); - assertConsoleErrorDev([ - 'The default for the prop is changing. ' + - 'To be future compatible you must explictly specify either ' + - '"visible" (the current default), "collapsed" or "hidden".' + - '\n in SuspenseList (at **)' + - '\n in Foo (at **)', - ]); + ReactNoop.render(); + + await waitForAll(['Suspend! [A]', 'Loading A']); + + expect(ReactNoop).toMatchRenderedOutput(null); + + await A.resolve(); + + await waitForAll(['A', 'Suspend! [B]', 'Loading B']); + + // Incremental loading is suspended. + jest.advanceTimersByTime(500); + + expect(ReactNoop).toMatchRenderedOutput(A); + + await act(() => B.resolve()); + assertLog(['B', 'Suspend! [C]', 'Loading C']); + + // Incremental loading is suspended. + jest.advanceTimersByTime(500); + + expect(ReactNoop).toMatchRenderedOutput( + <> + A + B + , + ); + + await act(() => C.resolve()); + assertLog(['C']); + + expect(ReactNoop).toMatchRenderedOutput( + <> + A + B + C + , + ); }); // @gate enableSuspenseList @@ -1758,7 +1771,7 @@ describe('ReactSuspenseList', () => { }); assertConsoleErrorDev([ ' is only valid if ' + - 'revealOrder is "forwards" or "backwards". ' + + 'revealOrder is "forwards" (default) or "backwards". ' + 'Did you mean to specify revealOrder="forwards"?' + '\n in SuspenseList (at **)' + '\n in Foo (at **)', diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 07408d64e88..33c0ab68a8e 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -1913,7 +1913,7 @@ function renderSuspenseListRows( task: Task, keyPath: KeyNode, rows: Array, - revealOrder: 'forwards' | 'backwards' | 'unstable_legacy-backwards', + revealOrder: void | 'forwards' | 'backwards' | 'unstable_legacy-backwards', ): void { // This is a fork of renderChildrenArray that's aware of tracking rows. const prevKeyPath = task.keyPath; @@ -2017,7 +2017,11 @@ function renderSuspenseListRows( const parentSegment = task.blockedSegment; const childIndex = parentSegment.children.length; const insertionIndex = parentSegment.chunks.length; - for (let i = totalChildren - 1; i >= 0; i--) { + for (let n = 0; n < totalChildren; n++) { + const i = + revealOrder === 'unstable_legacy-backwards' + ? totalChildren - 1 - n + : n; const node = rows[i]; task.row = previousSuspenseListRow = createSuspenseListRow( previousSuspenseListRow, @@ -2098,11 +2102,7 @@ function renderSuspenseList( const revealOrder: SuspenseListRevealOrder = props.revealOrder; // TODO: Support tail hidden/collapsed modes. // const tailMode: SuspenseListTailMode = props.tail; - if ( - revealOrder === 'forwards' || - revealOrder === 'backwards' || - revealOrder === 'unstable_legacy-backwards' - ) { + if (revealOrder !== 'independent' && revealOrder !== 'together') { // For ordered reveal, we need to produce rows from the children. if (isArray(children)) { renderSuspenseListRows(request, task, keyPath, children, revealOrder);