Skip to content

Commit

Permalink
Detect subscriptions wrapped in startTransition (#22271)
Browse files Browse the repository at this point in the history
* Detect subscriptions wrapped in startTransition
  • Loading branch information
salazarm authored Sep 8, 2021
1 parent 95d762e commit a3fde23
Show file tree
Hide file tree
Showing 17 changed files with 184 additions and 4 deletions.
18 changes: 18 additions & 0 deletions packages/react-reconciler/src/ReactFiberHooks.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ import {
} from './ReactUpdateQueue.new';
import {pushInterleavedQueue} from './ReactFiberInterleavedUpdates.new';
import {getIsStrictModeForDevtools} from './ReactFiberReconciler.new';
import {warnOnSubscriptionInsideStartTransition} from 'shared/ReactFeatureFlags';

const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals;

Expand Down Expand Up @@ -1861,6 +1862,23 @@ function startTransition(setPending, callback) {
} finally {
setCurrentUpdatePriority(previousPriority);
ReactCurrentBatchConfig.transition = prevTransition;
if (__DEV__) {
if (
prevTransition !== 1 &&
warnOnSubscriptionInsideStartTransition &&
ReactCurrentBatchConfig._updatedFibers
) {
const updatedFibersCount = ReactCurrentBatchConfig._updatedFibers.size;
if (updatedFibersCount > 10) {
console.warn(
'Detected a large number of updates inside startTransition. ' +
'If this is due to a subscription please re-write it to use React provided hooks. ' +
'Otherwise concurrent mode guarantees are off the table.',
);
}
ReactCurrentBatchConfig._updatedFibers.clear();
}
}
}
}

Expand Down
18 changes: 18 additions & 0 deletions packages/react-reconciler/src/ReactFiberHooks.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ import {
} from './ReactUpdateQueue.old';
import {pushInterleavedQueue} from './ReactFiberInterleavedUpdates.old';
import {getIsStrictModeForDevtools} from './ReactFiberReconciler.old';
import {warnOnSubscriptionInsideStartTransition} from 'shared/ReactFeatureFlags';

const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals;

Expand Down Expand Up @@ -1861,6 +1862,23 @@ function startTransition(setPending, callback) {
} finally {
setCurrentUpdatePriority(previousPriority);
ReactCurrentBatchConfig.transition = prevTransition;
if (__DEV__) {
if (
prevTransition !== 1 &&
warnOnSubscriptionInsideStartTransition &&
ReactCurrentBatchConfig._updatedFibers
) {
const updatedFibersCount = ReactCurrentBatchConfig._updatedFibers.size;
if (updatedFibersCount > 10) {
console.warn(
'Detected a large number of updates inside startTransition. ' +
'If this is due to a subscription please re-write it to use React provided hooks. ' +
'Otherwise concurrent mode guarantees are off the table.',
);
}
ReactCurrentBatchConfig._updatedFibers.clear();
}
}
}
}

