[fail] Only warn on unacted effects for strict / non sync modes (#16041)
* only warn on unacted effects for strict / non sync modes (basically, when `fiber.mode !== 0b0000`) Warnings on unacted effects may be too noisy, especially for legacy apps. This PR fires the warning only when in a non sync mode (concurrent/batched), or when in strict mode. This should make updating easier. I also added batched mode tests to the act() suite. * explicitly check for modes before warning, explicit tests for all modes
Sunil Pai committed
Jul 3, 2019 at 01:29 UTC
bd846459d6c96e6caa64c44e0fb42696db839447
4 files changed
+89
-27
packages/react-dom/src/__tests__/ReactDOMHooks-test.js
+17
-26
@@ -53,32 +53,23 @@ describe('ReactDOMHooks', () => {
53
return 3 * n;
54
}
55
56
- // we explicitly catch the missing act() warnings
57
- // to simulate this tricky repro
58
- expect(() => {
59
- ReactDOM.render(<Example1 n={1} />, container);
60
- expect(container.textContent).toBe('1');
61
- expect(container2.textContent).toBe('');
62
- expect(container3.textContent).toBe('');
63
- Scheduler.unstable_flushAll();
64
- expect(container.textContent).toBe('1');
65
- expect(container2.textContent).toBe('2');
66
- expect(container3.textContent).toBe('3');
67
-
68
- ReactDOM.render(<Example1 n={2} />, container);
69
- expect(container.textContent).toBe('2');
70
- expect(container2.textContent).toBe('2'); // Not flushed yet
71
- expect(container3.textContent).toBe('3'); // Not flushed yet
72
- Scheduler.unstable_flushAll();
73
- expect(container.textContent).toBe('2');
74
- expect(container2.textContent).toBe('4');
75
- expect(container3.textContent).toBe('6');
76
- }).toWarnDev([
77
- 'An update to Example1 ran an effect',
78
- 'An update to Example2 ran an effect',
79
- 'An update to Example1 ran an effect',
80
- 'An update to Example2 ran an effect',
81
- ]);
56
+ ReactDOM.render(<Example1 n={1} />, container);
57
+ expect(container.textContent).toBe('1');
58
+ expect(container2.textContent).toBe('');
59
+ expect(container3.textContent).toBe('');
60
+ Scheduler.unstable_flushAll();
61
+ expect(container.textContent).toBe('1');
62
+ expect(container2.textContent).toBe('2');
63
+ expect(container3.textContent).toBe('3');
64
+
65
+ ReactDOM.render(<Example1 n={2} />, container);
66
+ expect(container.textContent).toBe('2');
67
+ expect(container2.textContent).toBe('2'); // Not flushed yet
68
+ expect(container3.textContent).toBe('3'); // Not flushed yet
69
+ Scheduler.unstable_flushAll();
70
+ expect(container.textContent).toBe('2');
71
+ expect(container2.textContent).toBe('4');
72
+ expect(container3.textContent).toBe('6');
73
});
74
75
it('should not bail out when an update is scheduled from within an event handler', () => {
packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
+67
@@ -48,6 +48,73 @@ describe('ReactTestUtils.act()', () => {
48
ReactDOM.unmountComponentAtNode(dom);
49
}
50
runActTests('legacy sync mode', renderSync, unmountSync);
51
+
52
+ // and then in batched mode
53
+ let batchedRoot;
54
+ function renderBatched(el, dom) {
55
+ batchedRoot = ReactDOM.unstable_createSyncRoot(dom);
56
+ batchedRoot.render(el);
57
+ }
58
+ function unmountBatched(dom) {
59
+ if (batchedRoot !== null) {
60
+ batchedRoot.unmount();
61
+ batchedRoot = null;
62
+ }
63
+ }
64
+ runActTests('batched mode', renderBatched, unmountBatched);
65
+
66
+ describe('unacted effects', () => {
67
+ function App() {
68
+ React.useEffect(() => {}, []);
69
+ return null;
70
+ }
71
+
72
+ it('does not warn in legacy sync mode', () => {
73
+ expect(() => {
74
+ ReactDOM.render(<App />, document.createElement('div'));
75
+ }).toWarnDev([]);
76
+ });
77
+
78
+ it('warns in strict mode', () => {
79
+ expect(() => {
80
+ ReactDOM.render(
81
+ <React.StrictMode>
82
+ <App />
83
+ </React.StrictMode>,
84
+ document.createElement('div'),
85
+ );
86
+ }).toWarnDev([
87
+ 'An update to App ran an effect, but was not wrapped in act(...)',
88
+ 'An update to App ran an effect, but was not wrapped in act(...)',
89
+ ]);
90
+ });
91
+
92
+ it('warns in batched mode', () => {
93
+ expect(() => {
94
+ const root = ReactDOM.unstable_createSyncRoot(
95
+ document.createElement('div'),
96
+ );
97
+ root.render(<App />);
98
+ Scheduler.unstable_flushAll();
99
+ }).toWarnDev([
100
+ 'An update to App ran an effect, but was not wrapped in act(...)',
101
+ 'An update to App ran an effect, but was not wrapped in act(...)',
102
+ ]);
103
+ });
104
+
105
+ it('warns in concurrent mode', () => {
106
+ expect(() => {
107
+ const root = ReactDOM.unstable_createRoot(
108
+ document.createElement('div'),
109
+ );
110
+ root.render(<App />);
111
+ Scheduler.unstable_flushAll();
112
+ }).toWarnDev([
113
+ 'An update to App ran an effect, but was not wrapped in act(...)',
114
+ 'An update to App ran an effect, but was not wrapped in act(...)',
115
+ ]);
116
+ });
117
+ });
118
});
119
120
function runActTests(label, render, unmount) {
packages/react-reconciler/src/ReactFiberWorkLoop.js
+5
@@ -61,6 +61,7 @@ import {
61
import {createWorkInProgress, assignFiberPropertiesInDEV} from './ReactFiber';
62
import {
63
NoMode,
64
+ StrictMode,
65
ProfileMode,
66
BatchedMode,
67
ConcurrentMode,
@@ -2453,6 +2454,10 @@ export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void {
2454
if (__DEV__) {
2455
if (
2456
warnsIfNotActing === true &&
2457
+ (fiber.mode & StrictMode ||
2458
+ fiber.mode & ProfileMode ||
2459
+ fiber.mode & BatchedMode ||
2460
+ fiber.mode & ConcurrentMode) &&
2461
IsSomeRendererActing.current === false &&
2462
IsThisRendererActing.current === false
2463
) {
packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js
-1
@@ -1806,7 +1806,6 @@ describe('ReactHooks', () => {
1806
globalListener();
1807
globalListener();
1808
}).toWarnDev([
1809
- 'An update to C ran an effect',
1809
'An update to C inside a test was not wrapped in act',
1810
'An update to C inside a test was not wrapped in act',
1811
// Note: should *not* warn about updates on unmounted component.