@samitouri / QOS-React-2 / commits / d3b183c323

Debug render-phase side effects in "strict" mode (#12094)

A new feature flag has been added, debugRenderPhaseSideEffectsForStrictMode. When enabled, StrictMode subtrees will also double-invoke lifecycles in the same way as debugRenderPhaseSideEffects. By default, this flag is enabled for __DEV__ only. Internally we can toggle it with a GK. This breaks several of our incremental tests which make use of the noop-renderer. Updating the tests to account for the double-rendering in development mode makes them significantly more complicated. The most straight forward fix for this will be to convert them to be run as internal tests only. I believe this is reasonable since we are the only people making use of the noop renderer.

Brian Vaughn committed Jan 25, 2018 at 14:30 UTC d3b183c32326cacc29efea43ca9300a17ed4aca0
18 files changed +231 -15
packages/react-call-return/src/__tests__/ReactCallReturn-test.internal.js renamed
+3
@@ -10,12 +10,15 @@
10 'use strict';
11
12 let React;
13 +let ReactFeatureFlags;
14 let ReactNoop;
15 let ReactCallReturn;
16
17 describe('ReactCallReturn', () => {
18 beforeEach(() => {
19 jest.resetModules();
20 + ReactFeatureFlags = require('shared/ReactFeatureFlags');
21 + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
22 React = require('react');
23 ReactNoop = require('react-noop-renderer');
24 ReactCallReturn = require('react-call-return');
packages/react-reconciler/src/ReactFiberBeginWork.js
+15 -4
@@ -40,7 +40,10 @@ import {
40 Ref,
41 } from 'shared/ReactTypeOfSideEffect';
42 import {ReactCurrentOwner} from 'shared/ReactGlobalSharedState';
43 -import {debugRenderPhaseSideEffects} from 'shared/ReactFeatureFlags';
43 +import {
44 + debugRenderPhaseSideEffects,
45 + debugRenderPhaseSideEffectsForStrictMode,
46 +} from 'shared/ReactFeatureFlags';
47 import invariant from 'fbjs/lib/invariant';
48 import getComponentName from 'shared/getComponentName';
49 import warning from 'fbjs/lib/warning';
@@ -64,7 +67,7 @@ import {
67 } from './ReactFiberContext';
68 import {pushProvider} from './ReactFiberNewContext';
69 import {NoWork, Never} from './ReactFiberExpirationTime';
67 -import {AsyncUpdates} from './ReactTypeOfInternalContext';
70 +import {AsyncUpdates, StrictMode} from './ReactTypeOfInternalContext';
71 import MAX_SIGNED_31_BIT_INT from './maxSigned31BitInt';
72
73 let didWarnAboutBadClass;
@@ -287,12 +290,20 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
290 if (__DEV__) {
291 ReactDebugCurrentFiber.setCurrentPhase('render');
292 nextChildren = instance.render();
290 - if (debugRenderPhaseSideEffects) {
293 + if (
294 + debugRenderPhaseSideEffects ||
295 + (debugRenderPhaseSideEffectsForStrictMode &&
296 + workInProgress.internalContextTag & StrictMode)
297 + ) {
298 instance.render();
299 }
300 ReactDebugCurrentFiber.setCurrentPhase(null);
301 } else {
295 - if (debugRenderPhaseSideEffects) {
302 + if (
303 + debugRenderPhaseSideEffects ||
304 + (debugRenderPhaseSideEffectsForStrictMode &&
305 + workInProgress.internalContextTag & StrictMode)
306 + ) {
307 instance.render();
308 }
309 nextChildren = instance.render();
packages/react-reconciler/src/ReactFiberClassComponent.js
+11 -2
@@ -13,6 +13,7 @@ import type {ExpirationTime} from './ReactFiberExpirationTime';
13 import {Update} from 'shared/ReactTypeOfSideEffect';
14 import {
15 debugRenderPhaseSideEffects,
16 + debugRenderPhaseSideEffectsForStrictMode,
17 enableAsyncSubtreeAPI,
18 warnAboutDeprecatedLifecycles,
19 } from 'shared/ReactFeatureFlags';
@@ -391,7 +392,11 @@ export default function(
392 : emptyObject;
393
394 // Instantiate twice to help detect side-effects.
394 - if (debugRenderPhaseSideEffects) {
395 + if (
396 + debugRenderPhaseSideEffects ||
397 + (debugRenderPhaseSideEffectsForStrictMode &&
398 + workInProgress.internalContextTag & StrictMode)
399 + ) {
400 new ctor(props, context); // eslint-disable-line no-new
401 }
402
@@ -537,7 +542,11 @@ export default function(
542 }
543 }
544
540 - if (debugRenderPhaseSideEffects) {
545 + if (
546 + debugRenderPhaseSideEffects ||
547 + (debugRenderPhaseSideEffectsForStrictMode &&
548 + workInProgress.internalContextTag & StrictMode)
549 + ) {
550 // Invoke method an extra time to help detect side-effects.
551 type.getDerivedStateFromProps.call(
552 null,
packages/react-reconciler/src/ReactFiberUpdateQueue.js
+16 -9
@@ -10,11 +10,15 @@
10 import type {Fiber} from './ReactFiber';
11 import type {ExpirationTime} from './ReactFiberExpirationTime';
12
13 -import {debugRenderPhaseSideEffects} from 'shared/ReactFeatureFlags';
13 +import {
14 + debugRenderPhaseSideEffects,
15 + debugRenderPhaseSideEffectsForStrictMode,
16 +} from 'shared/ReactFeatureFlags';
17 import {Callback as CallbackEffect} from 'shared/ReactTypeOfSideEffect';
18 import {ClassComponent, HostRoot} from 'shared/ReactTypeOfWork';
19 import invariant from 'fbjs/lib/invariant';
20 import warning from 'fbjs/lib/warning';
21 +import {StrictMode} from './ReactTypeOfInternalContext';
22
23 import {NoWork} from './ReactFiberExpirationTime';
24
@@ -183,14 +187,7 @@ export function getUpdateExpirationTime(fiber: Fiber): ExpirationTime {
187 function getStateFromUpdate(update, instance, prevState, props) {
188 const partialState = update.partialState;
189 if (typeof partialState === 'function') {
186 - const updateFn = partialState;
187 -
188 - // Invoke setState callback an extra time to help detect side-effects.
189 - if (debugRenderPhaseSideEffects) {
190 - updateFn.call(instance, prevState, props);
191 - }
192 -
193 - return updateFn.call(instance, prevState, props);
190 + return partialState.call(instance, prevState, props);
191 } else {
192 return partialState;
193 }
@@ -276,6 +273,16 @@ export function processUpdateQueue<State>(
273 }
274 }
275
276 + // Invoke setState callback an extra time to help detect side-effects.
277 + // Ignore the return value in this case.
278 + if (
279 + debugRenderPhaseSideEffects ||
280 + (debugRenderPhaseSideEffectsForStrictMode &&
281 + workInProgress.internalContextTag & StrictMode)
282 + ) {
283 + getStateFromUpdate(update, instance, state, props);
284 + }
285 +
286 // Process the update
287 let partialState;
288 if (update.isReplace) {
packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js renamed
+3
@@ -11,12 +11,15 @@
11 'use strict';
12
13 let React;
14 +let ReactFeatureFlags;
15 let ReactNoop;
16 let PropTypes;
17
18 describe('ReactIncremental', () => {
19 beforeEach(() => {
20 jest.resetModules();
21 + ReactFeatureFlags = require('shared/ReactFeatureFlags');
22 + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
23 React = require('react');
24 ReactNoop = require('react-noop-renderer');
25 PropTypes = require('prop-types');
packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js renamed
+3
@@ -11,12 +11,15 @@
11 'use strict';
12
13 let PropTypes;
14 +let ReactFeatureFlags;
15 let React;
16 let ReactNoop;
17
18 describe('ReactIncrementalErrorHandling', () => {
19 beforeEach(() => {
20 jest.resetModules();
21 + ReactFeatureFlags = require('shared/ReactFeatureFlags');
22 + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
23 PropTypes = require('prop-types');
24 React = require('react');
25 ReactNoop = require('react-noop-renderer');
packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.internal.js
+3
@@ -11,11 +11,14 @@
11 'use strict';
12
13 let React;
14 +let ReactFeatureFlags;
15 let ReactNoop;
16
17 describe('ReactIncrementalErrorLogging', () => {
18 beforeEach(() => {
19 jest.resetModules();
20 + ReactFeatureFlags = require('shared/ReactFeatureFlags');
21 + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
22 React = require('react');
23 ReactNoop = require('react-noop-renderer');
24 });
packages/react-reconciler/src/__tests__/ReactIncrementalReflection-test.internal.js renamed
+3
@@ -11,11 +11,14 @@
11 'use strict';
12
13 let React;
14 +let ReactFeatureFlags;
15 let ReactNoop;
16
17 describe('ReactIncrementalReflection', () => {
18 beforeEach(() => {
19 jest.resetModules();
20 + ReactFeatureFlags = require('shared/ReactFeatureFlags');
21 + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
22 React = require('react');
23 ReactNoop = require('react-noop-renderer');
24 });
packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.internal.js renamed
+3
@@ -11,11 +11,14 @@
11 'use strict';
12
13 let React;
14 +let ReactFeatureFlags;
15 let ReactNoop;
16
17 describe('ReactIncrementalScheduling', () => {
18 beforeEach(() => {
19 jest.resetModules();
20 + ReactFeatureFlags = require('shared/ReactFeatureFlags');
21 + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
22 React = require('react');
23 ReactNoop = require('react-noop-renderer');
24 });
packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.internal.js renamed
+3
@@ -11,11 +11,14 @@
11 'use strict';
12
13 let React;
14 +let ReactFeatureFlags;
15 let ReactNoop;
16
17 describe('ReactIncrementalSideEffects', () => {
18 beforeEach(() => {
19 jest.resetModules();
20 + ReactFeatureFlags = require('shared/ReactFeatureFlags');
21 + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
22 React = require('react');
23 ReactNoop = require('react-noop-renderer');
24 });
packages/react-reconciler/src/__tests__/ReactIncrementalTriangle-test.internal.js renamed
+3
@@ -11,11 +11,14 @@
11 'use strict';
12
13 let React;
14 +let ReactFeatureFlags;
15 let ReactNoop;
16
17 describe('ReactIncrementalTriangle', () => {
18 beforeEach(() => {
19 jest.resetModules();
20 + ReactFeatureFlags = require('shared/ReactFeatureFlags');
21 + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
22 React = require('react');
23 ReactNoop = require('react-noop-renderer');
24 });
packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js renamed
+3
@@ -11,11 +11,14 @@
11 'use strict';
12
13 let React;
14 +let ReactFeatureFlags;
15 let ReactNoop;
16
17 describe('ReactIncrementalUpdates', () => {
18 beforeEach(() => {
19 jest.resetModuleRegistry();
20 + ReactFeatureFlags = require('shared/ReactFeatureFlags');
21 + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
22 React = require('react');
23 ReactNoop = require('react-noop-renderer');
24 });
packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js
+1
@@ -20,6 +20,7 @@ describe('ReactNewContext', () => {
20 beforeEach(() => {
21 jest.resetModules();
22 ReactFeatureFlags = require('shared/ReactFeatureFlags');
23 + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
24 ReactFeatureFlags.enableNewContextAPI = true;
25 React = require('react');
26 ReactNoop = require('react-noop-renderer');
packages/react/src/__tests__/ReactStrictMode-test.internal.js
+152
@@ -140,6 +140,158 @@ describe('ReactStrictMode', () => {
140 });
141 });
142
143 + [true, false].forEach(debugRenderPhaseSideEffectsForStrictMode => {
144 + describe(`StrictMode (${debugRenderPhaseSideEffectsForStrictMode})`, () => {
145 + beforeEach(() => {
146 + jest.resetModules();
147 + ReactFeatureFlags = require('shared/ReactFeatureFlags');
148 + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = debugRenderPhaseSideEffectsForStrictMode;
149 + React = require('react');
150 + ReactTestRenderer = require('react-test-renderer');
151 + });
152 +
153 + it('should invoke precommit lifecycle methods twice in DEV', () => {
154 + const {StrictMode} = React;
155 +
156 + let log = [];
157 + let shouldComponentUpdate = false;
158 +
159 + function Root() {
160 + return (
161 + <StrictMode>
162 + <ClassComponent />
163 + </StrictMode>
164 + );
165 + }
166 +
167 + class ClassComponent extends React.Component {
168 + state = {};
169 + static getDerivedStateFromProps() {
170 + log.push('getDerivedStateFromProps');
171 + return null;
172 + }
173 + constructor(props) {
174 + super(props);
175 + log.push('constructor');
176 + }
177 + componentDidMount() {
178 + log.push('componentDidMount');
179 + }
180 + componentDidUpdate() {
181 + log.push('componentDidUpdate');
182 + }
183 + componentWillUnmount() {
184 + log.push('componentWillUnmount');
185 + }
186 + shouldComponentUpdate() {
187 + log.push('shouldComponentUpdate');
188 + return shouldComponentUpdate;
189 + }
190 + render() {
191 + log.push('render');
192 + return null;
193 + }
194 + }
195 +
196 + const component = ReactTestRenderer.create(<Root />);
197 +
198 + if (debugRenderPhaseSideEffectsForStrictMode) {
199 + expect(log).toEqual([
200 + 'constructor',
201 + 'constructor',
202 + 'getDerivedStateFromProps',
203 + 'getDerivedStateFromProps',
204 + 'render',
205 + 'render',
206 + 'componentDidMount',
207 + ]);
208 + } else {
209 + expect(log).toEqual([
210 + 'constructor',
211 + 'getDerivedStateFromProps',
212 + 'render',
213 + 'componentDidMount',
214 + ]);
215 + }
216 +
217 + log = [];
218 + shouldComponentUpdate = true;
219 +
220 + component.update(<Root />);
221 + if (debugRenderPhaseSideEffectsForStrictMode) {
222 + expect(log).toEqual([
223 + 'getDerivedStateFromProps',
224 + 'getDerivedStateFromProps',
225 + 'shouldComponentUpdate',
226 + 'render',
227 + 'render',
228 + 'componentDidUpdate',
229 + ]);
230 + } else {
231 + expect(log).toEqual([
232 + 'getDerivedStateFromProps',
233 + 'shouldComponentUpdate',
234 + 'render',
235 + 'componentDidUpdate',
236 + ]);
237 + }
238 +
239 + log = [];
240 + shouldComponentUpdate = false;
241 +
242 + component.update(<Root />);
243 + if (debugRenderPhaseSideEffectsForStrictMode) {
244 + expect(log).toEqual([
245 + 'getDerivedStateFromProps',
246 + 'getDerivedStateFromProps',
247 + 'shouldComponentUpdate',
248 + ]);
249 + } else {
250 + expect(log).toEqual([
251 + 'getDerivedStateFromProps',
252 + 'shouldComponentUpdate',
253 + ]);
254 + }
255 + });
256 +
257 + it('should invoke setState callbacks twice in DEV', () => {
258 + const {StrictMode} = React;
259 +
260 + let instance;
261 + class ClassComponent extends React.Component {
262 + state = {
263 + count: 1,
264 + };
265 + render() {
266 + instance = this;
267 + return null;
268 + }
269 + }
270 +
271 + let setStateCount = 0;
272 +
273 + ReactTestRenderer.create(
274 + <StrictMode>
275 + <ClassComponent />
276 + </StrictMode>,
277 + );
278 + instance.setState(state => {
279 + setStateCount++;
280 + return {
281 + count: state.count + 1,
282 + };
283 + });
284 +
285 + // Callback should be invoked twice (in DEV)
286 + expect(setStateCount).toBe(
287 + debugRenderPhaseSideEffectsForStrictMode ? 2 : 1,
288 + );
289 + // But each time `state` should be the previous value
290 + expect(instance.state.count).toBe(2);
291 + });
292 + });
293 + });
294 +
295 describe('async subtree', () => {
296 beforeEach(() => {
297 jest.resetModules();
packages/shared/ReactFeatureFlags.js
+6
@@ -26,6 +26,12 @@ export const enableNewContextAPI = false;
26 // Helps identify side effects in begin-phase lifecycle hooks and setState reducers:
27 export const debugRenderPhaseSideEffects = false;
28
29 +// In some cases, StrictMode should also double-render lifecycles.
30 +// This can be confusing for tests though,
31 +// And it can be bad for performance in production.
32 +// This feature flag can be used to control the behavior:
33 +export const debugRenderPhaseSideEffectsForStrictMode = __DEV__;
34 +
35 // Warn about deprecated, async-unsafe lifecycles; relates to RFC #6:
36 export const warnAboutDeprecatedLifecycles = false;
37
packages/shared/forks/ReactFeatureFlags.native-fabric.js
+1
@@ -13,6 +13,7 @@ import typeof * as FeatureFlagsType from 'shared/ReactFeatureFlags';
13 import typeof * as FabricFeatureFlagsType from './ReactFeatureFlags.native-fabric';
14
15 export const debugRenderPhaseSideEffects = false;
16 +export const debugRenderPhaseSideEffectsForStrictMode = false;
17 export const enableAsyncSubtreeAPI = true;
18 export const enableCreateRoot = false;
19 export const enableUserTimingAPI = __DEV__;
packages/shared/forks/ReactFeatureFlags.native.js
+1
@@ -15,6 +15,7 @@ import typeof * as FeatureFlagsShimType from './ReactFeatureFlags.native';
15 // Re-export dynamic flags from the fbsource version.
16 export const {
17 debugRenderPhaseSideEffects,
18 + debugRenderPhaseSideEffectsForStrictMode,
19 warnAboutDeprecatedLifecycles,
20 } = require('ReactFeatureFlags');
21
packages/shared/forks/ReactFeatureFlags.www.js
+1
@@ -13,6 +13,7 @@ import typeof * as FeatureFlagsShimType from './ReactFeatureFlags.www';
13 // Re-export dynamic flags from the www version.
14 export const {
15 debugRenderPhaseSideEffects,
16 + debugRenderPhaseSideEffectsForStrictMode,
17 warnAboutDeprecatedLifecycles,
18 } = require('ReactFeatureFlags');
19