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.
facebook · Feb 21, 2022 · 35a7af8 · 35a7af8
1 parent 6a47428
commit 35a7af8
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 32 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js
@@ -1291,10 +1291,7 @@ function mountSyncExternalStore<T>(
     if (__DEV__) {
       if (!didWarnUncachedGetSnapshot) {
         const cachedSnapshot = getSnapshot();
-        if (
-          !(Number.isNaN(nextSnapshot) && Number.isNaN(cachedSnapshot)) &&
-          nextSnapshot !== cachedSnapshot
-        ) {
+        if (!is(nextSnapshot, cachedSnapshot)) {
           console.error(
             'The result of getSnapshot should be cached to avoid an infinite loop',
           );
@@ -1367,10 +1364,7 @@ function updateSyncExternalStore<T>(
   if (__DEV__) {
     if (!didWarnUncachedGetSnapshot) {
       const cachedSnapshot = getSnapshot();
-      if (
-        !(Number.isNaN(nextSnapshot) && Number.isNaN(cachedSnapshot)) &&
-        nextSnapshot !== cachedSnapshot
-      ) {
+      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
@@ -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
@@ -587,28 +587,34 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {
     );
   });
 
-  describe('extra features implemented in user-space', () => {
-    test('Selector can return NaN without infinite loop warning', async () => {
-      const store = createExternalStore({nan: 'not a number'});
+  test('getSnapshot can return NaN without infinite loop warning', async () => {
+    const store = createExternalStore('not a number');
 
-      function App() {
-        const selectedNaN = useSyncExternalStoreWithSelector(
-          store.subscribe,
-          store.getState,
-          null,
-          s => parseInt(s.nan, 10),
-        );
-        return <Text text={selectedNaN.toString()} />;
-      }
+    function App() {
+      const value = useSyncExternalStore(store.subscribe, () =>
+        parseInt(store.getState(), 10),
+      );
+      return <Text text={value} />;
+    }
 
-      const container = document.createElement('div');
-      const root = createRoot(container);
+    const container = document.createElement('div');
+    const root = createRoot(container);
 
-      await act(() => root.render(<App />));
+    // 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');
 
-      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__)
     test('memoized selectors are only called once per update', async () => {

diff --git a/packages/use-sync-external-store/src/useSyncExternalStoreShimClient.js b/packages/use-sync-external-store/src/useSyncExternalStoreShimClient.js
@@ -57,11 +57,8 @@ export function useSyncExternalStore<T>(
   const value = getSnapshot();
   if (__DEV__) {
     if (!didWarnUncachedGetSnapshot) {
-      const valueShouldBeCached = getSnapshot();
-      if (
-        !(Number.isNaN(value) && Number.isNaN(valueShouldBeCached)) &&
-        value !== valueShouldBeCached
-      ) {
+      const cachedValue = getSnapshot();
+      if (!is(value, cachedValue)) {
         console.error(
           'The result of getSnapshot should be cached to avoid an infinite loop',
         );