From 8f4dc3e5d005459058ed7ffc26c2fb76b845ce62 Mon Sep 17 00:00:00 2001
From: Brian Vaughn <bvaughn@fb.com>
Date: Thu, 21 May 2020 16:14:29 -0700
Subject: [PATCH] Warn if MutableSource snapshot is a function (#18933)

* Warn if MutableSource snapshot is a function

useMutableSource does not properly support snapshots that are functions. In part this is because of how it is implemented internally (the function gets mistaken for a state updater function). To fix this we could just wrap another function around the returned snapshot, but this pattern seems problematic to begin with- because the function that gets returned might itself close over mutable values, which would defeat the purpose of using the hook in the first place.

This PR proposes adding a new DEV warning if the snapshot returned is a function. It does not change the behavior (meaning that a function could still work in some cases- but at least the current behavior prevents passing around a closure that may later become stale unless you're really intentional about it e.g. () => () => {...}).

* Replaced .warn with .error
---
 .../src/ReactFiberHooks.new.js                | 20 ++++++++++++-
 .../src/ReactFiberHooks.old.js                | 20 ++++++++++++-
 .../useMutableSource-test.internal.js         | 28 +++++++++++++++++++
 3 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js
index 8f87aea6cf..7c24cfa2b3 100644
--- a/packages/react-reconciler/src/ReactFiberHooks.new.js
+++ b/packages/react-reconciler/src/ReactFiberHooks.new.js
@@ -914,7 +914,16 @@ function readFromUnsubcribedMutableSource<Source, Snapshot>(
   }
 
   if (isSafeToReadFromSource) {
-    return getSnapshot(source._source);
+    const snapshot = getSnapshot(source._source);
+    if (__DEV__) {
+      if (typeof snapshot === 'function') {
+        console.error(
+          'Mutable source should not return a function as the snapshot value. ' +
+            'Functions may close over mutable values and cause tearing.',
+        );
+      }
+    }
+    return snapshot;
   } else {
     // This handles the special case of a mutable source being shared beween renderers.
     // In that case, if the source is mutated between the first and second renderer,
@@ -992,6 +1001,15 @@ function useMutableSource<Source, Snapshot>(
     const maybeNewVersion = getVersion(source._source);
     if (!is(version, maybeNewVersion)) {
       const maybeNewSnapshot = getSnapshot(source._source);
+      if (__DEV__) {
+        if (typeof maybeNewSnapshot === 'function') {
+          console.error(
+            'Mutable source should not return a function as the snapshot value. ' +
+              'Functions may close over mutable values and cause tearing.',
+          );
+        }
+      }
+
       if (!is(snapshot, maybeNewSnapshot)) {
         setSnapshot(maybeNewSnapshot);
 
diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js
index 6b733ac698..fbb9a55b27 100644
--- a/packages/react-reconciler/src/ReactFiberHooks.old.js
+++ b/packages/react-reconciler/src/ReactFiberHooks.old.js
@@ -900,7 +900,16 @@ function readFromUnsubcribedMutableSource<Source, Snapshot>(
   }
 
   if (isSafeToReadFromSource) {
-    return getSnapshot(source._source);
+    const snapshot = getSnapshot(source._source);
+    if (__DEV__) {
+      if (typeof snapshot === 'function') {
+        console.error(
+          'Mutable source should not return a function as the snapshot value. ' +
+            'Functions may close over mutable values and cause tearing.',
+        );
+      }
+    }
+    return snapshot;
   } else {
     // This handles the special case of a mutable source being shared beween renderers.
     // In that case, if the source is mutated between the first and second renderer,
@@ -978,6 +987,15 @@ function useMutableSource<Source, Snapshot>(
     const maybeNewVersion = getVersion(source._source);
     if (!is(version, maybeNewVersion)) {
       const maybeNewSnapshot = getSnapshot(source._source);
+      if (__DEV__) {
+        if (typeof maybeNewSnapshot === 'function') {
+          console.error(
+            'Mutable source should not return a function as the snapshot value. ' +
+              'Functions may close over mutable values and cause tearing.',
+          );
+        }
+      }
+
       if (!is(snapshot, maybeNewSnapshot)) {
         setSnapshot(maybeNewSnapshot);
 
diff --git a/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js b/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js
index 7bdb1b4420..4523e753be 100644
--- a/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js
+++ b/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js
@@ -1493,6 +1493,34 @@ describe('useMutableSource', () => {
     },
   );
 
+  // @gate experimental
+  it('warns about functions being used as snapshot values', async () => {
+    const source = createSource(() => 'a');
+    const mutableSource = createMutableSource(source);
+
+    const getSnapshot = () => source.value;
+
+    function Read() {
+      const fn = useMutableSource(mutableSource, getSnapshot, defaultSubscribe);
+      const value = fn();
+      Scheduler.unstable_yieldValue(value);
+      return value;
+    }
+
+    const root = ReactNoop.createRoot();
+    await act(async () => {
+      root.render(
+        <>
+          <Read />
+        </>,
+      );
+      expect(() => expect(Scheduler).toFlushAndYield(['a'])).toErrorDev(
+        'Mutable source should not return a function as the snapshot value.',
+      );
+    });
+    expect(root).toMatchRenderedOutput('a');
+  });
+
   // @gate experimental
   it('getSnapshot changes and then source is mutated during interleaved event', async () => {
     const {useEffect} = React;