Repository: facebook/react
Pull request: fix getSnapshot warning when a selector returns NaN

- [x] cla
- [x] lint
- [x] prettier
- [x] test
  (but issue remains)


<!--
  Thanks for submitting a pull request!
  We appreciate you spending the time to work on these changes. Please provide enough information so that others can review your pull request. The three fields below are mandatory.

  Before submitting a pull request, please make sure the following is done:

  1. Fork [the repository](https://github.com/facebook/react) and create your branch from `main`.
  2. Run `yarn` in the repository root.
  3. If you've fixed a bug or added code that should be tested, add tests!
  4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch TestName` is helpful in development.
  5. Run `yarn test --prod` to test in the production environment. It supports the same options as `yarn test`.
  6. If you need a debugger, run `yarn debug-test --watch TestName`, open `chrome://inspect`, and press "Inspect".
  7. Format your code with [prettier](https://github.com/prettier/prettier) (`yarn prettier`).
  8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only check changed files.
  9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
  10. If you haven't already, complete the CLA.

  Learn more about contributing: https://reactjs.org/docs/how-to-contribute.html
-->

## Summary

<!--
 Explain the **motivation** for making this change. What existing problem does the pull request solve?
-->

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.

```typescript
        const selectedNaN = useSyncExternalStoreWithSelector(
          store.subscribe,
          store.getState,
          null,
          s => parseInt(s.nan, 10),
        );
```

## How did you test this change?

<!--
  Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes the user interface.
  How exactly did you verify that your PR solves the issue you wanted to solve?
  If you leave this empty, your PR will very likely be closed.
-->

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!

Changed files:
filename: packages/react-reconciler/src/ReactFiberHooks.new.js
patch: @@ -1290,7 +1290,8 @@ function mountSyncExternalStore<T>(
     nextSnapshot = getSnapshot();
     if (__DEV__) {
       if (!didWarnUncachedGetSnapshot) {
-        if (nextSnapshot !== getSnapshot()) {
+        const cachedSnapshot = getSnapshot();
+        if (!is(nextSnapshot, cachedSnapshot)) {
           console.error(
             'The result of getSnapshot should be cached to avoid an infinite loop',
           );
@@ -1362,7 +1363,8 @@ function updateSyncExternalStore<T>(
   const nextSnapshot = getSnapshot();
   if (__DEV__) {
     if (!didWarnUncachedGetSnapshot) {
-      if (nextSnapshot !== getSnapshot()) {
+      const cachedSnapshot = getSnapshot();
+      if (!is(nextSnapshot, cachedSnapshot)) {
         console.error(
           'The result of getSnapshot should be cached to avoid an infinite loop',
         );

filename: packages/react-reconciler/src/ReactFiberHooks.old.js
patch: @@ -1290,7 +1290,8 @@ function mountSyncExternalStore<T>(
     nextSnapshot = getSnapshot();
     if (__DEV__) {
       if (!didWarnUncachedGetSnapshot) {
-        if (nextSnapshot !== getSnapshot()) {
+        const cachedSnapshot = getSnapshot();
+        if (!is(nextSnapshot, cachedSnapshot)) {
           console.error(
             'The result of getSnapshot should be cached to avoid an infinite loop',
           );
@@ -1362,7 +1363,8 @@ function updateSyncExternalStore<T>(
   const nextSnapshot = getSnapshot();
   if (__DEV__) {
     if (!didWarnUncachedGetSnapshot) {
-      if (nextSnapshot !== getSnapshot()) {
+      const cachedSnapshot = getSnapshot();
+      if (!is(nextSnapshot, cachedSnapshot)) {
         console.error(
           'The result of getSnapshot should be cached to avoid an infinite loop',
         );

filename: packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js
patch: @@ -587,6 +587,33 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {
     );
   });
 
+  test('getSnapshot can return NaN without infinite loop warning', async () => {
+    const store = createExternalStore('not a number');
+
+    function App() {
+      const value = useSyncExternalStore(store.subscribe, () =>
+        parseInt(store.getState(), 10),
+      );
+      return <Text text={value} />;
+    }
+
+    const container = document.createElement('div');
+    const root = createRoot(container);
+
+    // Initial render that reads a snapshot of NaN. This is OK because we use
+    // Object.is algorithm to compare values.
+    await act(() => root.render(<App />));
+    expect(container.textContent).toEqual('NaN');
+
+    // Update to real number
+    await act(() => store.set(123));
+    expect(container.textContent).toEqual('123');
+
+    // Update back to NaN
+    await act(() => store.set('not a number'));
+    expect(container.textContent).toEqual('NaN');
+  });
+
   describe('extra features implemented in user-space', () => {
     // The selector implementation uses the lazy ref initialization pattern
     // @gate !(enableUseRefAccessWarning && __DEV__)

filename: packages/use-sync-external-store/src/useSyncExternalStoreShimClient.js
patch: @@ -57,7 +57,8 @@ export function useSyncExternalStore<T>(
   const value = getSnapshot();
   if (__DEV__) {
     if (!didWarnUncachedGetSnapshot) {
-      if (value !== getSnapshot()) {
+      const cachedValue = getSnapshot();
+      if (!is(value, cachedValue)) {
         console.error(
           'The result of getSnapshot should be cached to avoid an infinite loop',
         );