Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fizz] Always insert a dummy node with an ID into fallbacks #21272

Merged
merged 1 commit into from
Apr 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 76 additions & 2 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ describe('ReactDOMFizzServer', () => {
if (
node.tagName !== 'SCRIPT' &&
node.tagName !== 'TEMPLATE' &&
node.tagName !== 'template' &&
!node.hasAttribute('hidden') &&
!node.hasAttribute('aria-hidden')
) {
Expand Down Expand Up @@ -153,7 +154,6 @@ describe('ReactDOMFizzServer', () => {
}
}

/*
function rejectText(text, error) {
const record = textCache.get(text);
if (record === undefined) {
Expand All @@ -169,7 +169,6 @@ describe('ReactDOMFizzServer', () => {
thenable.pings.forEach(t => t());
}
}
*/

function readText(text) {
const record = textCache.get(text);
Expand Down Expand Up @@ -800,4 +799,79 @@ describe('ReactDOMFizzServer', () => {
</div>,
);
});

it('client renders a boundary if it errors before finishing the fallback', async () => {
function App({isClient}) {
return (
<Suspense fallback="Loading root...">
<div>
<Suspense fallback={<AsyncText text="Loading..." />}>
<h1>
{isClient ? <Text text="Hello" /> : <AsyncText text="Hello" />}
</h1>
</Suspense>
</div>
</Suspense>
);
}

const loggedErrors = [];
let controls;
await act(async () => {
controls = ReactDOMFizzServer.pipeToNodeWritable(
<App isClient={false} />,
writable,
{
onError(x) {
loggedErrors.push(x);
},
},
);
controls.startWriting();
});

// We're still showing a fallback.

// Attempt to hydrate the content.
const root = ReactDOM.unstable_createRoot(container, {hydrate: true});
root.render(<App isClient={true} />);
Scheduler.unstable_flushAll();

// We're still loading because we're waiting for the server to stream more content.
expect(getVisibleChildren(container)).toEqual('Loading root...');

expect(loggedErrors).toEqual([]);

const theError = new Error('Test');
// Error the content, but we don't have a fallback yet.
await act(async () => {
rejectText('Hello', theError);
});

expect(loggedErrors).toEqual([theError]);

// We still can't render it on the client because we haven't unblocked the parent.
Scheduler.unstable_flushAll();
expect(getVisibleChildren(container)).toEqual('Loading root...');

// Unblock the loading state
await act(async () => {
resolveText('Loading...');
});

// Now we're able to show the inner boundary.
expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);

// That will let us client render it instead.
Scheduler.unstable_flushAll();

// The client rendered HTML is now in place.
expect(getVisibleChildren(container)).toEqual(
<div>
<h1>Hello</h1>
</div>,
);

expect(loggedErrors).toEqual([theError]);
});
});
17 changes: 15 additions & 2 deletions packages/react-server/src/ReactFizzServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -419,17 +419,30 @@ function renderSuspenseBoundary(
task.blockedSegment = parentSegment;
}

// This injects an extra segment just to contain an empty tag with an ID.
// This means that we're not actually using the assignID anywhere.
// TODO: Rethink the assignID approach.
pushEmpty(boundarySegment.chunks, request.responseState, newBoundary.id);
const innerSegment = createPendingSegment(
request,
boundarySegment.chunks.length,
null,
boundarySegment.formatContext,
);
boundarySegment.status = COMPLETED;
boundarySegment.children.push(innerSegment);

// We create suspended task for the fallback because we don't want to actually work
// on it yet in case we finish the main content, so we queue for later.
const suspendedFallbackTask = createTask(
request,
fallback,
parentBoundary,
boundarySegment,
innerSegment,
fallbackAbortSet,
task.legacyContext,
task.context,
newBoundary.id, // This is the ID we want to give this fallback so we can replace it later.
null,
);
// TODO: This should be queued at a separate lower priority queue so that we only work
// on preparing fallbacks if we don't have any more main content to task on.
Expand Down