diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index fe49e725fd7fc..46c6b5af0e96f 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -904,18 +904,6 @@ 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; @@ -978,17 +966,44 @@ function readFromUnsubcribedMutableSource( // but there's nothing we can do about that (short of throwing here and refusing to continue the render). markSourceAsDirty(source); + // Intentioally throw an error to force React to retry synchronously. During + // the synchronous retry, it will block interleaved mutations, so we should + // get a consistent read. Therefore, the following error should never be + // visible to the user. + // + // If it were to become visible to the user, it suggests one of two things: + // a bug in React, or (more likely), a mutation during the render phase that + // caused the second re-render attempt to be different from the first. + // + // We know it's the second case if the logs are currently disabled. So in + // dev, we can present a more accurate error message. if (__DEV__) { - if (mutableSourceSideEffectDetected) { + // eslint-disable-next-line react-internal/no-production-logging + if (console.log.__reactDisabledLog) { + // If the logs are disabled, this is the dev-only double render. This is + // only reachable if there was a mutation during render. Show a helpful + // error message. + // + // Something interesting to note: because we only double render in + // development, this error will never happen during production. This is + // actually true of all errors that occur during a double render, + // because if the first render had thrown, we would have exited the + // begin phase without double rendering. We should consider suppressing + // any error from a double render (with a warning) to more closely match + // the production behavior. 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.', + invariant( + false, + 'A mutable source was mutated while the %s component was rendering. ' + + 'This is not supported. Move any mutations into event handlers ' + + 'or effects.', componentName, ); } } + // We expect this error not to be thrown during the synchronous retry, + // because we blocked interleaved mutations. invariant( false, '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 ccb719dc3d8a1..35b0c2ca15482 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -885,18 +885,6 @@ 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; @@ -959,17 +947,44 @@ function readFromUnsubcribedMutableSource( // but there's nothing we can do about that (short of throwing here and refusing to continue the render). markSourceAsDirty(source); + // Intentioally throw an error to force React to retry synchronously. During + // the synchronous retry, it will block interleaved mutations, so we should + // get a consistent read. Therefore, the following error should never be + // visible to the user. + // + // If it were to become visible to the user, it suggests one of two things: + // a bug in React, or (more likely), a mutation during the render phase that + // caused the second re-render attempt to be different from the first. + // + // We know it's the second case if the logs are currently disabled. So in + // dev, we can present a more accurate error message. if (__DEV__) { - if (mutableSourceSideEffectDetected) { + // eslint-disable-next-line react-internal/no-production-logging + if (console.log.__reactDisabledLog) { + // If the logs are disabled, this is the dev-only double render. This is + // only reachable if there was a mutation during render. Show a helpful + // error message. + // + // Something interesting to note: because we only double render in + // development, this error will never happen during production. This is + // actually true of all errors that occur during a double render, + // because if the first render had thrown, we would have exited the + // begin phase without double rendering. We should consider suppressing + // any error from a double render (with a warning) to more closely match + // the production behavior. 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.', + invariant( + false, + 'A mutable source was mutated while the %s component was rendering. ' + + 'This is not supported. Move any mutations into event handlers ' + + 'or effects.', componentName, ); } } + // We expect this error not to be thrown during the synchronous retry, + // because we blocked interleaved mutations. invariant( false, '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 243232f1b83bf..1969c2f7adce4 100644 --- a/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js +++ b/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js @@ -1724,6 +1724,46 @@ describe('useMutableSource', () => { describe('side effecte detection', () => { // @gate experimental it('should throw if a mutable source is mutated during render', () => { + const source = createSource(0); + const mutableSource = createMutableSource( + source, + param => param.version, + ); + + let mutatedValueInRender = 1; + 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. + source.value = mutatedValueInRender++; + return null; + } + + expect(() => { + act(() => { + ReactNoop.render(); + }); + }).toThrow( + 'A mutable source was mutated while the MutateDuringRead component ' + + 'was rendering. This is not supported. Move any mutations into ' + + 'event handlers or effects.', + ); + + expect(Scheduler).toHaveYielded([ + // First attempt + 'MutateDuringRead:0', + + // Synchronous retry + 'MutateDuringRead:1', + ]); + }); + + // @gate experimental + it('should throw if a mutable source is mutated during render (legacy mode)', () => { const source = createSource('initial'); const mutableSource = createMutableSource( source, @@ -1745,21 +1785,17 @@ describe('useMutableSource', () => { } 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 **)', + act(() => { + ReactNoop.renderLegacySyncRoot( + + + , + ); + }); + }).toThrow( + 'A mutable source was mutated while the MutateDuringRead component ' + + 'was rendering. This is not supported. Move any mutations into ' + + 'event handlers or effects.', ); expect(Scheduler).toHaveYielded(['MutateDuringRead:initial']); diff --git a/scripts/error-codes/codes.json b/scripts/error-codes/codes.json index 61b6b33d02bc2..e758c79be2e58 100644 --- a/scripts/error-codes/codes.json +++ b/scripts/error-codes/codes.json @@ -339,7 +339,7 @@ "345": "Root did not complete. This is a bug in React.", "348": "ensureListeningTo(): received a container that was not an element node. This is likely a bug in React.", "349": "Expected a work-in-progress root. This is a bug in React. Please file an issue.", - "350": "Cannot read from mutable source during the current render without tearing. This is a bug in React. Please file an issue.", + "350": "Cannot read from mutable source during the current render without tearing. This may be a bug in React. Please file an issue.", "351": "Unsupported server component type: %s", "352": "React Lazy Components are not yet supported on the server.", "353": "A server block should never encode any other slots. This is a bug in React.", @@ -373,5 +373,5 @@ "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.", - "385": "Cannot read from mutable source during the current render without tearing. This may be a bug in React. Please file an issue." + "385": "A mutable source was mutated while the %s component was rendering. This is not supported. Move any mutations into event handlers or effects." }