Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 24 additions & 1 deletion packages/react-reconciler/src/ReactFiberHooks.new.js
Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this account for multiple components that read from the same source?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nvm I get it now

source._currentlyRenderingFiber = currentlyRenderingFiber;
source._initialVersionAsOfFirstRender = version;
Comment on lines +911 to +913
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will cause us to over-retain Fibers a little DEV mode. I assume that's not a problem but if it was I could add explicit cleanup somewhere.

} 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;

Expand Down Expand Up @@ -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.',
);
}
}
Expand Down
25 changes: 24 additions & 1 deletion packages/react-reconciler/src/ReactFiberHooks.old.js
Expand Up @@ -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;

Expand Down Expand Up @@ -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.',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem solved 😎

);
}
}
Expand Down
Expand Up @@ -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');
Expand Down Expand Up @@ -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', () => {
Expand Down
5 changes: 5 additions & 0 deletions packages/react/src/ReactMutableSource.js
Expand Up @@ -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;
Expand Down
6 changes: 6 additions & 0 deletions packages/shared/ReactTypes.js
Expand Up @@ -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.
Expand Down
3 changes: 2 additions & 1 deletion scripts/error-codes/codes.json
Expand Up @@ -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."
}