Skip to content

Commit

Permalink
Don't prerender siblings of suspended component (#26380)
Browse files Browse the repository at this point in the history
Today if something suspends, React will continue rendering the siblings
of that component.

Our original rationale for prerendering the siblings of a suspended
component was to initiate any lazy fetches that they might contain. This
was when we were more bullish about lazy fetching being a good idea some
of the time (when combined with prefetching), as opposed to our latest
thinking, which is that it's almost always a bad idea.

Another rationale for the original behavior was that the render was I/O
bound, anyway, so we might as do some extra work in the meantime. But
this was before we had the concept of instant loading states: when
navigating to a new screen, it's better to show a loading state as soon
as you can (often a skeleton UI), rather than delay the transition.
(There are still cases where we block the render, when a suitable
loading state is not available; it's just not _all_ cases where
something suspends.) So the biggest issue with our existing
implementation is that the prerendering of the siblings happens within
the same render pass as the one that suspended — _before_ the loading
state appears.

What we should do instead is immediately unwind the stack as soon as
something suspends, to unblock the loading state.

If we want to preserve the ability to prerender the siblings, what we
could do is schedule special render pass immediately after the fallback
is displayed. This is likely what we'll do in the future. However, in
the new implementation of `use`, there's another reason we don't
prerender siblings: so we can preserve the state of the stack when
something suspends, and resume where we left of when the promise
resolves without replaying the parents. The only way to do this
currently is to suspend the entire work loop. Fiber does not currently
support rendering multiple siblings in "parallel". Once you move onto
the next sibling, the stack of the previous sibling is discarded and
cannot be restored. We do plan to implement this feature, but it will
require a not-insignificant refactor.

Given that lazy data fetching is already bad for performance, the best
trade off for now seems to be to disable prerendering of siblings. This
gives us the best performance characteristics when you're following best
practices (i.e. hoist data fetches to Server Components or route
loaders), at the expense of making an already bad pattern a bit worse.

Later, when we implement resumable context stacks, we can reenable
sibling prerendering. Though even then the use case will mostly be to
prerender the CPU-bound work, not lazy fetches.
  • Loading branch information
acdlite authored Mar 21, 2023
1 parent 77ba161 commit 12a1d14
Show file tree
Hide file tree
Showing 33 changed files with 476 additions and 487 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,9 @@ describe('ReactInternalTestUtils', () => {
assertLog([
'A',
'B',
'C',
'D',
// React will try one more time before giving up.
'A',
'B',
'C',
'D',
]);
});

Expand Down
39 changes: 24 additions & 15 deletions packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -208,19 +208,19 @@ describe('ReactCache', () => {
unstable_isConcurrent: true,
},
);
await waitForAll([
'Suspend! [1]',
'Suspend! [2]',
'Suspend! [3]',
'Loading...',
]);
await waitForAll(['Suspend! [1]', 'Loading...']);
jest.advanceTimersByTime(100);
assertLog([
'Promise resolved [1]',
'Promise resolved [2]',
'Promise resolved [3]',
]);
assertLog(['Promise resolved [1]']);
await waitForAll([1, 'Suspend! [2]']);

jest.advanceTimersByTime(100);
assertLog(['Promise resolved [2]']);
await waitForAll([1, 2, 'Suspend! [3]']);

jest.advanceTimersByTime(100);
assertLog(['Promise resolved [3]']);
await waitForAll([1, 2, 3]);

expect(root).toMatchRenderedOutput('123');

// Render 1, 4, 5
Expand All @@ -232,10 +232,16 @@ describe('ReactCache', () => {
</Suspense>,
);

await waitForAll([1, 'Suspend! [4]', 'Suspend! [5]', 'Loading...']);
await waitForAll([1, 'Suspend! [4]', 'Loading...']);

jest.advanceTimersByTime(100);
assertLog(['Promise resolved [4]']);
await waitForAll([1, 4, 'Suspend! [5]', 'Loading...']);

jest.advanceTimersByTime(100);
assertLog(['Promise resolved [4]', 'Promise resolved [5]']);
assertLog(['Promise resolved [5]']);
await waitForAll([1, 4, 5]);

expect(root).toMatchRenderedOutput('145');

// We've now rendered values 1, 2, 3, 4, 5, over our limit of 3. The least
Expand All @@ -254,11 +260,14 @@ describe('ReactCache', () => {
1,
// 2 and 3 suspend because they were evicted from the cache
'Suspend! [2]',
'Suspend! [3]',
'Loading...',
]);
jest.advanceTimersByTime(100);
assertLog(['Promise resolved [2]', 'Promise resolved [3]']);
assertLog(['Promise resolved [2]']);
await waitForAll([1, 2, 'Suspend! [3]', 'Loading...']);

jest.advanceTimersByTime(100);
assertLog(['Promise resolved [3]']);
await waitForAll([1, 2, 3]);
expect(root).toMatchRenderedOutput('123');
});
Expand Down
Loading

0 comments on commit 12a1d14

Please sign in to comment.