Disallow forced sync updates during begin phase
Stack allows this, but in Fiber it may cause an infinite loop. Instead we'll print a warning and defer the update by giving it Task priority.
Andrew Clark committed
Dec 28, 2016 at 21:28 UTC
ce8c978d6305e22bf27ea9754cc4176a0eabd4ef
6 files changed
+75
-5
src/renderers/shared/__tests__/ReactPerf-test.js
+17
-2
@@ -16,6 +16,7 @@ describe('ReactPerf', () => {
16
var ReactDOM;
17
var ReactPerf;
18
var ReactTestUtils;
19
+ var ReactDOMFeatureFlags;
20
var emptyFunction;
21
22
var App;
@@ -38,6 +39,7 @@ describe('ReactPerf', () => {
39
ReactDOM = require('ReactDOM');
40
ReactPerf = require('ReactPerf');
41
ReactTestUtils = require('ReactTestUtils');
42
+ ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
43
emptyFunction = require('emptyFunction');
44
45
App = class extends React.Component {
@@ -614,7 +616,7 @@ describe('ReactPerf', () => {
616
}
617
}
618
class EvilPortal extends React.Component {
617
- componentWillMount() {
619
+ componentDidMount() {
620
var portalContainer = document.createElement('div');
621
ReactDOM.render(<Evil />, portalContainer);
622
}
@@ -645,6 +647,10 @@ describe('ReactPerf', () => {
647
var container = document.createElement('div');
648
var thrownErr = new Error('Muhaha!');
649
650
+ if (ReactDOMFeatureFlags.useFiber) {
651
+ spyOn(console, 'error');
652
+ }
653
+
654
class Evil extends React.Component {
655
componentWillMount() {
656
throw thrownErr;
@@ -679,6 +685,15 @@ describe('ReactPerf', () => {
685
}
686
ReactDOM.unmountComponentAtNode(container);
687
ReactPerf.stop();
688
+
689
+ if (ReactDOMFeatureFlags.useFiber) {
690
+ // A sync `render` inside cWM will print a warning. That should be the
691
+ // only warning.
692
+ expect(console.error.calls.count()).toEqual(1);
693
+ expect(console.error.calls.argsFor(0)[0]).toMatch(
694
+ /Render methods should be a pure function of props and state/
695
+ );
696
+ }
697
});
698
699
it('should not print errant warnings if portal throws in componentDidMount()', () => {
@@ -694,7 +709,7 @@ describe('ReactPerf', () => {
709
}
710
}
711
class EvilPortal extends React.Component {
697
- componentWillMount() {
712
+ componentDidMount() {
713
var portalContainer = document.createElement('div');
714
ReactDOM.render(<Evil />, portalContainer);
715
}
src/renderers/shared/fiber/ReactFiberScheduler.js
+12
-2
@@ -69,6 +69,7 @@ var {
69
if (__DEV__) {
70
var ReactFiberInstrumentation = require('ReactFiberInstrumentation');
71
var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber');
72
+ var warning = require('warning');
73
}
74
75
var timeHeuristicForUnitOfWork = 1;
@@ -1029,8 +1030,17 @@ module.exports = function<T, P, I, TI, C, CX>(config : HostConfig<T, P, I, TI, C
1030
}
1031
1032
function scheduleUpdate(fiber : Fiber, priorityLevel : PriorityLevel) {
1032
- // If we're in a batch, downgrade sync priority to task priority
1033
- if (priorityLevel === SynchronousPriority && isPerformingWork) {
1033
+ // Detect if a synchronous update is made during render (or begin phase).
1034
+ if (priorityLevel === SynchronousPriority && isPerformingWork && !isCommitting) {
1035
+ if (__DEV__) {
1036
+ warning(
1037
+ false,
1038
+ 'Render methods should be a pure function of props and state; ' +
1039
+ 'triggering nested component updates from render is not allowed. ' +
1040
+ 'If necessary, trigger nested updates in componentDidUpdate.'
1041
+ );
1042
+ }
1043
+ // Downgrade to Task priority to prevent an infinite loop.
1044
priorityLevel = TaskPriority;
1045
}
1046
src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js
+10
@@ -340,4 +340,14 @@ describe('ReactIncrementalScheduling', () => {
340
// animation priority.
341
expect(ReactNoop.getChildren()).toEqual([span(1)]);
342
});
343
+
344
+ it('can force synchronous updates with syncUpdates, even inside batchedUpdates', done => {
345
+ ReactNoop.batchedUpdates(() => {
346
+ ReactNoop.syncUpdates(() => {
347
+ ReactNoop.render(<span />);
348
+ expect(ReactNoop.getChildren()).toEqual([span()]);
349
+ done();
350
+ });
351
+ });
352
+ });
353
});
src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js
+13
@@ -18,6 +18,7 @@ describe('ReactComponentTreeHook', () => {
18
var ReactInstanceMap;
19
var ReactComponentTreeHook;
20
var ReactComponentTreeTestUtils;
21
+ var ReactDOMFeatureFlags;
22
23
beforeEach(() => {
24
jest.resetModules();
@@ -28,6 +29,7 @@ describe('ReactComponentTreeHook', () => {
29
ReactInstanceMap = require('ReactInstanceMap');
30
ReactComponentTreeHook = require('ReactComponentTreeHook');
31
ReactComponentTreeTestUtils = require('ReactComponentTreeTestUtils');
32
+ ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
33
});
34
35
function assertTreeMatches(pairs) {
@@ -1841,6 +1843,9 @@ describe('ReactComponentTreeHook', () => {
1843
// https://github.com/facebook/react/issues/7187
1844
var el = document.createElement('div');
1845
var portalEl = document.createElement('div');
1846
+ if (ReactDOMFeatureFlags.useFiber) {
1847
+ spyOn(console, 'error');
1848
+ }
1849
class Foo extends React.Component {
1850
componentWillMount() {
1851
ReactDOM.render(<div />, portalEl);
@@ -1850,6 +1855,14 @@ describe('ReactComponentTreeHook', () => {
1855
}
1856
}
1857
ReactDOM.render(<Foo />, el);
1858
+ if (ReactDOMFeatureFlags.useFiber) {
1859
+ // A sync `render` inside cWM will print a warning. That should be the
1860
+ // only warning.
1861
+ expect(console.error.calls.count()).toEqual(1);
1862
+ expect(console.error.calls.argsFor(0)[0]).toMatch(
1863
+ /Render methods should be a pure function of props and state/
1864
+ );
1865
+ }
1866
});
1867
1868
it('is created when calling renderToString during render', () => {
src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js
+1
-1
@@ -1266,7 +1266,7 @@ describe('ReactCompositeComponent', () => {
1266
var layer = document.createElement('div');
1267
1268
class Component extends React.Component {
1269
- componentWillMount() {
1269
+ componentDidMount() {
1270
ReactDOM.render(<div />, layer);
1271
}
1272
src/renderers/shared/shared/__tests__/ReactUpdates-test.js
+22
@@ -1142,4 +1142,26 @@ describe('ReactUpdates', () => {
1142
expect(container.textContent).toBe('goodbye');
1143
expect(mounts).toBe(1);
1144
});
1145
+
1146
+ it('mounts and unmounts are sync even in a batch', () => {
1147
+ var container1 = document.createElement('div');
1148
+ var container2 = document.createElement('div');
1149
+
1150
+ let called = false;
1151
+ class Foo extends React.Component {
1152
+ componentDidMount() {
1153
+ called = true;
1154
+ ReactDOM.render(<div>Hello</div>, container2);
1155
+ expect(container2.textContent).toEqual('Hello');
1156
+ ReactDOM.unmountComponentAtNode(container2);
1157
+ expect(container2.textContent).toEqual('');
1158
+ }
1159
+ render() {
1160
+ return <div>{this.props.step}</div>;
1161
+ }
1162
+ }
1163
+
1164
+ ReactDOM.render(<Foo />, container1);
1165
+ expect(called).toEqual(true);
1166
+ });
1167
});