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

[Hydration] Fallback to client render if server rendered extra nodes #23176

Merged
merged 5 commits into from
Feb 1, 2022

Conversation

salazarm
Copy link
Contributor

@salazarm salazarm commented Jan 24, 2022

Summary

In the case of extra nodes being rendered on the server thats technically a mismatch so we should follow the mismatch recovery behavior of doing a client render.

Since we throw from inside completeWork in popHydrationState I had to move popTreeContext and popSuspenseContext so that they happen after it that way we don't double pop when re-entering completeWork from handleError

How did you test this change?

jest

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jan 24, 2022
@sizebot
Copy link

sizebot commented Jan 24, 2022

Comparing: 505c15c...3ae3605

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js +0.17% 129.59 kB 129.81 kB +0.15% 41.55 kB 41.61 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.16% 134.77 kB 134.99 kB +0.13% 43.10 kB 43.15 kB
facebook-www/ReactDOM-prod.classic.js +0.20% 428.19 kB 429.06 kB +0.20% 78.60 kB 78.76 kB
facebook-www/ReactDOM-prod.modern.js +0.21% 418.18 kB 419.05 kB +0.18% 77.17 kB 77.31 kB
facebook-www/ReactDOMForked-prod.classic.js +0.20% 428.19 kB 429.06 kB +0.20% 78.60 kB 78.76 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable-semver/react-reconciler/cjs/react-reconciler.production.min.js +0.21% 91.70 kB 91.89 kB +0.21% 28.25 kB 28.30 kB
oss-stable/react-reconciler/cjs/react-reconciler.production.min.js +0.21% 91.70 kB 91.89 kB +0.21% 28.25 kB 28.30 kB
facebook-www/ReactART-dev.modern.js +0.21% 746.36 kB 747.93 kB +0.20% 159.35 kB 159.67 kB
facebook-www/ReactDOM-prod.modern.js +0.21% 418.18 kB 419.05 kB +0.18% 77.17 kB 77.31 kB
facebook-www/ReactDOMForked-prod.modern.js +0.21% 418.18 kB 419.05 kB +0.18% 77.17 kB 77.31 kB
facebook-www/ReactART-dev.classic.js +0.21% 756.58 kB 758.14 kB +0.19% 161.47 kB 161.78 kB
oss-experimental/react-reconciler/cjs/react-reconciler.production.min.js +0.20% 96.30 kB 96.50 kB +0.18% 29.52 kB 29.58 kB
facebook-www/ReactDOM-prod.classic.js +0.20% 428.19 kB 429.06 kB +0.20% 78.60 kB 78.76 kB
facebook-www/ReactDOMForked-prod.classic.js +0.20% 428.19 kB 429.06 kB +0.20% 78.60 kB 78.76 kB

Generated by 🚫 dangerJS against 3ae3605

@salazarm salazarm force-pushed the alsoThrowOnExtraNodes branch 6 times, most recently from 54aac5c to 2e2efb6 Compare January 27, 2022 15:58
@@ -556,6 +572,16 @@ function popHydrationState(fiber: Fiber): boolean {
return true;
}

function hasUnhydratedTailNodes() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this name but just to add clarification for future readers.

There's a lot of terminology in the code base already that refers to "dehydrated" things. There's a subtle difference between "dehydrated" and "unhydrated". The dehydrated things are in a pending state yet to be hydrated. The unhydrated was ones that failed after hydration. So I think it makes sense to use "unhydrated" here.

return isHydrating && nextHydratableInstance !== null;
}

function warnUnhydratedNextInstance(fiber: Fiber) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The "NextInstance" part is an implementation detail of this file. Maybe go with the other terminology you had. "warnUnhydratedTailNodes"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, one thing though I only warn for the nextHydratableInstance instead of warning for them in a loop because I know the implementation uses a flag to only warn the first time anyways. Though with the error logging API maybe we will want it to there every time so maybe I should add the loop here to warn for all for all of the tail nodes and not just the next one.

Similarly above we throw after the first one is warned for so we don't go through through the warning function for all of the tail nodes there either.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason it logged them in a loop is so that the implementation in the host config could batch them up and build a tree that shows all the errors in one message. Without that the batching it's too noisy. We never landed the PR that did that but we should.

So this should probably call it in a loop too.

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

Nit on the name but looks good other wise.

