From 9f7a346ee33f161e033484a3336ffdf966b82324 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sun, 12 May 2019 23:16:55 +0100 Subject: [PATCH] warn if passive effects get queued outside of an act() call. based on #15591. of note, we don't modify our own tests to satisfy this warning, instead using a feature flag to disable the warning for some tests. this is because we're testing the actual sequencing of work in these tests, and don't want to flush everything with act(). --- .../ReactHooksInspectionIntegration-test.js | 3 ++ .../src/__tests__/ReactDOMHooks-test.js | 3 ++ ...DOMServerIntegrationHooks-test.internal.js | 1 + .../ReactErrorBoundaries-test.internal.js | 1 + .../src/__tests__/ReactUpdates-test.js | 3 ++ .../react-reconciler/src/ReactFiberHooks.js | 25 ++++++++++++++-- .../src/ReactFiberScheduler.js | 30 +++++++++++++++++-- .../src/__tests__/ReactHooks-test.internal.js | 1 + ...eactHooksWithNoopRenderer-test.internal.js | 1 + ...eactIncrementalScheduling-test.internal.js | 1 + .../ReactIncrementalUpdates-test.internal.js | 1 + ...ReactSchedulerIntegration-test.internal.js | 1 + .../src/__tests__/withComponentStack-test.js | 8 ++--- packages/shared/ReactFeatureFlags.js | 5 ++++ .../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 | 2 ++ 20 files changed, 81 insertions(+), 10 deletions(-) diff --git a/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js b/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js index 5c092f585328..cb7371d7c233 100644 --- a/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js +++ b/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js @@ -14,11 +14,14 @@ let React; let ReactTestRenderer; let Scheduler; let ReactDebugTools; +let ReactFeatureFlags; let act; describe('ReactHooksInspectionIntegration', () => { beforeEach(() => { jest.resetModules(); + ReactFeatureFlags = require('shared/ReactFeatureFlags'); + ReactFeatureFlags.warnAboutUnactedEffectsinDEV = false; React = require('react'); ReactTestRenderer = require('react-test-renderer'); Scheduler = require('scheduler'); diff --git a/packages/react-dom/src/__tests__/ReactDOMHooks-test.js b/packages/react-dom/src/__tests__/ReactDOMHooks-test.js index 360cfa9f9a39..f58f077e494d 100644 --- a/packages/react-dom/src/__tests__/ReactDOMHooks-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMHooks-test.js @@ -11,6 +11,7 @@ let React; let ReactDOM; +let ReactFeatureFlags; let Scheduler; describe('ReactDOMHooks', () => { @@ -22,6 +23,8 @@ describe('ReactDOMHooks', () => { React = require('react'); ReactDOM = require('react-dom'); Scheduler = require('scheduler'); + ReactFeatureFlags = require('shared/ReactFeatureFlags'); + ReactFeatureFlags.warnAboutUnactedEffectsinDEV = false; container = document.createElement('div'); document.body.appendChild(container); diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js index a0a91763a82c..ef3343889d51 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js @@ -38,6 +38,7 @@ function initModules() { ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; + ReactFeatureFlags.warnAboutUnactedEffectsinDEV = false; React = require('react'); ReactDOM = require('react-dom'); ReactDOMServer = require('react-dom/server'); diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js index 3b19a87ecbf8..feffe0e4c80b 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js @@ -43,6 +43,7 @@ describe('ReactErrorBoundaries', () => { PropTypes = require('prop-types'); ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false; + ReactFeatureFlags.warnAboutUnactedEffectsinDEV = false; ReactDOM = require('react-dom'); React = require('react'); Scheduler = require('scheduler'); diff --git a/packages/react-dom/src/__tests__/ReactUpdates-test.js b/packages/react-dom/src/__tests__/ReactUpdates-test.js index 591ffa2d3153..1e9b3c6e3eab 100644 --- a/packages/react-dom/src/__tests__/ReactUpdates-test.js +++ b/packages/react-dom/src/__tests__/ReactUpdates-test.js @@ -12,6 +12,7 @@ let React; let ReactDOM; let ReactTestUtils; +let ReactFeatureFlags; let Scheduler; describe('ReactUpdates', () => { @@ -20,6 +21,8 @@ describe('ReactUpdates', () => { React = require('react'); ReactDOM = require('react-dom'); ReactTestUtils = require('react-dom/test-utils'); + ReactFeatureFlags = require('shared/ReactFeatureFlags'); + ReactFeatureFlags.warnAboutUnactedEffectsinDEV = false; Scheduler = require('scheduler'); }); diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index b5b7f9dd79f7..c5bd9dd24489 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -33,7 +33,8 @@ import { computeExpirationForFiber, flushPassiveEffects, requestCurrentTime, - warnIfNotCurrentlyActingUpdatesInDev, + warnIfNotCurrentlyActingEffectsInDEV, + warnIfNotCurrentlyActingUpdatesInDEV, markRenderEventTime, } from './ReactFiberScheduler'; @@ -869,6 +870,16 @@ function mountEffect( create: () => (() => void) | void, deps: Array | void | null, ): void { + if (__DEV__) { + // jest isn't a 'global', it's just exposed to tests via a wrapped function + // further, this isn't a test file, so flow doesn't recognize the symbol. So... + // $FlowExpectedError - because requirements don't give a damn about your type sigs. + if ('undefined' !== typeof jest) { + warnIfNotCurrentlyActingEffectsInDEV( + ((currentlyRenderingFiber: any): Fiber), + ); + } + } return mountEffectImpl( UpdateEffect | PassiveEffect, UnmountPassive | MountPassive, @@ -881,6 +892,16 @@ function updateEffect( create: () => (() => void) | void, deps: Array | void | null, ): void { + if (__DEV__) { + // jest isn't a 'global', it's just exposed to tests via a wrapped function + // further, this isn't a test file, so flow doesn't recognize the symbol. So... + // $FlowExpectedError - because requirements don't give a damn about your type sigs. + if ('undefined' !== typeof jest) { + warnIfNotCurrentlyActingEffectsInDEV( + ((currentlyRenderingFiber: any): Fiber), + ); + } + } return updateEffectImpl( UpdateEffect | PassiveEffect, UnmountPassive | MountPassive, @@ -1180,7 +1201,7 @@ function dispatchAction( // further, this isn't a test file, so flow doesn't recognize the symbol. So... // $FlowExpectedError - because requirements don't give a damn about your type sigs. if ('undefined' !== typeof jest) { - warnIfNotCurrentlyActingUpdatesInDev(fiber); + warnIfNotCurrentlyActingUpdatesInDEV(fiber); } } scheduleWork(fiber, expirationTime); diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index f748228f8eb0..8ea980fe2838 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -24,6 +24,7 @@ import { enableProfilerTimer, disableYielding, enableSchedulerTracing, + warnAboutUnactedEffectsinDEV, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import invariant from 'shared/invariant'; @@ -2088,7 +2089,32 @@ function warnAboutInvalidUpdatesOnClassComponentsInDEV(fiber) { } } -function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { +export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void { + if (__DEV__) { + if (warnAboutUnactedEffectsinDEV) { + if (ReactShouldWarnActingUpdates.current === false) { + warningWithoutStack( + false, + 'An effect from %s inside a test was not wrapped in act(...).\n\n' + + 'When testing, code that causes React state updates should be ' + + 'wrapped into act(...):\n\n' + + 'act(() => {\n' + + ' /* fire events that update state */\n' + + '});\n' + + '/* assert on the output */\n\n' + + "This ensures that you're testing the behavior the user would see " + + 'in the browser.' + + ' Learn more at https://fb.me/react-wrap-tests-with-act' + + '%s', + getComponentName(fiber.type), + getStackByFiberInDevAndProd(fiber), + ); + } + } + } +} + +export function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { if (__DEV__) { if ( workPhase === NotWorking && @@ -2114,8 +2140,6 @@ function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { } } -export const warnIfNotCurrentlyActingUpdatesInDev = warnIfNotCurrentlyActingUpdatesInDEV; - let componentsWithSuspendedDiscreteUpdates = null; export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) { if (__DEV__) { diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 1e4b1939ef05..d16e12069e28 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -27,6 +27,7 @@ describe('ReactHooks', () => { ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; + ReactFeatureFlags.warnAboutUnactedEffectsinDEV = false; React = require('react'); ReactTestRenderer = require('react-test-renderer'); Scheduler = require('scheduler'); diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index 7a6be5385373..00601a86d36b 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -39,6 +39,7 @@ describe('ReactHooksWithNoopRenderer', () => { ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; ReactFeatureFlags.enableSchedulerTracing = true; + ReactFeatureFlags.warnAboutUnactedEffectsinDEV = false; React = require('react'); ReactNoop = require('react-noop-renderer'); Scheduler = require('scheduler'); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.internal.js index ddc62c60aa6f..a9441eea126d 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.internal.js @@ -20,6 +20,7 @@ describe('ReactIncrementalScheduling', () => { jest.resetModules(); ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; + ReactFeatureFlags.warnAboutUnactedEffectsinDEV = false; React = require('react'); ReactNoop = require('react-noop-renderer'); Scheduler = require('scheduler'); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js index 26af1eb0014a..5dd917a3a3ee 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js @@ -20,6 +20,7 @@ describe('ReactIncrementalUpdates', () => { jest.resetModuleRegistry(); ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; + ReactFeatureFlags.warnAboutUnactedEffectsinDEV = false; React = require('react'); ReactNoop = require('react-noop-renderer'); Scheduler = require('scheduler'); diff --git a/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.internal.js index dec4bfbfc57b..13d2e83f29f7 100644 --- a/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.internal.js @@ -26,6 +26,7 @@ describe('ReactSchedulerIntegration', () => { jest.resetModules(); ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; + ReactFeatureFlags.warnAboutUnactedEffectsinDEV = false; React = require('react'); ReactNoop = require('react-noop-renderer'); Scheduler = require('scheduler'); diff --git a/packages/react/src/__tests__/withComponentStack-test.js b/packages/react/src/__tests__/withComponentStack-test.js index e46d7f2e5499..3289b224ae6a 100644 --- a/packages/react/src/__tests__/withComponentStack-test.js +++ b/packages/react/src/__tests__/withComponentStack-test.js @@ -44,7 +44,6 @@ describe('withComponentStack', () => { let React = null; let ReactTestRenderer = null; let error = null; - let scheduler = null; let warn = null; beforeEach(() => { @@ -53,7 +52,6 @@ describe('withComponentStack', () => { React = require('react'); ReactTestRenderer = require('react-test-renderer'); - scheduler = require('scheduler'); error = React.error; warn = React.warn; @@ -179,9 +177,9 @@ describe('withComponentStack', () => { return null; } - ReactTestRenderer.create(); - - scheduler.flushAll(); // Flush passive effects + ReactTestRenderer.act(() => { + ReactTestRenderer.create(); + }); expectMessageAndStack( 'logged in child render method', diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 5feb40a9a004..94710379f37c 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -71,3 +71,8 @@ export const enableJSXTransformAPI = false; // We will enforce mocking scheduler with scheduler/unstable_mock at some point. (v17?) // Till then, we warn about the missing mock, but still fallback to a sync mode compatible version export const warnAboutMissingMockScheduler = false; + +// We warn if effects are queued in tests without a surround act() +// However, this interferes withour internal tests that test the actual +// sequencing work. So we use a feature flag to disable the warning *only* for ourselves +export const warnAboutUnactedEffectsinDEV = true; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 78f2995ae1a7..89cc2933b35d 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -33,6 +33,7 @@ export const warnAboutDeprecatedSetNativeProps = true; export const enableEventAPI = false; export const enableJSXTransformAPI = false; export const warnAboutMissingMockScheduler = true; +export const warnAboutUnactedEffectsinDEV = true; // 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 a95a2db448bf..13d0c1ba63d3 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -30,6 +30,7 @@ export const warnAboutDeprecatedSetNativeProps = false; export const enableEventAPI = false; export const enableJSXTransformAPI = false; export const warnAboutMissingMockScheduler = false; +export const warnAboutUnactedEffectsinDEV = true; // 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 baaebcffd0ff..8df295cb9023 100644 --- a/packages/shared/forks/ReactFeatureFlags.persistent.js +++ b/packages/shared/forks/ReactFeatureFlags.persistent.js @@ -30,6 +30,7 @@ export const warnAboutDeprecatedSetNativeProps = false; export const enableEventAPI = false; export const enableJSXTransformAPI = false; export const warnAboutMissingMockScheduler = true; +export const warnAboutUnactedEffectsinDEV = true; // 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 166bfbf448ba..e796e650dc75 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -30,6 +30,7 @@ export const warnAboutDeprecatedSetNativeProps = false; export const enableEventAPI = false; export const enableJSXTransformAPI = false; export const warnAboutMissingMockScheduler = false; +export const warnAboutUnactedEffectsinDEV = true; // 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 b1265132955a..2d3500745e74 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -28,6 +28,7 @@ export const disableYielding = false; export const enableEventAPI = true; export const enableJSXTransformAPI = true; export const warnAboutMissingMockScheduler = true; +export const warnAboutUnactedEffectsinDEV = true; // 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 3ee472739745..d558f1673697 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -73,6 +73,8 @@ export const enableJSXTransformAPI = true; export const warnAboutMissingMockScheduler = true; +export const warnAboutUnactedEffectsinDEV = true; + // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars type Check<_X, Y: _X, X: Y = _X> = null;