Avoid accumulating hydration mismatch errors after the first hydration error #24427
Conversation
|
Comparing: bd08137...d016d50 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) |
…n error If there is a suspended component or an error during hydration there will almost certainly be many additional hydration mismatch errors because the hydration target does not pair up the server rendered html with an expected slot on the client. To avoid spamming users with warnings there was already logic in place to suppress console warnings if such an occurrence happens. This commit takes another approach to avoid queueing thrown errors. when suspending this isn't that big of an issue becuase queued errors are discarded becasue the suspense boundary does not complete. When erroring within a resolved suspense boundary however the root completes and all queued errors are upgraded to recoverable errors and in many cases wihll flood the console. What is worse is the console warnings which offer much more specific guidance on what went wrong (in dev) are suppressed so the user is left with very little actionable information on which to go on and the volume of mismatch errors may distract from identifying the root cause error The hueristic is as follows 1. always queue the first error during hydration 2. always queue non hydration mismatch errors 2. discard hydration mismatch errors before queueing If there is an already queued error or any type
gnoff
force-pushed
the
hydration-error-queueing
branch
from
4db23e0 to
0f8775a
Compare
|
Echoing @xiel, can we add a test where there is no mismatch (from the product code perspective, regardless of impl details)? I don't believe there was any mismatch at all in the original repro in #24404 (comment), so we need a regression test showing that the fix works for that case. |
|
definitely: #24436 |
…not use error properties
| originalError !== null && | ||
| typeof originalError === 'object' && | ||
| typeof originalError.then === 'function' | ||
| hydrationDidSuspendOrErrorDEV() || |
There was a problem hiding this comment.
What’s the timing of this flag being set? We want to be able to still break on the first error so if this flag is already set it wouldn’t.
I think you might need to read it before beginWork to see if it was already set before.
There was a problem hiding this comment.
Begin work catches first and then handleError delegates to throwException where the flag is set. Timing too fragile?
| // a hydration mismatch error. In those cases we do not want to queue the | ||
| // error because there is a more useful error related to hydration that | ||
| // was already queued. | ||
| if (!didThrowNotYetQueuedHydrationMismatchError) { |
There was a problem hiding this comment.
I’m not sure about this. Why do we need this? There’s something about special casing mismatches that doesn’t sit right with me. Partly because we’d have to remember which ones to tag and this is a very specific set of rules that we have to remember (eg only throws, not console errors, if we change something to no longer be a runtime error we have to also change this flag) and it should only be used if the mismatch could be due to not advancing the sibling.
There was a problem hiding this comment.
My general feeling is that if we warn we should error and if we don’t warn we shouldn’t error. It’s unfortunate that the parity here is coincidental rather than structural but if the logic makes sense to suppress mismatch warnings after the first one then I think it makes sense to extend to errors.
If there is any kind of error during hydration it could be reasonable to halt work and switch to client rendering mode immediately in which case there would be no more mismatch errors by definition. The choice to warm up components before client rendering is an implementation detail and thus suppressing the errors from them makes sense to me. If there is a problem the only rational debugging process it seems is to start with the first error (that has a preceding warning) and once fixed either there will be no more errors or the next problem will be uncovered. If we show the other mismatch errors they will generally be of no use to this kind of process.
One caveat is maybe including the first warning and error after successful hydration. So if hydration can somehow recover and then there is a new mismatch error we could included that. I didn’t go that route bc it felt incremental in value and the larger refactor of hydration would need to reimagine these errors anyway
There was a problem hiding this comment.
If there is any kind of error during hydration it could be reasonable to halt work and switch to client rendering mode immediately in which case there would be no more mismatch errors by definition. The choice to warm up components before client rendering is an implementation detail and thus suppressing the errors from them makes sense to me. If there is a problem the only rational debugging process it seems is to start with the first error (that has a preceding warning) and once fixed either there will be no more errors or the next problem will be uncovered. If we show the other mismatch errors they will generally be of no use to this kind of process.
This whole argument makes sense for all errors, but there's nothing in here that says it should only apply to this subset of errors. IMO, it's not worth the complexity of special casing this subset when there are a bunch of other classes of errors that are not.
There was a problem hiding this comment.
Hmm. If there were two independent user errors within a suspense boundary wouldn't you expect to report both of them? the first error isn't necessarily going to imply anything about the second error? The causal link between later errors being ignorable after an initial one is that these later errors are caused by the earlier one but that IMO makes sense only for hydration mismatch errors, not errors in general. Why do you feel like all errors should follow a similar logic?
Is it just that we could choose not to continue rendering and in that case you wouldn't discover the second error until you fixed the first?
There was a problem hiding this comment.
A mismatch earlier doesn't mean that a mismatch later is also a false. It could be two mismatches.
Similarly, a regular error later doesn't mean it's not the same cause as the first error. So you can get spammed by multiple errors from a single cause anyway.
Either just because it's a shared errors but also due to purely ordering like using a technique like the old useID where you use an incrementing counter during render. Any render-impurity really can cause it.
Once imperative code kicks in you'll likely have a bunch of errors following a first one anyway. So the rule is always to look for the first one as the root cause.
React's implementation details is just one of many implementation details that can cause this.
The important rule is that the order is preserved. I.e. first displayed should always be the one that happened first.
If noise is an argument then we should drop all but one error.
There was a problem hiding this comment.
Mainly I care because reducing special case means reducing complexity. Especially this one I fear we'll screw up by making DEV/PROD differ that can be much worse.
There was a problem hiding this comment.
I'm not convinced that all errors are actually equivalent (enough) but I see the argument for simplicity. I'll update to keep only the first error. the complication is that we also use hydrationErrors to queue the descriptive error that reports that the boundary was rendered on client as a fallback. It uses the current hydrationErrors path and I think we would want something like that still so it won't be as simple as just storing the first queued error I think
|
Closing in favor of: #24480 it eliminates the excessive uncaught errors that show up on every render (even ones that do not commit) and does not require further complication of the hydration error logic when we are likely to refactor it singificantly in the future. The consequence of this is if your root commits and there were many hydration errors they will all be reported in onRecoverable error. The key to why this should be ok in general is
|
This PR attempts to make reasonably simple interventions to improve the situation with error logging / reporting for hydration errors without introducing significant new runtime costs. It is clear that the larger refactor of hydration is where the most gains will come from in simplifying the error pathways for hydration.
The general hueristic is