diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 311595f5d7b4..bac6749f6446 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -904,6 +904,18 @@ function readFromUnsubcribedMutableSource( 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( // 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 index 40e878014580..6a42f73527f9 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -885,6 +885,18 @@ function readFromUnsubcribedMutableSource( 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( // 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 index 7ddaf5de7d19..243232f1b83b 100644 --- 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( + + + , + ); + }); + }).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( + + + , + ); + 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 index 684594829db3..f8e5d0b28303 100644 --- a/packages/react/src/ReactMutableSource.js +++ b/packages/react/src/ReactMutableSource.js @@ -23,6 +23,11 @@ export function createMutableSource>( 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 index c03ab5056f6f..99e1bbb9bcd4 100644 --- a/packages/shared/ReactTypes.js +++ b/packages/shared/ReactTypes.js @@ -197,6 +197,12 @@ export type MutableSource> = {| // 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 index 12a8db733b0c..61b6b33d02bc 100644 --- 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." }