Skip to content

Commit

Permalink
Change retry priority to "Never" for dehydrated boundaries (#17105)
Browse files Browse the repository at this point in the history
This changes the "default" retryTime to NoWork which schedules at Normal
pri.

Dehydrated bouundaries normally hydrate at Never priority except when they
retry where we accidentally increased them to Normal because Never was used
as the default value. This changes it so NoWork is the default.

Dehydrated boundaries however get initialized to Never as the default.

Therefore they now hydrate as Never pri unless their priority gets
increased by a forced rerender or selective hydration.

This revealed that erroring at this Never priority can cause an infinite
rerender. So I fixed that too.
  • Loading branch information
sebmarkbage committed Oct 16, 2019
1 parent 2c832b4 commit 6ff23f2
Show file tree
Hide file tree
Showing 4 changed files with 102 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ describe('ReactDOMServerPartialHydration', () => {
ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.enableSuspenseCallback = true;
ReactFeatureFlags.enableFlareAPI = true;
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;

React = require('react');
ReactDOM = require('react-dom');
Expand Down Expand Up @@ -2475,4 +2476,85 @@ describe('ReactDOMServerPartialHydration', () => {

document.body.removeChild(container);
});

it('finishes normal pri work before continuing to hydrate a retry', async () => {
let suspend = false;
let resolve;
let promise = new Promise(resolvePromise => (resolve = resolvePromise));
let ref = React.createRef();

function Child() {
if (suspend) {
throw promise;
} else {
Scheduler.unstable_yieldValue('Child');
return 'Hello';
}
}

function Sibling() {
Scheduler.unstable_yieldValue('Sibling');
React.useLayoutEffect(() => {
Scheduler.unstable_yieldValue('Commit Sibling');
});
return 'World';
}

// Avoid rerendering the tree by hoisting it.
const tree = (
<Suspense fallback="Loading...">
<span ref={ref}>
<Child />
</span>
</Suspense>
);

function App({showSibling}) {
return (
<div>
{tree}
{showSibling ? <Sibling /> : null}
</div>
);
}

suspend = false;
let finalHTML = ReactDOMServer.renderToString(<App />);
expect(Scheduler).toHaveYielded(['Child']);

let container = document.createElement('div');
container.innerHTML = finalHTML;

suspend = true;
let root = ReactDOM.unstable_createRoot(container, {hydrate: true});
root.render(<App showSibling={false} />);
expect(Scheduler).toFlushAndYield([]);

expect(ref.current).toBe(null);
expect(container.textContent).toBe('Hello');

// Resolving the promise should continue hydration
suspend = false;
resolve();
await promise;

Scheduler.unstable_advanceTime(100);

// Before we have a chance to flush it, we'll also render an update.
root.render(<App showSibling={true} />);

// When we flush we expect the Normal pri render to take priority
// over hydration.
expect(Scheduler).toFlushAndYieldThrough(['Sibling', 'Commit Sibling']);

// We shouldn't have hydrated the child yet.
expect(ref.current).toBe(null);
// But we did have a chance to update the content.
expect(container.textContent).toBe('HelloWorld');

expect(Scheduler).toFlushAndYield(['Child']);

// Now we're hydrated.
expect(ref.current).not.toBe(null);
});
});
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -1482,7 +1482,7 @@ function validateFunctionComponentInDev(workInProgress: Fiber, Component: any) {

const SUSPENDED_MARKER: SuspenseState = {
dehydrated: null,
retryTime: Never,
retryTime: NoWork,
};

function shouldRemainOnFallback(
Expand Down
4 changes: 3 additions & 1 deletion packages/react-reconciler/src/ReactFiberSuspenseComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ export type SuspenseState = {|
// to check things like isSuspenseInstancePending.
dehydrated: null | SuspenseInstance,
// Represents the earliest expiration time we should attempt to hydrate
// a dehydrated boundary at. Never is the default.
// a dehydrated boundary at.
// Never is the default for dehydrated boundaries.
// NoWork is the default for normal boundaries, which turns into "normal" pri.
retryTime: ExpirationTime,
|};

Expand Down
30 changes: 16 additions & 14 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -741,17 +741,19 @@ function finishConcurrentRender(
// statement, but eslint doesn't know about invariant, so it complains
// if I do. eslint-disable-next-line no-fallthrough
case RootErrored: {
if (expirationTime !== Idle) {
// If this was an async render, the error may have happened due to
// a mutation in a concurrent event. Try rendering one more time,
// synchronously, to see if the error goes away. If there are
// lower priority updates, let's include those, too, in case they
// fix the inconsistency. Render at Idle to include all updates.
markRootExpiredAtTime(root, Idle);
break;
}
// Commit the root in its errored state.
commitRoot(root);
// If this was an async render, the error may have happened due to
// a mutation in a concurrent event. Try rendering one more time,
// synchronously, to see if the error goes away. If there are
// lower priority updates, let's include those, too, in case they
// fix the inconsistency. Render at Idle to include all updates.
// If it was Idle or Never or some not-yet-invented time, render
// at that time.
markRootExpiredAtTime(
root,
expirationTime > Idle ? Idle : expirationTime,
);
// We assume that this second render pass will be synchronous
// and therefore not hit this path again.
break;
}
case RootSuspended: {
Expand Down Expand Up @@ -2376,7 +2378,7 @@ function retryTimedOutBoundary(
// previously was rendered in its fallback state. One of the promises that
// suspended it has resolved, which means at least part of the tree was
// likely unblocked. Try rendering again, at a new expiration time.
if (retryTime === Never) {
if (retryTime === NoWork) {
const suspenseConfig = null; // Retries don't carry over the already committed update.
const currentTime = requestCurrentTime();
retryTime = computeExpirationForFiber(
Expand All @@ -2395,15 +2397,15 @@ function retryTimedOutBoundary(

export function retryDehydratedSuspenseBoundary(boundaryFiber: Fiber) {
const suspenseState: null | SuspenseState = boundaryFiber.memoizedState;
let retryTime = Never;
let retryTime = NoWork;
if (suspenseState !== null) {
retryTime = suspenseState.retryTime;
}
retryTimedOutBoundary(boundaryFiber, retryTime);
}

export function resolveRetryThenable(boundaryFiber: Fiber, thenable: Thenable) {
let retryTime = Never; // Default
let retryTime = NoWork; // Default
let retryCache: WeakSet<Thenable> | Set<Thenable> | null;
if (enableSuspenseServerRenderer) {
switch (boundaryFiber.tag) {
Expand Down

0 comments on commit 6ff23f2

Please sign in to comment.