From d0e98c5f1795128313e4fef149c4b637d7b8f577 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 15 May 2019 12:41:28 -0700 Subject: [PATCH] Feature flag to revert #15650 PR #15650 is a bugfix but it's technically a semantic change that could cause regressions. I don't think it will be an issue, since the previous behavior was both broken and incoherent, but out of an abundance of caution, let's wrap it in a flag so we can easily revert it if necessary. --- .../src/ReactFiberClassComponent.js | 11 +++++ .../react-reconciler/src/ReactFiberHooks.js | 6 +++ .../src/ReactFiberReconciler.js | 14 +++++++ .../src/ReactFiberScheduler.js | 19 ++++++--- ...eactHooksWithNoopRenderer-test.internal.js | 42 +++++++++++++++++++ packages/shared/ReactFeatureFlags.js | 3 ++ .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.persistent.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + .../ReactFeatureFlags.test-renderer.www.js | 1 + .../shared/forks/ReactFeatureFlags.www.js | 1 + 12 files changed, 95 insertions(+), 6 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 9c65a665c8eeb..653de4526c31b 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -52,7 +52,9 @@ import { requestCurrentTime, computeExpirationForFiber, scheduleWork, + flushPassiveEffects, } from './ReactFiberScheduler'; +import {revertPassiveEffectsChange} from 'shared/ReactFeatureFlags'; const fakeInternalInstance = {}; const isArray = Array.isArray; @@ -193,6 +195,9 @@ const classComponentUpdater = { update.callback = callback; } + if (revertPassiveEffectsChange) { + flushPassiveEffects(); + } enqueueUpdate(fiber, update); scheduleWork(fiber, expirationTime); }, @@ -212,6 +217,9 @@ const classComponentUpdater = { update.callback = callback; } + if (revertPassiveEffectsChange) { + flushPassiveEffects(); + } enqueueUpdate(fiber, update); scheduleWork(fiber, expirationTime); }, @@ -230,6 +238,9 @@ const classComponentUpdater = { update.callback = callback; } + if (revertPassiveEffectsChange) { + flushPassiveEffects(); + } enqueueUpdate(fiber, update); scheduleWork(fiber, expirationTime); }, diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index ffa4779eae712..64eb1be345f74 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -31,6 +31,7 @@ import { import { scheduleWork, computeExpirationForFiber, + flushPassiveEffects, requestCurrentTime, warnIfNotCurrentlyActingUpdatesInDev, markRenderEventTime, @@ -41,6 +42,7 @@ import warning from 'shared/warning'; import getComponentName from 'shared/getComponentName'; import is from 'shared/objectIs'; import {markWorkInProgressReceivedUpdate} from './ReactFiberBeginWork'; +import {revertPassiveEffectsChange} from 'shared/ReactFeatureFlags'; const {ReactCurrentDispatcher} = ReactSharedInternals; @@ -1107,6 +1109,10 @@ function dispatchAction( lastRenderPhaseUpdate.next = update; } } else { + if (revertPassiveEffectsChange) { + flushPassiveEffects(); + } + const currentTime = requestCurrentTime(); const expirationTime = computeExpirationForFiber(currentTime, fiber); diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index cae00c7b8964c..025f86bd47841 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -64,6 +64,7 @@ import { } from './ReactCurrentFiber'; import {StrictMode} from './ReactTypeOfMode'; import {Sync} from './ReactFiberExpirationTime'; +import {revertPassiveEffectsChange} from 'shared/ReactFeatureFlags'; type OpaqueRoot = FiberRoot; @@ -152,6 +153,9 @@ function scheduleRootUpdate( update.callback = callback; } + if (revertPassiveEffectsChange) { + flushPassiveEffects(); + } enqueueUpdate(current, update); scheduleWork(current, expirationTime); @@ -391,6 +395,10 @@ if (__DEV__) { id--; } if (currentHook !== null) { + if (revertPassiveEffectChange) { + flushPassiveEffects(); + } + const newState = copyWithSet(currentHook.memoizedState, path, value); currentHook.memoizedState = newState; currentHook.baseState = newState; @@ -408,6 +416,9 @@ if (__DEV__) { // Support DevTools props for function components, forwardRef, memo, host components, etc. overrideProps = (fiber: Fiber, path: Array, value: any) => { + if (revertPassiveEffectChange) { + flushPassiveEffects(); + } fiber.pendingProps = copyWithSet(fiber.memoizedProps, path, value); if (fiber.alternate) { fiber.alternate.pendingProps = fiber.pendingProps; @@ -416,6 +427,9 @@ if (__DEV__) { }; scheduleUpdate = (fiber: Fiber) => { + if (revertPassiveEffectChange) { + flushPassiveEffects(); + } scheduleWork(fiber, Sync); }; diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 09b5c2b4b0c71..d7c30f462a868 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -24,6 +24,7 @@ import { enableProfilerTimer, disableYielding, enableSchedulerTracing, + revertPassiveEffectsChange, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import invariant from 'shared/invariant'; @@ -560,9 +561,11 @@ export function flushInteractiveUpdates() { return; } flushPendingDiscreteUpdates(); - // If the discrete updates scheduled passive effects, flush them now so that - // they fire before the next serial event. - flushPassiveEffects(); + if (!revertPassiveEffectsChange) { + // If the discrete updates scheduled passive effects, flush them now so that + // they fire before the next serial event. + flushPassiveEffects(); + } } function resolveLocksOnRoot(root: FiberRoot, expirationTime: ExpirationTime) { @@ -598,8 +601,10 @@ export function interactiveUpdates( // should explicitly call flushInteractiveUpdates. flushPendingDiscreteUpdates(); } - // TODO: Remove this call for the same reason as above. - flushPassiveEffects(); + if (!revertPassiveEffectsChange) { + // TODO: Remove this call for the same reason as above. + flushPassiveEffects(); + } return runWithPriority(UserBlockingPriority, fn.bind(null, a, b, c)); } @@ -750,7 +755,9 @@ function renderRoot( return commitRoot.bind(null, root); } - flushPassiveEffects(); + if (!revertPassiveEffectsChange) { + flushPassiveEffects(); + } // If the root or expiration time have changed, throw out the existing stack // and prepare a fresh one. Otherwise we'll continue where we left off. diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index b900086a49e74..3a2bf76122b10 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -2077,4 +2077,46 @@ describe('ReactHooksWithNoopRenderer', () => { expect(Scheduler).toFlushAndYield(['Step: 5, Shadow: 5']); expect(ReactNoop).toMatchRenderedOutput('5'); }); + + describe('revertPassiveEffectsChange', () => { + it('flushes serial effects before enqueueing work', () => { + jest.resetModules(); + + ReactFeatureFlags = require('shared/ReactFeatureFlags'); + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; + ReactFeatureFlags.enableSchedulerTracing = true; + ReactFeatureFlags.revertPassiveEffectsChange = true; + React = require('react'); + ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); + SchedulerTracing = require('scheduler/tracing'); + useState = React.useState; + useEffect = React.useEffect; + act = ReactNoop.act; + + let _updateCount; + function Counter(props) { + const [count, updateCount] = useState(0); + _updateCount = updateCount; + useEffect(() => { + Scheduler.yieldValue(`Will set count to 1`); + updateCount(1); + }, []); + return ; + } + + ReactNoop.render(, () => + Scheduler.yieldValue('Sync effect'), + ); + expect(Scheduler).toFlushAndYieldThrough(['Count: 0', 'Sync effect']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + // Enqueuing this update forces the passive effect to be flushed -- + // updateCount(1) happens first, so 2 wins. + act(() => _updateCount(2)); + expect(Scheduler).toHaveYielded(['Will set count to 1']); + expect(Scheduler).toFlushAndYield(['Count: 2']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 2')]); + }); + }); }); diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 398aa209707d4..1cabcfdd1b3b9 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -67,3 +67,6 @@ export const enableEventAPI = false; // New API for JSX transforms to target - https://github.com/reactjs/rfcs/pull/107 export const enableJSXTransformAPI = false; + +// Temporary flag to revert the fix in #15650 +export const revertPassiveEffectsChange = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 285ea06bbc64f..9aad62c48d515 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -32,6 +32,7 @@ export const warnAboutDeprecatedLifecycles = true; export const warnAboutDeprecatedSetNativeProps = true; export const enableEventAPI = false; export const enableJSXTransformAPI = false; +export const revertPassiveEffectsChange = false; // Only used in www builds. export function addUserTimingListener() { diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 60f29acdc7797..651a840b24981 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -29,6 +29,7 @@ export const enableSchedulerDebugging = false; export const warnAboutDeprecatedSetNativeProps = false; export const enableEventAPI = false; export const enableJSXTransformAPI = false; +export const revertPassiveEffectsChange = false; // Only used in www builds. export function addUserTimingListener() { diff --git a/packages/shared/forks/ReactFeatureFlags.persistent.js b/packages/shared/forks/ReactFeatureFlags.persistent.js index 14b8716b96342..085a9bae85873 100644 --- a/packages/shared/forks/ReactFeatureFlags.persistent.js +++ b/packages/shared/forks/ReactFeatureFlags.persistent.js @@ -29,6 +29,7 @@ export const enableSchedulerDebugging = false; export const warnAboutDeprecatedSetNativeProps = false; export const enableEventAPI = false; export const enableJSXTransformAPI = false; +export const revertPassiveEffectsChange = false; // Only used in www builds. export function addUserTimingListener() { diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 40c982f3e7cc3..f143adc2761e0 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -29,6 +29,7 @@ export const enableSchedulerDebugging = false; export const warnAboutDeprecatedSetNativeProps = false; export const enableEventAPI = false; export const enableJSXTransformAPI = false; +export const revertPassiveEffectsChange = false; // Only used in www builds. export function addUserTimingListener() { diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index f6f80c8985350..83b1ff0bbd319 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -27,6 +27,7 @@ export const disableJavaScriptURLs = false; export const disableYielding = false; export const enableEventAPI = true; export const enableJSXTransformAPI = true; +export const revertPassiveEffectsChange = false; // Only used in www builds. export function addUserTimingListener() { diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 0be35ad2d9f4a..79d545431cbfa 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -20,6 +20,7 @@ export const { disableInputAttributeSyncing, warnAboutShorthandPropertyCollision, warnAboutDeprecatedSetNativeProps, + revertPassiveEffectsChange, } = require('ReactFeatureFlags'); // In www, we have experimental support for gathering data