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

Implement getServerSnapshot in userspace shim #22359

Merged
merged 2 commits into from
Sep 20, 2021

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Sep 19, 2021

If the DOM is not present, we assume that we are running in a server environment and return the result of getServerSnapshot.

This heuristic doesn't work in React Native, so we'll need to provide a separate native build (using the .native extension). I've left this for a follow-up.

We can't call getServerSnapshot on the client, because in versions of React before 18, there's no built-in mechanism to detect whether we're hydrating. To avoid a server mismatch warning, users must account for this themselves and return the correct value inside getSnapshot.

Note that none of this is relevant to the built-in API that is being added in 18. This only affects the userspace shim that is provided for backwards compatibility with versions 16 and 17.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Sep 19, 2021
@sizebot
Copy link

sizebot commented Sep 19, 2021

Comparing: 86b3e24...c496f7c

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 = 127.43 kB 127.43 kB = 40.61 kB 40.61 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 130.26 kB 130.26 kB = 41.53 kB 41.53 kB
facebook-www/ReactDOM-prod.classic.js = 405.48 kB 405.48 kB = 75.07 kB 75.07 kB
facebook-www/ReactDOM-prod.modern.js = 394.07 kB 394.07 kB = 73.36 kB 73.36 kB
facebook-www/ReactDOMForked-prod.classic.js = 405.48 kB 405.48 kB = 75.07 kB 75.07 kB
oss-experimental/use-sync-external-store/cjs/use-sync-external-store.production.min.js +57.29% 0.89 kB 1.39 kB +51.04% 0.53 kB 0.80 kB
oss-stable-semver/use-sync-external-store/cjs/use-sync-external-store.production.min.js +57.29% 0.89 kB 1.39 kB +51.04% 0.53 kB 0.80 kB
oss-stable/use-sync-external-store/cjs/use-sync-external-store.production.min.js +57.29% 0.89 kB 1.39 kB +51.04% 0.53 kB 0.80 kB
oss-experimental/use-sync-external-store/cjs/use-sync-external-store.development.js +11.04% 7.23 kB 8.02 kB +8.83% 2.97 kB 3.23 kB
oss-stable-semver/use-sync-external-store/cjs/use-sync-external-store.development.js +11.04% 7.23 kB 8.02 kB +8.83% 2.97 kB 3.23 kB
oss-stable/use-sync-external-store/cjs/use-sync-external-store.development.js +11.04% 7.23 kB 8.02 kB +8.83% 2.97 kB 3.23 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/use-sync-external-store/cjs/use-sync-external-store.production.min.js +57.29% 0.89 kB 1.39 kB +51.04% 0.53 kB 0.80 kB
oss-stable-semver/use-sync-external-store/cjs/use-sync-external-store.production.min.js +57.29% 0.89 kB 1.39 kB +51.04% 0.53 kB 0.80 kB
oss-stable/use-sync-external-store/cjs/use-sync-external-store.production.min.js +57.29% 0.89 kB 1.39 kB +51.04% 0.53 kB 0.80 kB
oss-experimental/use-sync-external-store/cjs/use-sync-external-store.development.js +11.04% 7.23 kB 8.02 kB +8.83% 2.97 kB 3.23 kB
oss-stable-semver/use-sync-external-store/cjs/use-sync-external-store.development.js +11.04% 7.23 kB 8.02 kB +8.83% 2.97 kB 3.23 kB
oss-stable/use-sync-external-store/cjs/use-sync-external-store.development.js +11.04% 7.23 kB 8.02 kB +8.83% 2.97 kB 3.23 kB

Generated by 🚫 dangerJS against c496f7c

