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

Move hydration code out of normal Suspense path #24532

Merged
merged 2 commits into from May 25, 2022

Conversation

acdlite
Copy link
Member

@acdlite acdlite commented May 10, 2022

Shuffling some code around to make it easier to follow. The logic for updating a dehydrated Suspense boundary is significantly different from the logic for a client-rendered Suspense boundary. Most of it was already lifted out into a separate function; this moves the remaining hydration-specific logic out of updateSuspenseComponent and into updateDehydratedSuspenseComponent instead.

No expected changes to program behavior.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team labels May 10, 2022
@sizebot
Copy link

sizebot commented May 10, 2022

Comparing: 2c68776...01562b7

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 = 131.49 kB 131.28 kB = 42.15 kB 42.13 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 136.75 kB 136.54 kB = 43.70 kB 43.68 kB
facebook-www/ReactDOM-prod.classic.js = 441.13 kB 439.35 kB = 80.41 kB 80.29 kB
facebook-www/ReactDOM-prod.modern.js = 426.42 kB 424.64 kB = 78.24 kB 78.13 kB
facebook-www/ReactDOMForked-prod.classic.js = 441.13 kB 439.35 kB = 80.42 kB 80.29 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-reconciler/cjs/react-reconciler.production.min.js = 97.50 kB 97.30 kB +0.04% 29.89 kB 29.90 kB
oss-stable/react-reconciler/cjs/react-reconciler.production.min.js = 92.65 kB 92.45 kB = 28.50 kB 28.50 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.production.min.js = 92.63 kB 92.43 kB = 28.48 kB 28.48 kB
oss-experimental/react-test-renderer/umd/react-test-renderer.production.min.js = 91.93 kB 91.73 kB +0.14% 28.61 kB 28.65 kB
oss-experimental/react-test-renderer/cjs/react-test-renderer.production.min.js = 91.65 kB 91.45 kB +0.04% 28.26 kB 28.27 kB
oss-stable/react-test-renderer/umd/react-test-renderer.production.min.js = 87.50 kB 87.30 kB +0.06% 27.30 kB 27.31 kB
oss-stable-semver/react-test-renderer/umd/react-test-renderer.production.min.js = 87.48 kB 87.28 kB +0.06% 27.30 kB 27.31 kB
oss-stable/react-test-renderer/cjs/react-test-renderer.production.min.js = 87.22 kB 87.02 kB +0.05% 26.97 kB 26.98 kB
oss-stable-semver/react-test-renderer/cjs/react-test-renderer.production.min.js = 87.20 kB 87.00 kB +0.05% 26.97 kB 26.98 kB
oss-experimental/react-art/cjs/react-art.production.min.js = 86.52 kB 86.32 kB = 26.77 kB 26.76 kB
oss-stable/react-art/cjs/react-art.production.min.js = 81.75 kB 81.55 kB +0.05% 25.37 kB 25.38 kB
oss-stable-semver/react-art/cjs/react-art.production.min.js = 81.72 kB 81.52 kB +0.10% 25.36 kB 25.38 kB
facebook-www/ReactDOM-profiling.classic.js = 471.82 kB 470.16 kB = 85.01 kB 84.87 kB
facebook-www/ReactDOMForked-profiling.classic.js = 471.82 kB 470.16 kB = 85.01 kB 84.88 kB
facebook-www/ReactDOM-profiling.modern.js = 457.05 kB 455.38 kB = 82.71 kB 82.59 kB
facebook-www/ReactDOMForked-profiling.modern.js = 457.05 kB 455.38 kB = 82.72 kB 82.60 kB
facebook-www/ReactDOMTesting-prod.classic.js = 432.94 kB 431.24 kB = 80.56 kB 80.46 kB
facebook-www/ReactDOM-prod.classic.js = 441.13 kB 439.35 kB = 80.41 kB 80.29 kB
facebook-www/ReactDOMForked-prod.classic.js = 441.13 kB 439.35 kB = 80.42 kB 80.29 kB
facebook-www/ReactDOMTesting-prod.modern.js = 416.83 kB 415.13 kB = 78.10 kB 78.00 kB
facebook-www/ReactDOM-prod.modern.js = 426.42 kB 424.64 kB = 78.24 kB 78.13 kB
facebook-www/ReactDOMForked-prod.modern.js = 426.42 kB 424.64 kB = 78.25 kB 78.14 kB
react-native/implementations/ReactNativeRenderer-profiling.fb.js = 331.07 kB 329.24 kB = 58.95 kB 58.83 kB
react-native/implementations/ReactFabric-profiling.fb.js = 323.90 kB 322.09 kB = 57.90 kB 57.78 kB
react-native/implementations/ReactNativeRenderer-profiling.js = 315.02 kB 313.18 kB = 56.29 kB 56.15 kB
react-native/implementations/ReactFabric-profiling.js = 307.90 kB 306.07 kB = 55.28 kB 55.12 kB
react-native/implementations/ReactNativeRenderer-prod.fb.js = 303.92 kB 302.06 kB = 54.71 kB 54.60 kB
react-native/implementations/ReactFabric-prod.fb.js = 296.83 kB 294.97 kB = 53.63 kB 53.52 kB
facebook-www/ReactART-prod.classic.js = 280.34 kB 278.58 kB = 49.56 kB 49.42 kB
react-native/implementations/ReactNativeRenderer-prod.js = 295.80 kB 293.93 kB = 53.14 kB 53.00 kB
react-native/implementations/ReactFabric-prod.js = 288.76 kB 286.89 kB = 52.06 kB 51.92 kB
facebook-www/ReactART-prod.modern.js = 269.52 kB 267.76 kB = 47.81 kB 47.67 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-profiling.js = 274.43 kB 272.57 kB = 49.37 kB 49.20 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-prod.js = 259.28 kB 257.50 kB = 47.13 kB 46.96 kB