@salazarm salazarm merged commit 3f5ff16 into facebook:main Feb 1, 2022
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Feb 9, 2022
Summary:
This sync includes the following changes:
- **[a3bde7974](facebook/react@a3bde7974 )**: Exclude react-dom/unstable_testing entry point from stable releases ([#23258](facebook/react#23258)) //<Sebastian Markbåge>//
- **[569093276](facebook/react@569093276 )**: Add onErrorShell Callback ([#23247](facebook/react#23247)) //<Sebastian Markbåge>//
- **[0dedfcc68](facebook/react@0dedfcc68 )**: Update the exports field ([#23257](facebook/react#23257)) //<Sebastian Markbåge>//
- **[9d4e8e84f](facebook/react@9d4e8e84f )**: React Native raw event EventEmitter - intended for app-specific perf listeners and debugging ([#23232](facebook/react#23232)) //<Joshua Gross>//
- **[1dece5235](facebook/react@1dece5235 )**: Add back warning with component stack on Hydration mismatch ([#23241](facebook/react#23241)) //<salazarm>//
- **[cd4eb116c](facebook/react@cd4eb116c )**: Revert "update node.js version for CI ([#23236](facebook/react#23236))" ([#23239](facebook/react#23239)) //<dan>//
- **[1d7728bf9](facebook/react@1d7728bf9 )**: update node.js version for CI ([#23236](facebook/react#23236)) //<sunderls>//
- **[848e802d2](facebook/react@848e802d2 )**: Add onRecoverableError option to hydrateRoot, createRoot ([#23207](facebook/react#23207)) //<Andrew Clark>//
- **[5318971f5](facebook/react@5318971f5 )**: Remove logic for multiple error recovery attempts ([#23227](facebook/react#23227)) //<Andrew Clark>//
- **[3a4462129](facebook/react@3a4462129 )**: Disable avoidThisFallback support in Fizz  ([#23224](facebook/react#23224)) //<salazarm>//
- **[0318ac2c4](facebook/react@0318ac2c4 )**: Revert 4f5449 //<Ricky>//
- **[4f5449eb4](facebook/react@4f5449eb4 )**: Remove main from scheduler `index.js` //<Ricky>//
- **[3f5ff16c1](facebook/react@3f5ff16c1 )**: [Hydration] Fallback to client render if server rendered extra nodes ([#23176](facebook/react#23176)) //<salazarm>//
- **[529dc3ce8](facebook/react@529dc3ce8 )**: Fix context providers in SSR when handling multiple requests ([#23171](facebook/react#23171)) //<Fran Dios>//
- **[505c15c9e](facebook/react@505c15c9e )**: Don't inject timeline hooks unless React supports profiling ([#23151](facebook/react#23151)) //<Brian Vaughn>//
- **[e12a9dfc9](facebook/react@e12a9dfc9 )**: Fix production-only updateSyncExternalStore() crash when doing setState in render ([#23150](facebook/react#23150)) //<Douglas Armstrong>//
- **[e48940255](facebook/react@e48940255 )**: Warn when a callback ref returns a function ([#23145](facebook/react#23145)) //<Dan Abramov>//
- **[d8cfeaf22](facebook/react@d8cfeaf22 )**: Fix context propagation for offscreen/fallback trees ([#23095](facebook/react#23095)) //<Dan Abramov>//
- **[d50482478](facebook/react@d50482478 )**: Enable scheduling profiler flag in react-dom/testing builds ([#23142](facebook/react#23142)) //<Brian Vaughn>//
- **[790b5246f](facebook/react@790b5246f )**: Fix setState ignored in Safari when iframe is added to DOM in the same commit ([#23111](facebook/react#23111)) //<Dan Abramov>//

Changelog:
[General][Changed] - React Native sync for revisions 51947a1...a3bde79

jest_e2e[run_all_tests]

Reviewed By: ShikaSD

Differential Revision: D34108924

fbshipit-source-id: 96acb66c1a7da79d6ef9403490cd0e29ad23d9fb
@salazarm salazarm deleted the alsoThrowOnExtraNodes branch February 10, 2022 14:27
@gurkerl83
Copy link

gurkerl83 commented Feb 28, 2022

Changes in this pull request cause artsy/fresnel#260

@gaearon gaearon mentioned this pull request Mar 29, 2022
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
…acebook#23176)

* rename

* rename

* replace-fork

* rename

* warn in a loop
nevilm-lt pushed a commit to nevilm-lt/react that referenced this pull request Apr 22, 2022
…acebook#23176)

* rename

* rename

* replace-fork

* rename

* warn in a loop
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Feb 13, 2023
Summary:
This sync includes the following changes:
- **[a3bde7974](facebook/react@a3bde7974 )**: Exclude react-dom/unstable_testing entry point from stable releases ([facebook#23258](facebook/react#23258)) //<Sebastian Markbåge>//
- **[569093276](facebook/react@569093276 )**: Add onErrorShell Callback ([facebook#23247](facebook/react#23247)) //<Sebastian Markbåge>//
- **[0dedfcc68](facebook/react@0dedfcc68 )**: Update the exports field ([facebook#23257](facebook/react#23257)) //<Sebastian Markbåge>//
- **[9d4e8e84f](facebook/react@9d4e8e84f )**: React Native raw event EventEmitter - intended for app-specific perf listeners and debugging ([facebook#23232](facebook/react#23232)) //<Joshua Gross>//
- **[1dece5235](facebook/react@1dece5235 )**: Add back warning with component stack on Hydration mismatch ([facebook#23241](facebook/react#23241)) //<salazarm>//
- **[cd4eb116c](facebook/react@cd4eb116c )**: Revert "update node.js version for CI ([facebook#23236](facebook/react#23236))" ([facebook#23239](facebook/react#23239)) //<dan>//
- **[1d7728bf9](facebook/react@1d7728bf9 )**: update node.js version for CI ([facebook#23236](facebook/react#23236)) //<sunderls>//
- **[848e802d2](facebook/react@848e802d2 )**: Add onRecoverableError option to hydrateRoot, createRoot ([facebook#23207](facebook/react#23207)) //<Andrew Clark>//
- **[5318971f5](facebook/react@5318971f5 )**: Remove logic for multiple error recovery attempts ([facebook#23227](facebook/react#23227)) //<Andrew Clark>//
- **[3a4462129](facebook/react@3a4462129 )**: Disable avoidThisFallback support in Fizz  ([facebook#23224](facebook/react#23224)) //<salazarm>//
- **[0318ac2c4](facebook/react@0318ac2c4 )**: Revert 4f5449 //<Ricky>//
- **[4f5449eb4](facebook/react@4f5449eb4 )**: Remove main from scheduler `index.js` //<Ricky>//
- **[3f5ff16c1](facebook/react@3f5ff16c1 )**: [Hydration] Fallback to client render if server rendered extra nodes ([facebook#23176](facebook/react#23176)) //<salazarm>//
- **[529dc3ce8](facebook/react@529dc3ce8 )**: Fix context providers in SSR when handling multiple requests ([facebook#23171](facebook/react#23171)) //<Fran Dios>//
- **[505c15c9e](facebook/react@505c15c9e )**: Don't inject timeline hooks unless React supports profiling ([facebook#23151](facebook/react#23151)) //<Brian Vaughn>//
- **[e12a9dfc9](facebook/react@e12a9dfc9 )**: Fix production-only updateSyncExternalStore() crash when doing setState in render ([facebook#23150](facebook/react#23150)) //<Douglas Armstrong>//
- **[e48940255](facebook/react@e48940255 )**: Warn when a callback ref returns a function ([facebook#23145](facebook/react#23145)) //<Dan Abramov>//
- **[d8cfeaf22](facebook/react@d8cfeaf22 )**: Fix context propagation for offscreen/fallback trees ([facebook#23095](facebook/react#23095)) //<Dan Abramov>//
- **[d50482478](facebook/react@d50482478 )**: Enable scheduling profiler flag in react-dom/testing builds ([facebook#23142](facebook/react#23142)) //<Brian Vaughn>//
- **[790b5246f](facebook/react@790b5246f )**: Fix setState ignored in Safari when iframe is added to DOM in the same commit ([facebook#23111](facebook/react#23111)) //<Dan Abramov>//

Changelog:
[General][Changed] - React Native sync for revisions 51947a1...a3bde79

jest_e2e[run_all_tests]

Reviewed By: ShikaSD

Differential Revision: D34108924

fbshipit-source-id: 96acb66c1a7da79d6ef9403490cd0e29ad23d9fb
sebmarkbage added a commit that referenced this pull request Mar 6, 2024
In #23176 we added a special case in completeWork for SuspenseBoundaries
if they still have trailing children. However, that misses a case
because it doesn't log a recoverable error for the hydration mismatch.
So we get an error that we rerendered.

I think this special case was done to avoid contexts getting out of
sync. I don't know why we didn't just move where the pop happens though
so that's what I did here and let the regular pass throw instead. Seems
to be pass the tests.
github-actions bot pushed a commit that referenced this pull request Mar 6, 2024
In #23176 we added a special case in completeWork for SuspenseBoundaries
if they still have trailing children. However, that misses a case
because it doesn't log a recoverable error for the hydration mismatch.
So we get an error that we rerendered.

I think this special case was done to avoid contexts getting out of
sync. I don't know why we didn't just move where the pop happens though
so that's what I did here and let the regular pass throw instead. Seems
to be pass the tests.

DiffTrain build for [c11b196](c11b196)
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
In facebook#23176 we added a special case in completeWork for SuspenseBoundaries
if they still have trailing children. However, that misses a case
because it doesn't log a recoverable error for the hydration mismatch.
So we get an error that we rerendered.

I think this special case was done to avoid contexts getting out of
sync. I don't know why we didn't just move where the pop happens though
so that's what I did here and let the regular pass throw instead. Seems
to be pass the tests.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
In #23176 we added a special case in completeWork for SuspenseBoundaries
if they still have trailing children. However, that misses a case
because it doesn't log a recoverable error for the hydration mismatch.
So we get an error that we rerendered.

I think this special case was done to avoid contexts getting out of
sync. I don't know why we didn't just move where the pop happens though
so that's what I did here and let the regular pass throw instead. Seems
to be pass the tests.

DiffTrain build for commit c11b196.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants