@samitouri / QOS-React-2 / commits / 02da938fd5

Don't double-invoke effects in legacy roots (#20028)

Large legacy applications are likely to be difficult to update to handle this feature, and it wouldn't add any value– since newer APIs that require this resilience are not legacy compatible.

Brian Vaughn committed Oct 15, 2020 at 08:40 UTC 02da938fd51f6345ca185d571ea6628189ae81a7
6 files changed +193 -56
packages/react-reconciler/src/ReactFiberClassComponent.new.js
+28 -5
@@ -30,7 +30,13 @@ import invariant from 'shared/invariant';
30 import {REACT_CONTEXT_TYPE, REACT_PROVIDER_TYPE} from 'shared/ReactSymbols';
31
32 import {resolveDefaultProps} from './ReactFiberLazyComponent.new';
33 -import {DebugTracingMode, StrictMode} from './ReactTypeOfMode';
33 +import {
34 + BlockingMode,
35 + ConcurrentMode,
36 + DebugTracingMode,
37 + NoMode,
38 + StrictMode,
39 +} from './ReactTypeOfMode';
40
41 import {
42 enqueueUpdate,
@@ -891,7 +897,12 @@ function mountClassInstance(
897 }
898
899 if (typeof instance.componentDidMount === 'function') {
894 - if (__DEV__ && enableDoubleInvokingEffects) {
900 + if (
901 + __DEV__ &&
902 + enableDoubleInvokingEffects &&
903 + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode
904 + ) {
905 + // Never double-invoke effects for legacy roots.
906 workInProgress.flags |= MountLayoutDev | Update;
907 } else {
908 workInProgress.flags |= Update;
@@ -965,7 +976,11 @@ function resumeMountClassInstance(
976 // If an update was already in progress, we should schedule an Update
977 // effect even though we're bailing out, so that cWU/cDU are called.
978 if (typeof instance.componentDidMount === 'function') {
968 - if (__DEV__ && enableDoubleInvokingEffects) {
979 + if (
980 + __DEV__ &&
981 + enableDoubleInvokingEffects &&
982 + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode
983 + ) {
984 workInProgress.flags |= MountLayoutDev | Update;
985 } else {
986 workInProgress.flags |= Update;
@@ -1012,7 +1027,11 @@ function resumeMountClassInstance(
1027 }
1028 }
1029 if (typeof instance.componentDidMount === 'function') {
1015 - if (__DEV__ && enableDoubleInvokingEffects) {
1030 + if (
1031 + __DEV__ &&
1032 + enableDoubleInvokingEffects &&
1033 + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode
1034 + ) {
1035 workInProgress.flags |= MountLayoutDev | Update;
1036 } else {
1037 workInProgress.flags |= Update;
@@ -1022,7 +1041,11 @@ function resumeMountClassInstance(
1041 // If an update was already in progress, we should schedule an Update
1042 // effect even though we're bailing out, so that cWU/cDU are called.
1043 if (typeof instance.componentDidMount === 'function') {
1025 - if (__DEV__ && enableDoubleInvokingEffects) {
1044 + if (
1045 + __DEV__ &&
1046 + enableDoubleInvokingEffects &&
1047 + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode
1048 + ) {
1049 workInProgress.flags |= MountLayoutDev | Update;
1050 } else {
1051 workInProgress.flags |= Update;
packages/react-reconciler/src/ReactFiberCommitWork.new.js
+8
@@ -2024,6 +2024,8 @@ function commitPassiveMount(
2024
2025 function invokeLayoutEffectMountInDEV(fiber: Fiber): void {
2026 if (__DEV__ && enableDoubleInvokingEffects) {
2027 + // We don't need to re-check for legacy roots here.
2028 + // This function will not be called within legacy roots.
2029 switch (fiber.tag) {
2030 case FunctionComponent:
2031 case ForwardRef:
@@ -2057,6 +2059,8 @@ function invokeLayoutEffectMountInDEV(fiber: Fiber): void {
2059
2060 function invokePassiveEffectMountInDEV(fiber: Fiber): void {
2061 if (__DEV__ && enableDoubleInvokingEffects) {
2062 + // We don't need to re-check for legacy roots here.
2063 + // This function will not be called within legacy roots.
2064 switch (fiber.tag) {
2065 case FunctionComponent:
2066 case ForwardRef:
@@ -2081,6 +2085,8 @@ function invokePassiveEffectMountInDEV(fiber: Fiber): void {
2085
2086 function invokeLayoutEffectUnmountInDEV(fiber: Fiber): void {
2087 if (__DEV__ && enableDoubleInvokingEffects) {
2088 + // We don't need to re-check for legacy roots here.
2089 + // This function will not be called within legacy roots.
2090 switch (fiber.tag) {
2091 case FunctionComponent:
2092 case ForwardRef:
@@ -2113,6 +2119,8 @@ function invokeLayoutEffectUnmountInDEV(fiber: Fiber): void {
2119
2120 function invokePassiveEffectUnmountInDEV(fiber: Fiber): void {
2121 if (__DEV__ && enableDoubleInvokingEffects) {
2122 + // We don't need to re-check for legacy roots here.
2123 + // This function will not be called within legacy roots.
2124 switch (fiber.tag) {
2125 case FunctionComponent:
2126 case ForwardRef:
packages/react-reconciler/src/ReactFiberHooks.new.js
+26 -5
@@ -29,7 +29,12 @@ import {
29 enableDoubleInvokingEffects,
30 } from 'shared/ReactFeatureFlags';
31
32 -import {NoMode, BlockingMode, DebugTracingMode} from './ReactTypeOfMode';
32 +import {
33 + NoMode,
34 + BlockingMode,
35 + ConcurrentMode,
36 + DebugTracingMode,
37 +} from './ReactTypeOfMode';
38 import {
39 NoLane,
40 NoLanes,
@@ -485,7 +490,11 @@ export function bailoutHooks(
490 lanes: Lanes,
491 ) {
492 workInProgress.updateQueue = current.updateQueue;
488 - if (__DEV__ && enableDoubleInvokingEffects) {
493 + if (
494 + __DEV__ &&
495 + enableDoubleInvokingEffects &&
496 + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode
497 + ) {
498 workInProgress.flags &= ~(
499 MountPassiveDevEffect |
500 PassiveEffect |
@@ -1253,7 +1262,11 @@ function mountEffect(
1262 }
1263 }
1264
1256 - if (__DEV__ && enableDoubleInvokingEffects) {
1265 + if (
1266 + __DEV__ &&
1267 + enableDoubleInvokingEffects &&
1268 + (currentlyRenderingFiber.mode & (BlockingMode | ConcurrentMode)) !== NoMode
1269 + ) {
1270 return mountEffectImpl(
1271 MountPassiveDevEffect | PassiveEffect | PassiveStaticEffect,
1272 HookPassive,
@@ -1287,7 +1300,11 @@ function mountLayoutEffect(
1300 create: () => (() => void) | void,
1301 deps: Array<mixed> | void | null,
1302 ): void {
1290 - if (__DEV__ && enableDoubleInvokingEffects) {
1303 + if (
1304 + __DEV__ &&
1305 + enableDoubleInvokingEffects &&
1306 + (currentlyRenderingFiber.mode & (BlockingMode | ConcurrentMode)) !== NoMode
1307 + ) {
1308 return mountEffectImpl(
1309 MountLayoutDevEffect | UpdateEffect,
1310 HookLayout,
@@ -1355,7 +1372,11 @@ function mountImperativeHandle<T>(
1372 const effectDeps =
1373 deps !== null && deps !== undefined ? deps.concat([ref]) : null;
1374
1358 - if (__DEV__ && enableDoubleInvokingEffects) {
1375 + if (
1376 + __DEV__ &&
1377 + enableDoubleInvokingEffects &&
1378 + (currentlyRenderingFiber.mode & (BlockingMode | ConcurrentMode)) !== NoMode
1379 + ) {
1380 return mountEffectImpl(
1381 MountLayoutDevEffect | UpdateEffect,
1382 HookLayout,
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+7
@@ -2874,6 +2874,11 @@ function commitDoubleInvokeEffectsInDEV(
2874 hasPassiveEffects: boolean,
2875 ) {
2876 if (__DEV__ && enableDoubleInvokingEffects) {
2877 + // Never double-invoke effects for legacy roots.
2878 + if ((fiber.mode & (BlockingMode | ConcurrentMode)) === NoMode) {
2879 + return;
2880 + }
2881 +
2882 setCurrentDebugFiberInDEV(fiber);
2883 invokeEffectsInDev(fiber, MountLayoutDev, invokeLayoutEffectUnmountInDEV);
2884 if (hasPassiveEffects) {
@@ -2898,6 +2903,8 @@ function invokeEffectsInDev(
2903 invokeEffectFn: (fiber: Fiber) => void,
2904 ): void {
2905 if (__DEV__ && enableDoubleInvokingEffects) {
2906 + // We don't need to re-check for legacy roots here.
2907 + // This function will not be called within legacy roots.
2908 let fiber = firstChild;
2909 while (fiber !== null) {
2910 if (fiber.child !== null) {
packages/react-reconciler/src/__tests__/ReactDoubleInvokeEvents-test.internal.js
+119 -45
@@ -11,17 +11,44 @@
11
12 let React;
13 let ReactFeatureFlags;
14 -let ReactNoop;
14 +let ReactTestRenderer;
15 let Scheduler;
16 +let act;
17
18 describe('ReactDoubleInvokeEvents', () => {
19 beforeEach(() => {
20 jest.resetModules();
21 React = require('react');
22 ReactFeatureFlags = require('shared/ReactFeatureFlags');
22 - ReactNoop = require('react-noop-renderer');
23 + ReactTestRenderer = require('react-test-renderer');
24 Scheduler = require('scheduler');
25 ReactFeatureFlags.enableDoubleInvokingEffects = __VARIANT__;
26 + act = ReactTestRenderer.unstable_concurrentAct;
27 + });
28 +
29 + it('should not double invoke effects in legacy mode', () => {
30 + function App({text}) {
31 + React.useEffect(() => {
32 + Scheduler.unstable_yieldValue('useEffect mount');
33 + return () => Scheduler.unstable_yieldValue('useEffect unmount');
34 + });
35 +
36 + React.useLayoutEffect(() => {
37 + Scheduler.unstable_yieldValue('useLayoutEffect mount');
38 + return () => Scheduler.unstable_yieldValue('useLayoutEffect unmount');
39 + });
40 +
41 + return text;
42 + }
43 +
44 + act(() => {
45 + ReactTestRenderer.create(<App text={'mount'} />);
46 + });
47 +
48 + expect(Scheduler).toHaveYielded([
49 + 'useLayoutEffect mount',
50 + 'useEffect mount',
51 + ]);
52 });
53
54 it('double invoking for effects works properly', () => {
@@ -38,8 +65,12 @@ describe('ReactDoubleInvokeEvents', () => {
65
66 return text;
67 }
41 - ReactNoop.act(() => {
42 - ReactNoop.render(<App text={'mount'} />);
68 +
69 + let renderer;
70 + act(() => {
71 + renderer = ReactTestRenderer.create(<App text={'mount'} />, {
72 + unstable_isConcurrent: true,
73 + });
74 });
75
76 if (__DEV__ && __VARIANT__) {
@@ -58,8 +89,8 @@ describe('ReactDoubleInvokeEvents', () => {
89 ]);
90 }
91
61 - ReactNoop.act(() => {
62 - ReactNoop.render(<App text={'update'} />);
92 + act(() => {
93 + renderer.update(<App text={'update'} />);
94 });
95
96 expect(Scheduler).toHaveYielded([
@@ -69,8 +100,8 @@ describe('ReactDoubleInvokeEvents', () => {
100 'useEffect mount',
101 ]);
102
72 - ReactNoop.act(() => {
73 - ReactNoop.render(null);
103 + act(() => {
104 + renderer.unmount();
105 });
106
107 expect(Scheduler).toHaveYielded([
@@ -94,8 +125,11 @@ describe('ReactDoubleInvokeEvents', () => {
125 return text;
126 }
127
97 - ReactNoop.act(() => {
98 - ReactNoop.render(<App text={'mount'} />);
128 + let renderer;
129 + act(() => {
130 + renderer = ReactTestRenderer.create(<App text={'mount'} />, {
131 + unstable_isConcurrent: true,
132 + });
133 });
134
135 if (__DEV__ && __VARIANT__) {
@@ -114,8 +148,8 @@ describe('ReactDoubleInvokeEvents', () => {
148 ]);
149 }
150
117 - ReactNoop.act(() => {
118 - ReactNoop.render(<App text={'update'} />);
151 + act(() => {
152 + renderer.update(<App text={'update'} />);
153 });
154
155 expect(Scheduler).toHaveYielded([
@@ -125,8 +159,8 @@ describe('ReactDoubleInvokeEvents', () => {
159 'useEffect Two mount',
160 ]);
161
128 - ReactNoop.act(() => {
129 - ReactNoop.render(null);
162 + act(() => {
163 + renderer.unmount(null);
164 });
165
166 expect(Scheduler).toHaveYielded([
@@ -152,8 +186,11 @@ describe('ReactDoubleInvokeEvents', () => {
186 return text;
187 }
188
155 - ReactNoop.act(() => {
156 - ReactNoop.render(<App text={'mount'} />);
189 + let renderer;
190 + act(() => {
191 + renderer = ReactTestRenderer.create(<App text={'mount'} />, {
192 + unstable_isConcurrent: true,
193 + });
194 });
195
196 if (__DEV__ && __VARIANT__) {
@@ -172,8 +209,8 @@ describe('ReactDoubleInvokeEvents', () => {
209 ]);
210 }
211
175 - ReactNoop.act(() => {
176 - ReactNoop.render(<App text={'update'} />);
212 + act(() => {
213 + renderer.update(<App text={'update'} />);
214 });
215
216 expect(Scheduler).toHaveYielded([
@@ -183,8 +220,8 @@ describe('ReactDoubleInvokeEvents', () => {
220 'useLayoutEffect Two mount',
221 ]);
222
186 - ReactNoop.act(() => {
187 - ReactNoop.render(null);
223 + act(() => {
224 + renderer.unmount();
225 });
226
227 expect(Scheduler).toHaveYielded([
@@ -206,8 +243,11 @@ describe('ReactDoubleInvokeEvents', () => {
243 return text;
244 }
245
209 - ReactNoop.act(() => {
210 - ReactNoop.render(<App text={'mount'} />);
246 + let renderer;
247 + act(() => {
248 + renderer = ReactTestRenderer.create(<App text={'mount'} />, {
249 + unstable_isConcurrent: true,
250 + });
251 });
252
253 if (__DEV__ && __VARIANT__) {
@@ -224,8 +264,8 @@ describe('ReactDoubleInvokeEvents', () => {
264 ]);
265 }
266
227 - ReactNoop.act(() => {
228 - ReactNoop.render(<App text={'update'} />);
267 + act(() => {
268 + renderer.update(<App text={'update'} />);
269 });
270
271 expect(Scheduler).toHaveYielded([
@@ -233,8 +273,8 @@ describe('ReactDoubleInvokeEvents', () => {
273 'useEffect mount',
274 ]);
275
236 - ReactNoop.act(() => {
237 - ReactNoop.render(null);
276 + act(() => {
277 + renderer.unmount();
278 });
279
280 expect(Scheduler).toHaveYielded([]);
@@ -264,8 +304,8 @@ describe('ReactDoubleInvokeEvents', () => {
304 }
305 }
306
267 - ReactNoop.act(() => {
268 - ReactNoop.render(<App />);
307 + act(() => {
308 + ReactTestRenderer.create(<App />, {unstable_isConcurrent: true});
309 });
310
311 if (__DEV__ && __VARIANT__) {
@@ -298,8 +338,11 @@ describe('ReactDoubleInvokeEvents', () => {
338 }
339 }
340
301 - ReactNoop.act(() => {
302 - ReactNoop.render(<App text={'mount'} />);
341 + let renderer;
342 + act(() => {
343 + renderer = ReactTestRenderer.create(<App text={'mount'} />, {
344 + unstable_isConcurrent: true,
345 + });
346 });
347
348 if (__DEV__ && __VARIANT__) {
@@ -312,19 +355,45 @@ describe('ReactDoubleInvokeEvents', () => {
355 expect(Scheduler).toHaveYielded(['componentDidMount']);
356 }
357
315 - ReactNoop.act(() => {
316 - ReactNoop.render(<App text={'update'} />);
358 + act(() => {
359 + renderer.update(<App text={'update'} />);
360 });
361
362 expect(Scheduler).toHaveYielded(['componentDidUpdate']);
363
321 - ReactNoop.act(() => {
322 - ReactNoop.render(null);
364 + act(() => {
365 + renderer.unmount();
366 });
367
368 expect(Scheduler).toHaveYielded(['componentWillUnmount']);
369 });
370
371 + it('should not double invoke class lifecycles in legacy mode', () => {
372 + class App extends React.PureComponent {
373 + componentDidMount() {
374 + Scheduler.unstable_yieldValue('componentDidMount');
375 + }
376 +
377 + componentDidUpdate() {
378 + Scheduler.unstable_yieldValue('componentDidUpdate');
379 + }
380 +
381 + componentWillUnmount() {
382 + Scheduler.unstable_yieldValue('componentWillUnmount');
383 + }
384 +
385 + render() {
386 + return this.props.text;
387 + }
388 + }
389 +
390 + act(() => {
391 + ReactTestRenderer.create(<App text={'mount'} />);
392 + });
393 +
394 + expect(Scheduler).toHaveYielded(['componentDidMount']);
395 + });
396 +
397 it('double flushing passive effects only results in one double invoke', () => {
398 function App({text}) {
399 const [state, setState] = React.useState(0);
@@ -345,8 +414,10 @@ describe('ReactDoubleInvokeEvents', () => {
414 return text;
415 }
416
348 - ReactNoop.act(() => {
349 - ReactNoop.render(<App text={'mount'} />);
417 + act(() => {
418 + ReactTestRenderer.create(<App text={'mount'} />, {
419 + unstable_isConcurrent: true,
420 + });
421 });
422
423 if (__DEV__ && __VARIANT__) {
@@ -410,8 +481,8 @@ describe('ReactDoubleInvokeEvents', () => {
481 return showChild && <Child />;
482 }
483
413 - ReactNoop.act(() => {
414 - ReactNoop.render(<App />);
484 + act(() => {
485 + ReactTestRenderer.create(<App />, {unstable_isConcurrent: true});
486 });
487
488 if (__DEV__ && __VARIANT__) {
@@ -430,7 +501,7 @@ describe('ReactDoubleInvokeEvents', () => {
501 ]);
502 }
503
433 - ReactNoop.act(() => {
504 + act(() => {
505 _setShowChild(true);
506 });
507
@@ -495,8 +566,11 @@ describe('ReactDoubleInvokeEvents', () => {
566 );
567 }
568
498 - ReactNoop.act(() => {
499 - ReactNoop.render(<App text={'mount'} />);
569 + let renderer;
570 + act(() => {
571 + renderer = ReactTestRenderer.create(<App text={'mount'} />, {
572 + unstable_isConcurrent: true,
573 + });
574 });
575
576 if (__DEV__ && __VARIANT__) {
@@ -519,8 +593,8 @@ describe('ReactDoubleInvokeEvents', () => {
593 ]);
594 }
595
522 - ReactNoop.act(() => {
523 - ReactNoop.render(<App text={'mount'} />);
596 + act(() => {
597 + renderer.update(<App text={'mount'} />);
598 });
599
600 expect(Scheduler).toHaveYielded([
@@ -530,8 +604,8 @@ describe('ReactDoubleInvokeEvents', () => {
604 'useEffect mount',
605 ]);
606
533 - ReactNoop.act(() => {
534 - ReactNoop.render(null);
607 + act(() => {
608 + renderer.unmount();
609 });
610
611 expect(Scheduler).toHaveYielded([
packages/react/src/__tests__/ReactProfiler-test.internal.js
+5 -1
@@ -4180,7 +4180,11 @@ describe('Profiler', () => {
4180 const interactions = SchedulerTracing.unstable_getCurrent();
4181 expect(interactions.size).toBe(1);
4182 interaction = Array.from(interactions)[0];
4183 - ReactTestRenderer.create(<Component />);
4183 + ReactTestRendererAct(() => {
4184 + ReactTestRenderer.create(<Component />, {
4185 + unstable_isConcurrent: true,
4186 + });
4187 + });
4188 },
4189 );
4190 Scheduler.unstable_flushAll();