Skip to content

Commit

Permalink
Improve React error message when mutable sources are mutated during r…
Browse files Browse the repository at this point in the history 
…ender

Changed previous error message from:
> Cannot read from mutable source during the current render without tearing. This is a bug in React. Please file an issue.

To:
> 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.

I think this is the best we can do without adding production overhead that we'd probably prefer to avoid.
  • Loading branch information
Brian Vaughn committed Jan 26, 2021
1 parent a511dc7 commit efade36
Show file tree
Hide file tree
Showing 6 changed files with 132 additions and 4 deletions.
22 changes: 21 additions & 1 deletion packages/react-reconciler/src/ReactFiberHooks.new.js
View file
Expand Up @@ -892,6 +892,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 @@ -954,9 +966,17 @@ 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) {
console.warn(
'React detected an unsafe side effect during render. Mutable sources should not be modified during render.',
);
}
}

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
22 changes: 21 additions & 1 deletion packages/react-reconciler/src/ReactFiberHooks.old.js
View file
Expand Up @@ -871,6 +871,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 @@ -933,9 +945,17 @@ 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) {
console.warn(
'React detected an unsafe side effect during render. Mutable sources should not be modified during render.',
);
}
}

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
78 changes: 77 additions & 1 deletion packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js
View file
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 @@ -1719,6 +1719,82 @@ 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(
'React detected an unsafe side effect during render. Mutable sources should not be modified during render.',
);

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
View file
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
View file
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
View file
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."
}

0 comments on commit efade36

Please sign in to comment.