Improve React error message when mutable sources are mutated during render (#20665)

Brian Vaughn · web-flow · commit 766a7a28a9a2 · 2021-01-29T10:22:55.000-05:00
Changed previous error message from:
&gt; Cannot read from mutable source during the current render without tearing. This is a bug in React. Please file an issue.

To:
&gt; Cannot read from mutable source during the current render without tearing. This may be a bug in React. Please file an issue.

Also added a DEV only warning about the unsafe side effect:
&gt; A mutable source was mutated while the %s component was rendering. This is not supported. Move any mutations into event handlers or effects.

I think this is the best we can do without adding production overhead that we'd probably prefer to avoid.
diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js
@@ -904,6 +904,18 @@ function readFromUnsubcribedMutableSource<Source, Snapshot>(
   const getVersion = source._getVersion;
   const version = getVersion(source._source);
 
+  let mutableSourceSideEffectDetected = false;
+  if (__DEV__) {
+    // Detect side effects that update a mutable source during render.
+    // See https://github.com/facebook/react/issues/19948
+    if (source._currentlyRenderingFiber !== currentlyRenderingFiber) {
+      source._currentlyRenderingFiber = currentlyRenderingFiber;
+      source._initialVersionAsOfFirstRender = version;
+    } else if (source._initialVersionAsOfFirstRender !== version) {
+      mutableSourceSideEffectDetected = true;
+    }
+  }
+
   // Is it safe for this component to read from this source during the current render?
   let isSafeToReadFromSource = false;
 
@@ -966,9 +978,20 @@ function readFromUnsubcribedMutableSource<Source, Snapshot>(
     // but there's nothing we can do about that (short of throwing here and refusing to continue the render).
     markSourceAsDirty(source);
 
+    if (__DEV__) {
+      if (mutableSourceSideEffectDetected) {
+        const componentName = getComponentName(currentlyRenderingFiber.type);
+        console.warn(
+          'A mutable source was mutated while the %s component was rendering. This is not supported. ' +
+            'Move any mutations into event handlers or effects.',
+          componentName,
+        );
+      }
+    }
+
     invariant(
       false,
-      'Cannot read from mutable source during the current render without tearing. This is a bug in React. Please file an issue.',
+      'Cannot read from mutable source during the current render without tearing. This may be a bug in React. Please file an issue.',
     );
   }
 }
diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js
@@ -885,6 +885,18 @@ function readFromUnsubcribedMutableSource<Source, Snapshot>(
   const getVersion = source._getVersion;
   const version = getVersion(source._source);
 
+  let mutableSourceSideEffectDetected = false;
+  if (__DEV__) {
+    // Detect side effects that update a mutable source during render.
+    // See https://github.com/facebook/react/issues/19948
+    if (source._currentlyRenderingFiber !== currentlyRenderingFiber) {
+      source._currentlyRenderingFiber = currentlyRenderingFiber;
+      source._initialVersionAsOfFirstRender = version;
+    } else if (source._initialVersionAsOfFirstRender !== version) {
+      mutableSourceSideEffectDetected = true;
+    }
+  }
+
   // Is it safe for this component to read from this source during the current render?
   let isSafeToReadFromSource = false;
 
@@ -947,9 +959,20 @@ function readFromUnsubcribedMutableSource<Source, Snapshot>(
     // but there's nothing we can do about that (short of throwing here and refusing to continue the render).
     markSourceAsDirty(source);
 
+    if (__DEV__) {
+      if (mutableSourceSideEffectDetected) {
+        const componentName = getComponentName(currentlyRenderingFiber.type);
+        console.warn(
+          'A mutable source was mutated while the %s component was rendering. This is not supported. ' +
+            'Move any mutations into event handlers or effects.',
+          componentName,
+        );
+      }
+    }
+
     invariant(
       false,
-      'Cannot read from mutable source during the current render without tearing. This is a bug in React. Please file an issue.',
+      'Cannot read from mutable source during the current render without tearing. This may be a bug in React. Please file an issue.',
     );
   }
 }
diff --git a/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js b/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js
@@ -25,9 +25,9 @@ function loadModules() {
   jest.useFakeTimers();
 
   ReactFeatureFlags = require('shared/ReactFeatureFlags');
-
   ReactFeatureFlags.enableSchedulerTracing = true;
   ReactFeatureFlags.enableProfilerTimer = true;
+
   React = require('react');
   ReactNoop = require('react-noop-renderer');
   Scheduler = require('scheduler');
@@ -1720,6 +1720,84 @@ describe('useMutableSource', () => {
   });
 
   if (__DEV__) {
+    // See https://github.com/facebook/react/issues/19948
+    describe('side effecte detection', () => {
+      // @gate experimental
+      it('should throw if a mutable source is mutated during render', () => {
+        const source = createSource('initial');
+        const mutableSource = createMutableSource(
+          source,
+          param => param.version,
+        );
+
+        function MutateDuringRead() {
+          const value = useMutableSource(
+            mutableSource,
+            defaultGetSnapshot,
+            defaultSubscribe,
+          );
+          Scheduler.unstable_yieldValue('MutateDuringRead:' + value);
+          // Note that mutating an exeternal value during render is a side effect and is not supported.
+          if (value === 'initial') {
+            source.value = 'updated';
+          }
+          return null;
+        }
+
+        expect(() => {
+          expect(() => {
+            act(() => {
+              ReactNoop.renderLegacySyncRoot(
+                <React.StrictMode>
+                  <MutateDuringRead />
+                </React.StrictMode>,
+              );
+            });
+          }).toThrow(
+            'Cannot read from mutable source during the current render without tearing. This may be a bug in React. Please file an issue.',
+          );
+        }).toWarnDev(
+          'A mutable source was mutated while the MutateDuringRead component was rendering. This is not supported. ' +
+            'Move any mutations into event handlers or effects.\n' +
+            '    in MutateDuringRead (at **)',
+        );
+
+        expect(Scheduler).toHaveYielded(['MutateDuringRead:initial']);
+      });
+
+      // @gate experimental
+      it('should not misidentify mutations after render as side effects', () => {
+        const source = createSource('initial');
+        const mutableSource = createMutableSource(
+          source,
+          param => param.version,
+        );
+
+        function MutateDuringRead() {
+          const value = useMutableSource(
+            mutableSource,
+            defaultGetSnapshot,
+            defaultSubscribe,
+          );
+          Scheduler.unstable_yieldValue('MutateDuringRead:' + value);
+          return null;
+        }
+
+        act(() => {
+          ReactNoop.renderLegacySyncRoot(
+            <React.StrictMode>
+              <MutateDuringRead />
+            </React.StrictMode>,
+          );
+          expect(Scheduler).toFlushAndYieldThrough([
+            'MutateDuringRead:initial',
+          ]);
+          source.value = 'updated';
+        });
+        expect(Scheduler).toHaveYielded(['MutateDuringRead:updated']);
+      });
+    });
+
     describe('dev warnings', () => {
       // @gate experimental
       it('should warn if the subscribe function does not return an unsubscribe function', () => {
diff --git a/packages/react/src/ReactMutableSource.js b/packages/react/src/ReactMutableSource.js
@@ -23,6 +23,11 @@ export function createMutableSource<Source: $NonMaybeType<mixed>>(
   if (__DEV__) {
     mutableSource._currentPrimaryRenderer = null;
     mutableSource._currentSecondaryRenderer = null;
+
+    // Used to detect side effects that update a mutable source during render.
+    // See https://github.com/facebook/react/issues/19948
+    mutableSource._currentlyRenderingFiber = null;
+    mutableSource._initialVersionAsOfFirstRender = null;
   }
 
   return mutableSource;
diff --git a/packages/shared/ReactTypes.js b/packages/shared/ReactTypes.js
@@ -197,6 +197,12 @@ export type MutableSource<Source: $NonMaybeType<mixed>> = {|
   // Used to detect multiple renderers using the same mutable source.
   _currentPrimaryRenderer?: Object | null,
   _currentSecondaryRenderer?: Object | null,
+
+  // DEV only
+  // Used to detect side effects that update a mutable source during render.
+  // See https://github.com/facebook/react/issues/19948
+  _currentlyRenderingFiber?: Fiber | null,
+  _initialVersionAsOfFirstRender?: MutableSourceVersion | null,
 |};
 
 // The subset of a Thenable required by things thrown by Suspense.
diff --git a/scripts/error-codes/codes.json b/scripts/error-codes/codes.json
@@ -372,5 +372,6 @@
   "381": "This feature is not supported by ReactSuspenseTestUtils.",
   "382": "This query has received more parameters than the last time the same query was used. Always pass the exact number of parameters that the query needs.",
   "383": "This query has received fewer parameters than the last time the same query was used. Always pass the exact number of parameters that the query needs.",
-  "384": "Refreshing the cache is not supported in Server Components."
+  "384": "Refreshing the cache is not supported in Server Components.",
+  "385": "Cannot read from mutable source during the current render without tearing. This may be a bug in React. Please file an issue."
 }
