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

fix getSnapshot warning when a selector returns NaN #23333

Conversation

hachibeeDI
Copy link
Contributor

  • cla
  • lint
  • prettier
  • test
    (but issue remains)

Summary

When I use a useSyncExternalStoreWithSelector with selecter which possibly returns NaN, I received a warning says "The result of getSnapshot should be cached to avoid an infinite loop" despite it was cached.

        const selectedNaN = useSyncExternalStoreWithSelector(
          store.subscribe,
          store.getState,
          null,
          s => parseInt(s.nan, 10),
        );

How did you test this change?

I added a unit test to src/packges/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js.

One more thing I noticed

While I was testing the code, the unit test doesn't use the functions declared in useSyncExternalStoreShimClient.js but packages/react-reconciler/src/ReactFiberHooks.new.js.

I guess it was due to that the shim could be bypassed if the React has a native version of the function.
I fixed both but a former does not have any unit test as it was (I checked it in my local env).

Thanks!

useSyncExternalStoreWithSelector delegate a selector as
getSnapshot of useSyncExternalStore.
@sizebot
Copy link

sizebot commented Feb 21, 2022

Comparing: 40eaa22...35a7af8

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 = 130.97 kB 130.97 kB = 41.94 kB 41.94 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 135.84 kB 135.84 kB = 43.38 kB 43.38 kB
facebook-www/ReactDOM-prod.classic.js = 432.04 kB 432.04 kB = 79.17 kB 79.17 kB
facebook-www/ReactDOM-prod.modern.js = 421.81 kB 421.81 kB = 77.70 kB 77.70 kB
facebook-www/ReactDOMForked-prod.classic.js = 432.04 kB 432.04 kB = 79.17 kB 79.17 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-shim.native.development.js +0.59% 7.79 kB 7.83 kB +0.26% 3.08 kB 3.09 kB
oss-stable-semver/use-sync-external-store/cjs/use-sync-external-store-shim.native.development.js +0.59% 7.79 kB 7.83 kB +0.26% 3.08 kB 3.09 kB
oss-stable/use-sync-external-store/cjs/use-sync-external-store-shim.native.development.js +0.59% 7.79 kB 7.83 kB +0.26% 3.08 kB 3.09 kB
oss-experimental/use-sync-external-store/cjs/use-sync-external-store-shim.development.js +0.55% 8.37 kB 8.42 kB +0.28% 3.18 kB 3.19 kB
oss-stable-semver/use-sync-external-store/cjs/use-sync-external-store-shim.development.js +0.55% 8.37 kB 8.42 kB +0.28% 3.18 kB 3.19 kB
oss-stable/use-sync-external-store/cjs/use-sync-external-store-shim.development.js +0.55% 8.37 kB 8.42 kB +0.28% 3.18 kB 3.19 kB

Generated by 🚫 dangerJS against 35a7af8

We use Object.is to check whether the snapshot value has been updated,
so we should also use it to check whether the value is cached.
@acdlite
Copy link
Collaborator

acdlite commented Feb 21, 2022

While I was testing the code, the unit test doesn't use the functions declared in useSyncExternalStoreShimClient.js but packages/react-reconciler/src/ReactFiberHooks.new.js.

I guess it was due to that the shim could be bypassed if the React has a native version of the function.
I fixed both but a former does not have any unit test as it was (I checked it in my local env).

We have a feature flag system that runs the tests in different configurations. You can test the shimmed version by passing the --no-variant option to the test command:

# Runs using built-in useSyncExternalStore implementation
yarn test

# Runs using shimmed implementation
yarn test --no-variant

Both variants run automatically in CI.

Your changes look good. (I pushed some minor nits.) Thanks for submitting your first React PR!

@acdlite acdlite merged commit 4de99b3 into facebook:main Feb 21, 2022
@hachibeeDI
Copy link
Contributor Author

Thanks!

