resumeMountClassComponent should check for mount lifecycles, not update (#12371)
We have other tests that would have caught this if resuming were enabled in all cases, but since it's currently only enabled for error boundaries, the test I've added to prevent a regression is a bit contrived.
Andrew Clark committed
Mar 13, 2018 at 16:36 UTC
9d24a81054aae6686e5ea341d493c0b705687019
2 files changed
+51
-8
packages/react-reconciler/src/ReactFiberClassComponent.js
+8
-8
@@ -789,20 +789,20 @@ export default function(
789
// In order to support react-lifecycles-compat polyfilled components,
790
// Unsafe lifecycles should not be invoked for any component with the new gDSFP.
791
if (
792
- (typeof instance.UNSAFE_componentWillUpdate === 'function' ||
793
- typeof instance.componentWillUpdate === 'function') &&
792
+ (typeof instance.UNSAFE_componentWillMount === 'function' ||
793
+ typeof instance.componentWillMount === 'function') &&
794
typeof ctor.getDerivedStateFromProps !== 'function'
795
) {
796
- startPhaseTimer(workInProgress, 'componentWillUpdate');
797
- if (typeof instance.componentWillUpdate === 'function') {
798
- instance.componentWillUpdate(newProps, newState, newContext);
796
+ startPhaseTimer(workInProgress, 'componentWillMount');
797
+ if (typeof instance.componentWillMount === 'function') {
798
+ instance.componentWillMount();
799
}
800
- if (typeof instance.UNSAFE_componentWillUpdate === 'function') {
801
- instance.UNSAFE_componentWillUpdate(newProps, newState, newContext);
800
+ if (typeof instance.UNSAFE_componentWillMount === 'function') {
801
+ instance.UNSAFE_componentWillMount();
802
}
803
stopPhaseTimer();
804
}
805
- if (typeof instance.componentDidUpdate === 'function') {
805
+ if (typeof instance.componentDidMount === 'function') {
806
workInProgress.effectTag |= Update;
807
}
808
} else {
packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js
+43
@@ -1259,4 +1259,47 @@ describe('ReactIncrementalErrorHandling', () => {
1259
]);
1260
expect(ReactNoop.getChildren()).toEqual([span('Caught an error: oops!')]);
1261
});
1262
+
1263
+ it('calls the correct lifecycles on the error boundary after catching an error (mixed)', () => {
1264
+ // This test seems a bit contrived, but it's based on an actual regression
1265
+ // where we checked for the existence of didUpdate instead of didMount, and
1266
+ // didMount was not defined.
1267
+ function BadRender() {
1268
+ ReactNoop.yield('throw');
1269
+ throw new Error('oops!');
1270
+ }
1271
+
1272
+ let parent;
1273
+ class Parent extends React.Component {
1274
+ state = {error: null, other: false};
1275
+ componentDidCatch(error) {
1276
+ ReactNoop.yield('did catch');
1277
+ this.setState({error});
1278
+ }
1279
+ componentDidUpdate() {
1280
+ ReactNoop.yield('did update');
1281
+ }
1282
+ render() {
1283
+ parent = this;
1284
+ if (this.state.error) {
1285
+ ReactNoop.yield('render error message');
1286
+ return <span prop={`Caught an error: ${this.state.error.message}`} />;
1287
+ }
1288
+ ReactNoop.yield('render');
1289
+ return <BadRender />;
1290
+ }
1291
+ }
1292
+
1293
+ ReactNoop.render(<Parent step={1} />);
1294
+ ReactNoop.flushThrough(['render', 'throw']);
1295
+ expect(ReactNoop.getChildren()).toEqual([]);
1296
+
1297
+ parent.setState({other: true});
1298
+ expect(ReactNoop.flush()).toEqual([
1299
+ 'did catch',
1300
+ 'render error message',
1301
+ 'did update',
1302
+ ]);
1303
+ expect(ReactNoop.getChildren()).toEqual([span('Caught an error: oops!')]);
1304
+ });
1305
});