Add test case for hydration warning suppression #24436
Conversation
In facebook#24404 a test case was added for supressing hydration warnings if a preceding component suspends. The test case showed that hydration warnings would still emit after resolution but it did not include a test case for when the resolution led to a complete hydration without errors. This commit adds the additional test case demonstrating that no warnings or Errors are observed if hydration completes successfully after suspended component resolution
|
Comparing: bd4784c...06b7825 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) |
|
Mh weird that the test is not failing? But I still see errors here: |
|
Yes, I'm also concerned the test doesn't fail since the behavior on main is not supposed to fix it yet |
|
@xiel I'm having trouble forking your sandbox for some reason. it tells me it cannot install the react versions in package.json becuase they are missing form the registry but clearly they are not because you are using them. I'm not sandbox expert, got any idea why a simple fork shouldn't work (plus @gaearon) As for the difference in behavior between test and this sandbox I believe there is something causing the root to complete even though the app is still suspended so the mismatch errors which were queued end up being logged. The test case (as constructed) does not commit the root when the boundary is in fallback and so the queued errors get dropped. The unmerged change in #24427 would suppress these. I don't think the test is wrong per se but perhaps there is another test that needs to recreate the conditions observed in this sandbox which I don't yet know how to do. will look more into it when I touch base with @acdlite later this week |
|
Closing to combine with #24427 |
|
@xiel the reason those errors are showing up has to do with how React makes dev-time errors "uncaught" for debugging purposes. When you run in dev mode and there is an error rendering a component a thing called We're going to change this over in #24427 The other issue your sandbox demonstrates is a rapid retry of the suspense boundary leading to a flood of errors. This is a separate issue I'll tackle next but essentially work is getting scheduled on the suspended component on each mousemove but it should instead wait if we're blocked on that boundary. |
|
@xiel and here is a modified codesandbox showing the errors are gone but the over-eager render retries still present: https://codesandbox.io/s/trusting-buck-2j21to?file=/src/index.js |
To be clear, I think that’s the reason why we have these invokeGuardedCallback wrappers at all. That is, they are explicitly for that purpose — to get the browser to report the error as uncaught despite us actually catching it. But yeah, in this case we wouldn’t want that. |
Yeah I think I was looking at it myopically and thinking it was about breaking on uncaught exceptions. makes sense that you would also want to surface in console when you don't necessarily know you need to be debugging. appreciate the callout |
In #24404 a test case was added for supressing hydration warnings if a preceding component suspends. The test case showed that hydration warnings would still emit after resolution but it did not include a test case for when the resolution led to a complete hydration without errors. This commit adds the additional test case demonstrating that no warnings or Errors are observed if hydration completes successfully after suspended component resolution