From f5dd422b0518896dd395b983659fc79d32c3e72e 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 | 10 ++++++++++ .../react-reconciler/src/ReactFiberHooks.js | 6 ++++++ .../src/ReactFiberReconciler.js | 14 ++++++++++++++ .../src/ReactFiberScheduler.js | 19 +++++++++++++------ 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 + 11 files changed, 52 insertions(+), 6 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 9c65a665c8eeb..c52a0777b6c28 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -52,6 +52,7 @@ import { requestCurrentTime, computeExpirationForFiber, scheduleWork, + flushPassiveEffects, } from './ReactFiberScheduler'; const fakeInternalInstance = {}; @@ -193,6 +194,9 @@ const classComponentUpdater = { update.callback = callback; } + if (revertPassiveEffectsChange) { + flushPassiveEffects(); + } enqueueUpdate(fiber, update); scheduleWork(fiber, expirationTime); }, @@ -212,6 +216,9 @@ const classComponentUpdater = { update.callback = callback; } + if (revertPassiveEffectsChange) { + flushPassiveEffects(); + } enqueueUpdate(fiber, update); scheduleWork(fiber, expirationTime); }, @@ -230,6 +237,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..85b95f12edf35 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 (!revertPassiveEffectChange) { + 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/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