Generated by 🚫 dangerJS against 01562b7

@acdlite acdlite force-pushed the move-hydration-code branch 2 times, most recently from 3d0367e to f7c9bf2 Compare May 20, 2022
gnoff
gnoff approved these changes May 25, 2022
Copy link
Collaborator

@gnoff gnoff left a comment

Makes sense and fixed described in #24578

acdlite added 2 commits May 25, 2022
Shuffling some code around to make it easier to follow. The logic for
updating a dehydrated Suspense boundary is significantly different
from the logic for a client-rendered Suspense boundary. Most of it was
already lifted out into a separate function; this moves the remaining
hydration-specific logic out of updateSuspenseComponent and into
updateDehydratedSuspenseComponent instead.

No expected changes to program behavior.
Same as previous step but for the complete phase. This is a separate
commit to make bisecting easier in case something breaks. The logic
is very subtle but mostly all I've done is extract it to
another function.
@acdlite acdlite merged commit b2763d3 into facebook:main May 25, 2022
35 of 36 checks passed
@sebmarkbage
Copy link
Collaborator

sebmarkbage commented May 25, 2022

If this fixes something, then there's a change to behavior. What's the change?

@gnoff
Copy link
Collaborator

gnoff commented May 25, 2022

hydration errors from an inner boundary can be upgraded to recoverable by an outer boundary. This moves the upgrade call to the code path that only runs for the boundary that is being hydrated

@acdlite
Copy link
Member Author

acdlite commented May 25, 2022

If this fixes something, then there's a change to behavior. What's the change?

This upgradeHydrationErrorsToRecoverable call is only meant to be called when going from dehydrated -> not dehydrated. Which was just a mistake in the old code that my refactoring happened to fix. I didn't realize this when I said "no expected changes" but as you can see I hedged with "expected" :)

https://github.com/acdlite/react/blob/01562b73ccd3eefdc5c0f3ebf028138576c6d9bf/packages/react-reconciler/src/ReactFiberCompleteWork.new.js#L838-L842

@acdlite
Copy link
Member Author

acdlite commented May 25, 2022

I didn't add a test because @gnoff already had one, here: #24618

facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Jun 6, 2022
Summary:
This sync includes the following changes:
- **[dd4950c90](facebook/react@dd4950c90 )**: [Flight] Implement useId hook ([#24172](facebook/react#24172)) //<Josh Story>//
- **[26a5b3c7f](facebook/react@26a5b3c7f )**: Explicitly set `highWaterMark` to 0 for `ReadableStream` ([#24641](facebook/react#24641)) //<Josh Larson>//
- **[aec575914](facebook/react@aec575914 )**: [Fizz] Send errors down to client ([#24551](facebook/react#24551)) //<Josh Story>//
- **[a2766387e](facebook/react@a2766387e )**: [Fizz] Improve text separator byte efficiency ([#24630](facebook/react#24630)) //<Josh Story>//
- **[f7860538a](facebook/react@f7860538a )**: Fix typo in useSyncExternalStore main entry point error ([#24631](facebook/react#24631)) //<François Chalifour>//
- **[1bed20731](facebook/react@1bed20731 )**: Add a module map option to the Webpack Flight Client ([#24629](facebook/react#24629)) //<Sebastian Markbåge>//
- **[b2763d3ea](facebook/react@b2763d3ea )**: Move hydration code out of normal Suspense path ([#24532](facebook/react#24532)) //<Andrew Clark>//
- **[357a61324](facebook/react@357a61324 )**: [DevTools][Transition Tracing] Added support for Suspense Boundaries ([#23365](facebook/react#23365)) //<Luna Ruan>//
- **[2c8a1452b](facebook/react@2c8a1452b )**: Fix ignored setState in Safari when iframe is touched ([#24459](facebook/react#24459)) //<dan>//
- **[62662633d](facebook/react@62662633d )**: Remove enableFlipOffscreenUnhideOrder ([#24545](facebook/react#24545)) //<Ricky>//
- **[34da5aa69](facebook/react@34da5aa69 )**: Only treat updates to lazy as a new mount in legacy mode ([#24530](facebook/react#24530)) //<Ricky>//
- **[46a6d77e3](facebook/react@46a6d77e3 )**: Unify JSResourceReference Interfaces ([#24507](facebook/react#24507)) //<Timothy Yung>//
- **[6cbf0f7fa](facebook/react@6cbf0f7fa )**: Fork ReactSymbols ([#24484](facebook/react#24484)) //<Ricky>//
- **[a10a9a6b5](facebook/react@a10a9a6b5 )**: Add test for hiding children after layout destroy ([#24483](facebook/react#24483)) //<Ricky>//
- **[b4eb0ad71](facebook/react@b4eb0ad71 )**: Do not replay erroring beginWork with invokeGuardedCallback when suspended or previously errored ([#24480](facebook/react#24480)) //<Josh Story>//
- **[99eef9e2d](facebook/react@99eef9e2d )**: Hide children of Offscreen after destroy effects ([#24446](facebook/react#24446)) //<Ricky>//
- **[ce1386028](facebook/react@ce1386028 )**: Remove enablePersistentOffscreenHostContainer flag ([#24460](facebook/react#24460)) //<Andrew Clark>//
- **[72b7462fe](facebook/react@72b7462fe )**: Bump local package.json versions for 18.1 release ([#24447](facebook/react#24447)) //<Andrew Clark>//
- **[22edb9f77](facebook/react@22edb9f77 )**: React `version` field should match package.json ([#24445](facebook/react#24445)) //<Andrew Clark>//
- **[6bf3deef5](facebook/react@6bf3deef5 )**: Upgrade react-shallow-renderer to support react 18 ([#24442](facebook/react#24442)) //<Michael サイトー 中村 Bashurov>//

Changelog:
[General][Changed] - React Native sync for revisions bd4784c...d300ceb

jest_e2e[run_all_tests]

Reviewed By: cortinico, kacieb

Differential Revision: D36874368

fbshipit-source-id: c0ee015f4ef2fa56e57f7a1f6bc37dd05c949877
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants