Skip to content

Suppress hydration warnings when a preceding sibling suspends #24404

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.

Sign up for GitHub

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

Merged
gnoff merged 14 commits into from
Apr 20, 2022
Merged

Suppress hydration warnings when a preceding sibling suspends #24404

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a9ccb8b
Add failing test case for #24384
gnoff Apr 19, 2022
b530537
Mark hydration as suspending on every thrownException
gnoff Apr 19, 2022
dc1cd0b
Fix failing test related to client render fallbacks
gnoff Apr 19, 2022
b06d2e9
Only mark didSuspend on suspense path
gnoff Apr 20, 2022
5e1cd0a
gate test on hydration fallback flags
gnoff Apr 20, 2022
668d8c9
refactor didSuspend to didSuspendOrError
gnoff Apr 20, 2022
3bccb4d
factor tests to assert different behavior between prod and dev
gnoff Apr 20, 2022
eeede50
add DEV suffix to didSuspendOrError to better indicate this feature s…
gnoff Apr 20, 2022
c827582
move tests back to ReactDOMFizzServer-test
gnoff Apr 20, 2022
945352e
fix comment casing
gnoff Apr 20, 2022
07a2eee
drop extra flag gates in tests
gnoff Apr 20, 2022
8839f1b
add test for user error case
gnoff Apr 20, 2022
3fd1eb6
remove unnecessary gate
gnoff Apr 20, 2022
be1cefa
Make test better
gnoff Apr 20, 2022
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
Prev Previous commit
Next Next commit
Mark hydration as suspending on every thrownException 
previously hydration would only be marked as supsending when a genuine error was thrown. This created an opportunity for a hydration mismatch that would warn after which later hydration mismatches would not lead to warnings. By moving the marker check earlier in the thrownException function we get the hydration context to enter the didSuspend state on both error and thrown promise cases which eliminates this gap.
  • Loading branch information
@gnoff
gnoff committed Apr 20, 2022
commit b530537361f0d80a1e66560690d7135995508534
6 changes: 4 additions & 2 deletions packages/react-reconciler/src/ReactFiberThrow.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,10 @@ function throwException(
}
}

if (getIsHydrating() && sourceFiber.mode & ConcurrentMode) {
markDidSuspendWhileHydratingDEV();
}

if (
value !== null &&
typeof value === 'object' &&
Expand Down Expand Up @@ -514,8 +518,6 @@ function throwException(
} else {
// This is a regular error, not a Suspense wakeable.
if (getIsHydrating() && sourceFiber.mode & ConcurrentMode) {
markDidSuspendWhileHydratingDEV();
Copy link
Collaborator

@acdlite acdlite Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was just a mistake and it was meant to go into a Suspense-only path, but it went into an error-only path instead.

What happens if you move this to a Suspense-only path? Like line 455.

Copy link
Collaborator Author

@gnoff gnoff Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah i assumed it was intentionally opting into the warning suppression after the first hydration error by using suspend that way and didn't consider it was maybe just an oversight. seems like all tests pass.

I think this is sort of a noop in that by fake suspending on real error you opt out of future warns but the warns already opted out after the first one so in the end no observable difference

Copy link
Collaborator

@sebmarkbage sebmarkbage Apr 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be intentional because if an error is thrown, and nothing suspends, we still continue rendering siblings I believe. So those siblings would still be hydrating and causing more fake errors which would be confusing.

Do we have a test for that?

Really markDidSuspendWhileHydratingDEV is a misnomer because it's more like markDidThrowWhileHydratingDEV.

Copy link
Collaborator

@acdlite acdlite Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recall writing a test for the error case but maybe. It does seem like it should be called in both cases, reason I asked whether it passes if you move it to be Suspense-only is I was curious if we already tested that

Copy link
Collaborator Author

@gnoff gnoff Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

functionally they are equivelent (I checked in a test) but I agree the didThrow seems to map better onto what we expect to happen after a throw

If you error we would expect hydration to fail for later siblings since we likely didn't consume the current hydration step and will not advance.

If you Suspend we have the exact same logic

The reason this distinction is unobservable is the hydration warning will only ever log once and the errors are not suppressed so you end up with many thrown errors in either case and 1 logged error (didSuspend suppression or didAlreadyLog supression)

I'm in favor of reframing as didThrow and leaving it in both branches so I'll work on that unless someone wants to real me in

Copy link
Collaborator Author

@gnoff gnoff Apr 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well there is a test to suppress warnings after a hydration error

https://github.com/gnoff/react/blob/hydration-warning-dev/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js#L2993-L3005

Are you concerned about an error thrown from user code?

Copy link
Collaborator

@acdlite acdlite Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think we should have a test for a user error, too, since the fact that hydration mismatches cause an error internally is an implementation detail; it doesn't have to work that way.

Copy link
Collaborator

@acdlite acdlite Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, the other way it could work is that we wait to walk the DOM tree until right before the commit phase. Then you'd be able to start rendering even before the HTML has arrived. In that case, we wouldn't know there was a mismatch until after the render phase had already completed. So instead of erroring, we'd throw out the work-in-progress and start again.

Copy link
Collaborator Author

@gnoff gnoff Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah that makes sense. I’ll get another test in there soon

Copy link
Collaborator Author

@gnoff gnoff Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Andrew is the new test covering what you hoped it would?


const suspenseBoundary = getNearestSuspenseBoundaryToCapture(returnFiber);
// If the error was thrown during hydration, we may be able to recover by
// discarding the dehydrated content and switching to a client render.
Expand Down
6 changes: 4 additions & 2 deletions packages/react-reconciler/src/ReactFiberThrow.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,10 @@ function throwException(
}
}

if (getIsHydrating() && sourceFiber.mode & ConcurrentMode) {
markDidSuspendWhileHydratingDEV();
}

if (
value !== null &&
typeof value === 'object' &&
Expand Down Expand Up @@ -514,8 +518,6 @@ function throwException(
} else {
// This is a regular error, not a Suspense wakeable.
if (getIsHydrating() && sourceFiber.mode & ConcurrentMode) {
markDidSuspendWhileHydratingDEV();

const suspenseBoundary = getNearestSuspenseBoundaryToCapture(returnFiber);
// If the error was thrown during hydration, we may be able to recover by
// discarding the dehydrated content and switching to a client render.
Expand Down