Expand Down
8 changes: 8 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
enableStrictEffects,
skipUnmountedBoundaries,
enableUpdaterTracking,
warnOnSubscriptionInsideStartTransition,
} from 'shared/ReactFeatureFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import invariant from 'shared/invariant';
Expand Down Expand Up @@ -385,6 +386,13 @@ export function requestUpdateLane(fiber: Fiber): Lane {

const isTransition = requestCurrentTransition() !== NoTransition;
if (isTransition) {
if (
__DEV__ &&
warnOnSubscriptionInsideStartTransition &&
ReactCurrentBatchConfig._updatedFibers
) {
ReactCurrentBatchConfig._updatedFibers.add(fiber);
}
// The algorithm for assigning an update to a lane should be stable for all
// updates at the same priority within the same event. To do this, the
// inputs to the algorithm must be the same.
Expand Down
8 changes: 8 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
enableStrictEffects,
skipUnmountedBoundaries,
enableUpdaterTracking,
warnOnSubscriptionInsideStartTransition,
} from 'shared/ReactFeatureFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import invariant from 'shared/invariant';
Expand Down Expand Up @@ -385,6 +386,13 @@ export function requestUpdateLane(fiber: Fiber): Lane {

const isTransition = requestCurrentTransition() !== NoTransition;
if (isTransition) {
if (
__DEV__ &&
warnOnSubscriptionInsideStartTransition &&
ReactCurrentBatchConfig._updatedFibers
) {
ReactCurrentBatchConfig._updatedFibers.add(fiber);
}
// The algorithm for assigning an update to a lane should be stable for all
// updates at the same priority within the same event. To do this, the
// inputs to the algorithm must be the same.
Expand Down
14 changes: 12 additions & 2 deletions packages/react/src/ReactCurrentBatchConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,22 @@
* @flow
*/

import type {Fiber} from 'react-reconciler/src/ReactInternalTypes';

type BatchConfig = {
transition: number,
_updatedFibers?: Set<Fiber>,
};
/**
* Keeps track of the current batch's configuration such as how long an update
* should suspend for if it needs to.
*/
const ReactCurrentBatchConfig = {
transition: (0: number),
const ReactCurrentBatchConfig: BatchConfig = {
transition: 0,
};

if (__DEV__) {
ReactCurrentBatchConfig._updatedFibers = new Set();
}

export default ReactCurrentBatchConfig;
18 changes: 18 additions & 0 deletions packages/react/src/ReactStartTransition.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/

import ReactCurrentBatchConfig from './ReactCurrentBatchConfig';
import {warnOnSubscriptionInsideStartTransition} from 'shared/ReactFeatureFlags';

export function startTransition(scope: () => void) {
const prevTransition = ReactCurrentBatchConfig.transition;
Expand All @@ -16,5 +17,22 @@ export function startTransition(scope: () => void) {
scope();
} finally {
ReactCurrentBatchConfig.transition = prevTransition;
if (__DEV__) {
if (
prevTransition !== 1 &&
warnOnSubscriptionInsideStartTransition &&
ReactCurrentBatchConfig._updatedFibers
) {
const updatedFibersCount = ReactCurrentBatchConfig._updatedFibers.size;
if (updatedFibersCount > 10) {
console.warn(
'Detected a large number of updates inside startTransition. ' +
'If this is due to a subscription please re-write it to use React provided hooks. ' +
'Otherwise concurrent mode guarantees are off the table.',
);
}
ReactCurrentBatchConfig._updatedFibers.clear();
}
}
}
}
91 changes: 91 additions & 0 deletions packages/react/src/__tests__/ReactStartTransition-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @emails react-core
*/

'use strict';

let React;
let ReactTestRenderer;
let act;
let useState;
let useTransition;

const SUSPICIOUS_NUMBER_OF_FIBERS_UPDATED = 10;

describe('ReactStartTransition', () => {
beforeEach(() => {
jest.resetModules();
React = require('react');
ReactTestRenderer = require('react-test-renderer');
act = require('jest-react').act;
useState = React.useState;
useTransition = React.useTransition;
});

// @gate warnOnSubscriptionInsideStartTransition || !__DEV__
it('Warns if a suspicious number of fibers are updated inside startTransition', () => {
const subs = new Set();
const useUserSpaceSubscription = () => {
const setState = useState(0)[1];
subs.add(setState);
};

let triggerHookTransition;

const Component = ({level}) => {
useUserSpaceSubscription();
if (level === 0) {
triggerHookTransition = useTransition()[1];
}
if (level < SUSPICIOUS_NUMBER_OF_FIBERS_UPDATED) {
return <Component level={level + 1} />;
}
return null;
};

act(() => {
ReactTestRenderer.create(<Component level={0} />, {
unstable_isConcurrent: true,
});
});

expect(() => {
act(() => {
React.startTransition(() => {
subs.forEach(setState => {
setState(state => state + 1);
});
});
});
}).toWarnDev(
[
'Detected a large number of updates inside startTransition. ' +
'If this is due to a subscription please re-write it to use React provided hooks. ' +
'Otherwise concurrent mode guarantees are off the table.',
],
{withoutStack: true},
);

expect(() => {
act(() => {
triggerHookTransition(() => {
subs.forEach(setState => {
setState(state => state + 1);
});
});
});
}).toWarnDev(
[
'Detected a large number of updates inside startTransition. ' +
'If this is due to a subscription please re-write it to use React provided hooks. ' +
'Otherwise concurrent mode guarantees are off the table.',
],
{withoutStack: true},
);
});
});
2 changes: 2 additions & 0 deletions packages/shared/ReactFeatureFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ export const enableTrustedTypesIntegration = false;
// a deprecated pattern we want to get rid of in the future
export const warnAboutSpreadingKeyToJSX = false;

export const warnOnSubscriptionInsideStartTransition = false;

export const enableComponentStackLocations = true;

export const enableNewReconciler = false;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-fb.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export const disableTextareaChildren = false;
export const disableModulePatternComponents = false;
export const warnUnstableRenderSubtreeIntoContainer = false;
export const warnAboutSpreadingKeyToJSX = false;
export const warnOnSubscriptionInsideStartTransition = false;
export const enableComponentStackLocations = false;
export const enableLegacyFBSupport = false;
export const enableFilterEmptyStringAttributesDOM = false;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-oss.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export const disableTextareaChildren = false;
export const disableModulePatternComponents = false;
export const warnUnstableRenderSubtreeIntoContainer = false;
export const warnAboutSpreadingKeyToJSX = false;
export const warnOnSubscriptionInsideStartTransition = false;
export const enableComponentStackLocations = false;
export const enableLegacyFBSupport = false;
export const enableFilterEmptyStringAttributesDOM = false;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export const disableTextareaChildren = false;
export const disableModulePatternComponents = false;
export const warnUnstableRenderSubtreeIntoContainer = false;
export const warnAboutSpreadingKeyToJSX = false;
export const warnOnSubscriptionInsideStartTransition = false;
export const enableComponentStackLocations = true;
export const enableLegacyFBSupport = false;
export const enableFilterEmptyStringAttributesDOM = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ export const enableSuspenseLayoutEffectSemantics = false;
export const enableGetInspectorDataForInstanceInProduction = false;
export const enableNewReconciler = false;
export const deferRenderPhaseUpdateToNextBatch = false;

export const warnOnSubscriptionInsideStartTransition = false;
export const enableStrictEffects = false;
export const createRootStrictEffectsByDefault = false;
export const enableUseRefAccessWarning = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export const disableTextareaChildren = false;
export const disableModulePatternComponents = true;
export const warnUnstableRenderSubtreeIntoContainer = false;
export const warnAboutSpreadingKeyToJSX = false;
export const warnOnSubscriptionInsideStartTransition = false;
export const enableComponentStackLocations = true;
export const enableLegacyFBSupport = false;
export const enableFilterEmptyStringAttributesDOM = false;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.testing.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export const disableTextareaChildren = false;
export const disableModulePatternComponents = false;
export const warnUnstableRenderSubtreeIntoContainer = false;
export const warnAboutSpreadingKeyToJSX = false;
export const warnOnSubscriptionInsideStartTransition = false;
export const enableComponentStackLocations = true;
export const enableLegacyFBSupport = false;
export const enableFilterEmptyStringAttributesDOM = false;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.testing.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export const disableTextareaChildren = __EXPERIMENTAL__;
export const disableModulePatternComponents = true;
export const warnUnstableRenderSubtreeIntoContainer = false;
export const warnAboutSpreadingKeyToJSX = false;
export const warnOnSubscriptionInsideStartTransition = false;
export const enableComponentStackLocations = true;
export const enableLegacyFBSupport = !__EXPERIMENTAL__;
export const enableFilterEmptyStringAttributesDOM = false;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.www-dynamic.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export const disableSchedulerTimeoutInWorkLoop = __VARIANT__;
export const enableLazyContextPropagation = __VARIANT__;
export const enableSyncDefaultUpdates = __VARIANT__;
export const consoleManagedByDevToolsDuringStrictMode = __VARIANT__;
export const warnOnSubscriptionInsideStartTransition = __VARIANT__;

// Enable this flag to help with concurrent mode debugging.
// It logs information to the console about React scheduling, rendering, and commit phases.
Expand Down
2 changes: 1 addition & 1 deletion packages/shared/forks/ReactFeatureFlags.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export const {
disableSchedulerTimeoutInWorkLoop,
enableLazyContextPropagation,
enableSyncDefaultUpdates,
warnOnSubscriptionInsideStartTransition,
} = dynamicFeatureFlags;

// On WWW, __EXPERIMENTAL__ is used for a new modern build.
Expand All @@ -56,7 +57,6 @@ export const enableSchedulingProfiler =
// For now, we'll turn it on for everyone because it's *already* on for everyone in practice.
// At least this will let us stop shipping <Profiler> implementation to all users.
export const enableSchedulerDebugging = true;

export const warnAboutDeprecatedLifecycles = true;
export const disableLegacyContext = __EXPERIMENTAL__;
export const warnAboutStringRefs = false;
Expand Down

0 comments on commit a3fde23

Please sign in to comment.