jest.mock('react', () => {
const {
// eslint-disable-next-line no-unused-vars
startTransition: _,
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious why do you need to remove these fields for the test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's to simulate a build of React 17, which includes neither useSyncExternalStore nor startTransition.

The reason startTransition is relevant is because I use it as a heuristic to check whether you're using an outdated alpha of React 18: https://github.com/facebook/react/blob/main/packages/use-sync-external-store/src/useSyncExternalStore.js#L49-L61

The eventual plan is to load a copy of React 17 from npm and test the shim using that.

Copy link
Contributor

@salazarm salazarm left a comment

Choose a reason for hiding this comment

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

good stuff

Idea is that eventually we'll run these tests against an actual build of
React DOM 17 to test backwards compatibility.
If the DOM is not present, we assume that we are running in a server
environment and return the result of `getServerSnapshot`.

This heuristic doesn't work in React Native, so we'll need to provide
a separate native build (using the `.native` extension). I've left this
for a follow-up.

We can't call `getServerSnapshot` on the client, because in versions of
React before 18, there's no built-in mechanism to detect whether we're
hydrating. To avoid a server mismatch warning, users must account for
this themselves and return the correct value inside `getSnapshot`.

Note that none of this is relevant to the built-in API that is being
added in 18. This only affects the userspace shim that is provided
for backwards compatibility with versions 16 and 17.
@acdlite acdlite merged commit 79b8fc6 into facebook:main Sep 20, 2021
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Oct 12, 2021
Summary:
This sync includes the following changes:
- **[579c008a7](facebook/react@579c008a7 )**: [Fizz/Flight] pipeToNodeWritable(..., writable).startWriting() -> renderToPipeableStream(...).pipe(writable) ([#22450](facebook/react#22450)) //<Sebastian Markbåge>//
- **[f2c381131](facebook/react@f2c381131 )**: fix: useSyncExternalStoreExtra ([#22500](facebook/react#22500)) //<Daishi Kato>//
- **[0ecbbe142](facebook/react@0ecbbe142 )**: Sync hydrate discrete events in capture phase and dont replay discrete events ([#22448](facebook/react#22448)) //<salazarm>//
- **[a724a3b57](facebook/react@a724a3b57 )**: [RFC] Codemod invariant -> throw new Error ([#22435](facebook/react#22435)) //<Andrew Clark>//
- **[201af81b0](facebook/react@201af81b0 )**: Release pooled cache reference in complete/unwind ([#22464](facebook/react#22464)) //<Joseph Savona>//
- **[033efe731](facebook/react@033efe731 )**: Call get snapshot in useSyncExternalStore server shim ([#22453](facebook/react#22453)) //<salazarm>//
- **[7843b142a](facebook/react@7843b142a )**: [Fizz/Flight] Pass in Destination lazily to startFlowing instead of in createRequest ([#22449](facebook/react#22449)) //<Sebastian Markbåge>//
- **[d9fb383d6](facebook/react@d9fb383d6 )**: Extract queueing logic into shared functions ([#22452](facebook/react#22452)) //<Andrew Clark>//
- **[9175f4d15](facebook/react@9175f4d15 )**: Scheduling Profiler: Show Suspense resource .displayName ([#22451](facebook/react#22451)) //<Brian Vaughn>//
- **[eba248c39](facebook/react@eba248c39 )**: [Fizz/Flight] Remove reentrancy hack ([#22446](facebook/react#22446)) //<Sebastian Markbåge>//
- **[66388150e](facebook/react@66388150e )**: Remove usereducer eager bailout ([#22445](facebook/react#22445)) //<Joseph Savona>//
- **[d3e086932](facebook/react@d3e086932 )**: Make root.unmount() synchronous  ([#22444](facebook/react#22444)) //<Andrew Clark>//
- **[2cc6d79c9](facebook/react@2cc6d79c9 )**: Rename onReadyToStream to onCompleteShell ([#22443](facebook/react#22443)) //<Sebastian Markbåge>//
- **[c88fb49d3](facebook/react@c88fb49d3 )**: Improve DEV errors if string coercion throws (Temporal.*, Symbol, etc.) ([#22064](facebook/react#22064)) //<Justin Grant>//
- **[05726d72c](facebook/react@05726d72c )**: [Fix] Errors should not "unsuspend" a transition ([#22423](facebook/react#22423)) //<Andrew Clark>//
- **[3746eaf98](facebook/react@3746eaf98 )**: Packages/React/src/ReactLazy ---> changing -1 to unintialized ([#22421](facebook/react#22421)) //<BIKI DAS>//
- **[04ccc01d9](facebook/react@04ccc01d9 )**: Hydration errors should force a client render ([#22416](facebook/react#22416)) //<Andrew Clark>//
- **[029fdcebb](facebook/react@029fdcebb )**: root.hydrate -> root.isDehydrated ([#22420](facebook/react#22420)) //<Andrew Clark>//
- **[af87f5a83](facebook/react@af87f5a83 )**: Scheduling Profiler marks should include thrown Errors ([#22417](facebook/react#22417)) //<Brian Vaughn>//
- **[d47339ea3](facebook/react@d47339ea3 )**: [Fizz] Remove assignID mechanism ([#22410](facebook/react#22410)) //<Sebastian Markbåge>//
- **[3a50d9557](facebook/react@3a50d9557 )**: Never attach ping listeners in legacy Suspense ([#22407](facebook/react#22407)) //<Andrew Clark>//
- **[82c8fa90b](facebook/react@82c8fa90b )**: Add back useMutableSource temporarily ([#22396](facebook/react#22396)) //<Andrew Clark>//
- **[5b57bc6e3](facebook/react@5b57bc6e3 )**: [Draft] don't patch console during first render ([#22308](facebook/react#22308)) //<Luna Ruan>//
- **[cf07c3df1](facebook/react@cf07c3df1 )**: Delete all but one `build2` reference ([#22391](facebook/react#22391)) //<Andrew Clark>//
- **[bb0d06935](facebook/react@bb0d06935 )**: [build2 -> build] Local scripts //<Andrew Clark>//
- **[0c81d347b](facebook/react@0c81d347b )**: Write artifacts to `build` instead of `build2` //<Andrew Clark>//
- **[4da03c9fb](facebook/react@4da03c9fb )**: useSyncExternalStore React Native version ([#22367](facebook/react#22367)) //<salazarm>//
- **[48d475c9e](facebook/react@48d475c9e )**: correct typos ([#22294](facebook/react#22294)) //<Bowen>//
- **[cb6c619c0](facebook/react@cb6c619c0 )**: Remove Fiber fields that were used for hydrating useMutableSource ([#22368](facebook/react#22368)) //<Sebastian Silbermann>//
- **[64e70f82e](facebook/react@64e70f82e )**: [Fizz] add avoidThisFallback support ([#22318](facebook/react#22318)) //<salazarm>//
- **[3ee7a004e](facebook/react@3ee7a004e )**: devtools: Display actual ReactDOM API name in root type ([#22363](facebook/react#22363)) //<Sebastian Silbermann>//
- **[79b8fc667](facebook/react@79b8fc667 )**: Implement getServerSnapshot in userspace shim ([#22359](facebook/react#22359)) //<Andrew Clark>//
- **[86b3e2461](facebook/react@86b3e2461 )**: Implement useSyncExternalStore on server ([#22347](facebook/react#22347)) //<Andrew Clark>//
- **[8209de269](facebook/react@8209de269 )**: Delete useMutableSource implementation ([#22292](facebook/react#22292)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions e8feb11...afcb9cd

jest_e2e[run_all_tests]

Reviewed By: yungsters

Differential Revision: D31541359

fbshipit-source-id: c35941bc303fdf55cb061e9996200dc868a6f2af
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
* Convert useSES shim tests to use React DOM

Idea is that eventually we'll run these tests against an actual build of
React DOM 17 to test backwards compatibility.

* Implement getServerSnapshot in userspace shim

If the DOM is not present, we assume that we are running in a server
environment and return the result of `getServerSnapshot`.

This heuristic doesn't work in React Native, so we'll need to provide
a separate native build (using the `.native` extension). I've left this
for a follow-up.

We can't call `getServerSnapshot` on the client, because in versions of
React before 18, there's no built-in mechanism to detect whether we're
hydrating. To avoid a server mismatch warning, users must account for
this themselves and return the correct value inside `getSnapshot`.

Note that none of this is relevant to the built-in API that is being
added in 18. This only affects the userspace shim that is provided
for backwards compatibility with versions 16 and 17.
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.

4 participants