From 657d8a46b6d18dc933fdd47cb335d344598e5f88 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Sat, 30 Jan 2021 14:36:58 -0600 Subject: [PATCH] Use double render to detect render phase mutation PR #20665 added a mechanism to detect when a useMutableSource source is mutated during the render phase. It relies on the fact that we double render components during development in Strict Mode. If the version in the double render doesn't match the first, that indicates there must have been a mutation during render. However, I realized during review that is true of all errors that occur during the double render. A pure component will never throw during the double render, because if it were pure, it would have also thrown during the first render... in which case it wouldn't have double rendered! So instead of tracking and comparing the source's version, we can instead check if we're inside a double render when the error is thrown. We probably shouldn't be throwing during the double render at all, since we know it won't happen in production. (It's still a tearing bug, but that doesn't mean the component will actually throw.) I considered suppressing the error entirely, but that requires a larger conversation about how to handle errors that we know are only possible in development. I think we should probably be suppressing *all* errors (with a warning) that occur during a double render. --- .../src/ReactFiberHooks.new.js | 47 +++++++++++------ .../src/ReactFiberHooks.old.js | 47 +++++++++++------ .../useMutableSource-test.internal.js | 52 +++++++------------ scripts/error-codes/codes.json | 4 +- 4 files changed, 83 insertions(+), 67 deletions(-) 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 06eba20e6cfe7..78d4b0f8ad1f5 100644 --- a/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js +++ b/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js @@ -1744,24 +1744,14 @@ describe('useMutableSource', () => { } expect(() => { - expect(() => { - act(() => { - ReactNoop.render(); - }); - }).toThrow( - 'Cannot read from mutable source during the current render without tearing. This may be a bug in React. Please file an issue.', - ); - }).toWarnDev([ - // Warns twice because of the retry-on-error render pass. Should - // consider only warning during the first attempt, not during the - // retry. Or maybe vice versa. - '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 **)', - '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.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([ 'MutateDuringRead:initial', @@ -1792,21 +1782,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." }