Suppress hydration warnings when a preceding sibling suspends #24404
Conversation
|
Comparing: 0dc4e66...be1cefa Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
ef55aca to
cc0158c
Compare
| const secondToLastCall = | ||
| mockError.mock.calls[mockError.mock.calls.length - 2]; | ||
| expect(secondToLastCall).toEqual([ | ||
| 'Warning: Expected server HTML to contain a matching <%s> in <%s>.%s', | ||
| 'div', | ||
| 'div', | ||
| 'article', | ||
| 'section', | ||
| '\n' + | ||
| ' in div (at **)\n' + | ||
| ' in article (at **)\n' + |
There was a problem hiding this comment.
This warning used to be for trying to match the first Component div against the Text node. It was an expression of sorts of the issue brought up in #24384
This warning now is the intentional mismatch between article and div that the test introduces between client/server renders.
| @@ -514,8 +518,6 @@ function throwException( | |||
| } else { | |||
| // This is a regular error, not a Suspense wakeable. | |||
| if (getIsHydrating() && sourceFiber.mode & ConcurrentMode) { | |||
| markDidSuspendWhileHydratingDEV(); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Well there is a test to suppress warnings after a hydration error
Are you concerned about an error thrown from user code?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah yeah that makes sense. I’ll get another test in there soon
There was a problem hiding this comment.
Andrew is the new test covering what you hoped it would?
1d103d0 to
f3cdd36
Compare
packages/react-dom/src/__tests__/ReactDOMFizzSuppressHydrationWarning-test.js
Outdated
Show resolved
Hide resolved
packages/react-reconciler/src/ReactFiberHydrationContext.old.js
Outdated
Show resolved
Hide resolved
If a components suspends during hydration we expect there to be mismatches with server rendered HTML but we were not always supressing warning messages related to these expected mismatches
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.
This test was actually subject to the project identified in the issue fixed in this branch. After fixing the underlying issue the assertion logic needed to change to pick the right warning which now emits after hydration successfully completes on promise resolution. I changed the container type to 'section' to make the error message slightly easier to read/understand (for me)
For unknown reasons the didSuspend was being set only on the error path and nto the suspense path. The original change hoisted this to happen on both paths. This change moves the didSuspend call to the suspense path only. This appears to be a noop because if the error path marked didSuspend it would suppress later warnings but if it does not the warning functions themsevles do that suppression (after the first one which necessarily already happened)
the orignial behavior applied the hydration warning bailout to error paths only. originally I moved it to Suspense paths only but this commit restores it to both paths and renames the marker function as didThrow rather than didSuspend The logic here is that for either case if we get a mismatch in hydration we want to warm up components but effectively consider the hydration for this boundary halted
…hould only affect dev behavior
851dde2 to
6ddb123
Compare
6ddb123 to
07a2eee
Compare
|
@gnoff Is this ready to merge or do you plan to add the test for the error case? |
Before this PR if there was a hydration error thrown in a ConcurrentMode tree the hydration procerss would enter a didSuspend state where further errors would be suppressed.
This did not previously happen however when a promise was thrown which would cause a Suspense boundary to suspend. Since it is typical for there to be hydration errors if a preceding sibling suspends because the server rendered content represented by that suspending component is not being claimed the didSuspend state is likely to follow quickly after (on the first non-promise thrown value)
This PR moves where the didSuspend state is activated to be for both Error and Promise thrown values.
closes #24384