@hachibeeDI hachibeeDI deleted the fix-package/use-sync-external-store-with-selector-warning-if-returns-NaN branch February 22, 2022 01:21
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Feb 24, 2022
Summary:
This sync includes the following changes:
- **[4de99b3ca](facebook/react@4de99b3ca )**: fix getSnapshot warning when a selector returns NaN ([#23333](facebook/react#23333)) //<OGURA Daiki>//
- **[40eaa22d9](facebook/react@40eaa22d9 )**: Remove dependency on Offscreen Fiber updateQueue for React Cache ([#23229](facebook/react#23229)) //<Luna Ruan>//
- **[caf6d4707](facebook/react@caf6d4707 )**: Enable enableCache on Test Renderer native ([#23314](facebook/react#23314)) //<David McCabe>//
- **[419ccc2b1](facebook/react@419ccc2b1 )**: Land skipUnmountedBoundaries experiment ([#23322](facebook/react#23322)) //<Andrew Clark>//
- **[54f785bc5](facebook/react@54f785bc5 )**: Disallow comments as DOM containers for createRoot ([#23321](facebook/react#23321)) //<Andrew Clark>//
- **[e9aa9592c](facebook/react@e9aa9592c )**: change ReactBatchConfig.transition //<Luna Ruan>//
- **[51c8411d9](facebook/react@51c8411d9 )**: Log a recoverable error whenever hydration fails ([#23319](facebook/react#23319)) //<Andrew Clark>//
- **[79ed5e18f](facebook/react@79ed5e18f )**: Delete vestigial RetryAfterError logic ([#23312](facebook/react#23312)) //<Andrew Clark>//
- **[80059bb73](facebook/react@80059bb73 )**: Switch to client rendering if root receives update ([#23309](facebook/react#23309)) //<Andrew Clark>//
- **[f7f7ed089](facebook/react@f7f7ed089 )**: Allow suspending in the shell during hydration ([#23304](facebook/react#23304)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions 27b5699...4de99b3

jest_e2e[run_all_tests]

Reviewed By: rickhanlonii

Differential Revision: D34399162

fbshipit-source-id: 5c49e2bdcf63eb6a601cfa6a4e4b8f2e1f83e2dd
@gaearon gaearon mentioned this pull request Mar 29, 2022
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
* fix getSnapshot warning when a selector returns NaN

useSyncExternalStoreWithSelector delegate a selector as
getSnapshot of useSyncExternalStore.

* Fiber's use sync external store has a same issue

* Small nits

We use Object.is to check whether the snapshot value has been updated,
so we should also use it to check whether the value is cached.

Co-authored-by: Andrew Clark <[email protected]>
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
Summary:
This sync includes the following changes:
- **[4de99b3ca](facebook/react@4de99b3ca )**: fix getSnapshot warning when a selector returns NaN ([facebook#23333](facebook/react#23333)) //<OGURA Daiki>//
- **[40eaa22d9](facebook/react@40eaa22d9 )**: Remove dependency on Offscreen Fiber updateQueue for React Cache ([facebook#23229](facebook/react#23229)) //<Luna Ruan>//
- **[caf6d4707](facebook/react@caf6d4707 )**: Enable enableCache on Test Renderer native ([facebook#23314](facebook/react#23314)) //<David McCabe>//
- **[419ccc2b1](facebook/react@419ccc2b1 )**: Land skipUnmountedBoundaries experiment ([facebook#23322](facebook/react#23322)) //<Andrew Clark>//
- **[54f785bc5](facebook/react@54f785bc5 )**: Disallow comments as DOM containers for createRoot ([facebook#23321](facebook/react#23321)) //<Andrew Clark>//
- **[e9aa9592c](facebook/react@e9aa9592c )**: change ReactBatchConfig.transition //<Luna Ruan>//
- **[51c8411d9](facebook/react@51c8411d9 )**: Log a recoverable error whenever hydration fails ([facebook#23319](facebook/react#23319)) //<Andrew Clark>//
- **[79ed5e18f](facebook/react@79ed5e18f )**: Delete vestigial RetryAfterError logic ([facebook#23312](facebook/react#23312)) //<Andrew Clark>//
- **[80059bb73](facebook/react@80059bb73 )**: Switch to client rendering if root receives update ([facebook#23309](facebook/react#23309)) //<Andrew Clark>//
- **[f7f7ed089](facebook/react@f7f7ed089 )**: Allow suspending in the shell during hydration ([facebook#23304](facebook/react#23304)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions 27b5699...4de99b3

jest_e2e[run_all_tests]

Reviewed By: rickhanlonii

Differential Revision: D34399162

fbshipit-source-id: 5c49e2bdcf63eb6a601cfa6a4e4b8f2e1f83e2dd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants