From 4de99b3ca689e8f995f90f199cd8d7bfb4b35eca Mon Sep 17 00:00:00 2001
From: OGURA Daiki <8hachibee125@gmail.com>
Date: Mon, 21 Feb 2022 15:07:41 +0900
Subject: [PATCH] fix getSnapshot warning when a selector returns NaN (#23333)

* 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 <git@andrewclark.io>
---
 .../src/ReactFiberHooks.new.js                |  6 +++--
 .../src/ReactFiberHooks.old.js                |  6 +++--
 .../useSyncExternalStoreShared-test.js        | 27 +++++++++++++++++++
 .../src/useSyncExternalStoreShimClient.js     |  3 ++-
 4 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js
index 6abe576b58fa1..9e4f954b1e88e 100644
--- a/packages/react-reconciler/src/ReactFiberHooks.new.js
+++ b/packages/react-reconciler/src/ReactFiberHooks.new.js
@@ -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',
         );
diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js
index cbac3348831fd..becadf35bd88f 100644
--- a/packages/react-reconciler/src/ReactFiberHooks.old.js
+++ b/packages/react-reconciler/src/ReactFiberHooks.old.js
@@ -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',
         );
diff --git a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js
index cb3a40aa13ab7..2a628990049b6 100644
--- a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js
+++ b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js
@@ -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__)
diff --git a/packages/use-sync-external-store/src/useSyncExternalStoreShimClient.js b/packages/use-sync-external-store/src/useSyncExternalStoreShimClient.js
index 8578b3d3fd1c7..4eb764091ae00 100644
--- a/packages/use-sync-external-store/src/useSyncExternalStoreShimClient.js
+++ b/packages/use-sync-external-store/src/useSyncExternalStoreShimClient.js
@@ -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',
         );