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

Fix hydration bug with nested suspense boundaries #16288

Merged
merged 2 commits into from
Aug 5, 2019

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Aug 4, 2019

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 (instead of left alone).

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.

Likewise, if the getNextHydratable would've skipped over the comment instead of deleted it, it also would've recovered correctly. So it's a combination of both bugs that cause this.

TODO: Test.

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.
@sizebot
Copy link

sizebot commented Aug 4, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS

@sebmarkbage sebmarkbage merged commit 606f76b into facebook:master Aug 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants