Skip to content

Commit

Permalink
Fix hydration bug with nested suspense boundaries (#16288)
Browse files Browse the repository at this point in the history
This happens in this case: `<!--$!--><!--$!-->...<!--/$--><!--/$-->...`

getNextHydratableInstanceAfterSuspenseInstance didn't take
SUSPENSE_FALLBACK_START_DATA or SUSPENSE_PENDING_START_DATA into account
so if a boundary was in one of those states, it wouldn't be considered to
push the stack of boundaries. As a result the first end comment was
considered the end but it really should've been the second end comment.

The next comment then would've been considered something that can be
skipped. However, since the logic in there considered all comments as
"hydratable", it was considered a hydratable node. Since it was considered
a node that didn't actually correspond to anything in the React tree it got
deleted.

The HTML is now `<!--$!--><!--$!-->...<!--/$-->...` and the trailing
content is now hydrated since it did match something.

Next, since this was client rendered, we're going to delete the suspended
boundary by calling clearSuspenseBoundary and then inserting new content.
However, clearSuspenseBoundary *is* aware of SUSPENSE_FALLBACK_START_DATA
and assumes that there must be another `<!--/$-->` after the first one.
As a result it deleted the trailing content from the DOM since it should
be part of the boundary. However, those DOM nodes were already hydrated in
the React tree. So we end up in an inconsistent state.

When we then try to insert the new content we throw as a result.

I think we would have recovered correctly if clearSuspenseBoundary and
getNextHydratableInstanceAfterSuspenseInstance had the *same* bug but
because they were inconsistent we ended up in a bad place.
  • Loading branch information
sebmarkbage committed Aug 5, 2019
1 parent a1dbb85 commit 606f76b
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1003,4 +1003,58 @@ describe('ReactDOMServerPartialHydration', () => {
let div = container.getElementsByTagName('div')[0];
expect(ref.current).toBe(div);
});

it('can client render nested boundaries', async () => {
let suspend = false;
let promise = new Promise(() => {});
let ref = React.createRef();

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

function App() {
return (
<div>
<Suspense
fallback={
<React.Fragment>
<Suspense fallback="Loading...">
<Child />
</Suspense>
<span>Inner Sibling</span>
</React.Fragment>
}>
<Child />
</Suspense>
<span ref={ref}>Sibling</span>
</div>
);
}

suspend = true;
let html = ReactDOMServer.renderToString(<App />);

let container = document.createElement('div');
container.innerHTML = html + '<!--unrelated comment-->';

let span = container.getElementsByTagName('span')[1];

suspend = false;
let root = ReactDOM.unstable_createRoot(container, {hydrate: true});
root.render(<App />);
Scheduler.unstable_flushAll();
jest.runAllTimers();

expect(ref.current).toBe(span);
expect(span.parentNode).not.toBe(null);

// It leaves non-React comments alone.
expect(container.lastChild.nodeType).toBe(8);
expect(container.lastChild.data).toBe('unrelated comment');
});
});
23 changes: 13 additions & 10 deletions packages/react-dom/src/client/ReactDOMHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -610,15 +610,14 @@ function getNextHydratable(node) {
}
if (enableSuspenseServerRenderer) {
if (nodeType === COMMENT_NODE) {
break;
}
const nodeData = (node: any).data;
if (
nodeData === SUSPENSE_START_DATA ||
nodeData === SUSPENSE_FALLBACK_START_DATA ||
nodeData === SUSPENSE_PENDING_START_DATA
) {
break;
const nodeData = (node: any).data;
if (
nodeData === SUSPENSE_START_DATA ||
nodeData === SUSPENSE_FALLBACK_START_DATA ||
nodeData === SUSPENSE_PENDING_START_DATA
) {
break;
}
}
}
}
Expand Down Expand Up @@ -691,7 +690,11 @@ export function getNextHydratableInstanceAfterSuspenseInstance(
} else {
depth--;
}
} else if (data === SUSPENSE_START_DATA) {
} else if (
data === SUSPENSE_START_DATA ||
data === SUSPENSE_FALLBACK_START_DATA ||
data === SUSPENSE_PENDING_START_DATA
) {
depth++;
}
}
Expand Down

0 comments on commit 606f76b

Please sign in to comment.