Replace console.error() with a throw in setTimeout() as last resort exception logging (#13310)
* Add a regression test for #13188 * Replace console.error() with a throw in setTimeout() as last resort * Fix lint and comment * Fix tests to check we throw after all * Fix build tests
Dan Abramov committed
Aug 2, 2018 at 18:16 UTC
46d5afc54dbd4d44b370f0a066021678d3a332ac
4 files changed
+94
-6
packages/react-dom/src/__tests__/ReactErrorLoggingRecovery-test.js
new
+79
@@ -0,0 +1,79 @@
1
+/**
2
+ * Copyright (c) 2013-present, Facebook, Inc.
3
+ *
4
+ * This source code is licensed under the MIT license found in the
5
+ * LICENSE file in the root directory of this source tree.
6
+ *
7
+ * @emails react-core
8
+ * @jest-environment node
9
+ */
10
+
11
+// This is a regression test for https://github.com/facebook/react/issues/13188.
12
+// It reproduces a combination of conditions that led to a problem.
13
+
14
+if (global.window) {
15
+ throw new Error('This test must run in a Node environment.');
16
+}
17
+
18
+// The issue only reproduced when React was loaded before JSDOM.
19
+const React = require('react');
20
+const ReactDOM = require('react-dom');
21
+
22
+// Unlike other tests, we want to enable error logging.
23
+// Note this is not a real Error prototype property,
24
+// it's only set in our Jest environment.
25
+// eslint-disable-next-line no-extend-native
26
+Error.prototype.suppressReactErrorLogging = false;
27
+
28
+// Initialize JSDOM separately.
29
+// We don't use our normal JSDOM setup because we want to load React first.
30
+const {JSDOM} = require('jsdom');
31
+global.requestAnimationFrame = setTimeout;
32
+global.cancelAnimationFrame = clearTimeout;
33
+const jsdom = new JSDOM(`<div id="app-root"></div>`);
34
+global.window = jsdom.window;
35
+global.document = jsdom.window.document;
36
+global.navigator = jsdom.window.navigator;
37
+
38
+class Bad extends React.Component {
39
+ componentDidUpdate() {
40
+ throw new Error('no');
41
+ }
42
+ render() {
43
+ return null;
44
+ }
45
+}
46
+
47
+describe('ReactErrorLoggingRecovery', () => {
48
+ let originalConsoleError = console.error;
49
+
50
+ beforeEach(() => {
51
+ console.error = error => {
52
+ throw new Error('Buggy console.error');
53
+ };
54
+ });
55
+
56
+ afterEach(() => {
57
+ console.error = originalConsoleError;
58
+ });
59
+
60
+ it('should recover from errors in console.error', function() {
61
+ const div = document.createElement('div');
62
+ let didCatch = false;
63
+ try {
64
+ ReactDOM.render(<Bad />, div);
65
+ ReactDOM.render(<Bad />, div);
66
+ } catch (e) {
67
+ expect(e.message).toBe('no');
68
+ didCatch = true;
69
+ }
70
+ expect(didCatch).toBe(true);
71
+ ReactDOM.render(<span>Hello</span>, div);
72
+ expect(div.firstChild.textContent).toBe('Hello');
73
+
74
+ // Verify the console.error bug is surfaced
75
+ expect(() => {
76
+ jest.runAllTimers();
77
+ }).toThrow('Buggy console.error');
78
+ });
79
+});
packages/react-reconciler/src/ReactFiberCommitWork.js
+6
-1
@@ -116,7 +116,12 @@ export function logError(boundary: Fiber, errorInfo: CapturedValue<mixed>) {
116
// A cycle may still occur if logCapturedError renders a component that throws.
117
const suppressLogging = e && e.suppressReactErrorLogging;
118
if (!suppressLogging) {
119
- console.error(e);
119
+ // Rethrow it from a clean stack because this function is assumed to never throw.
120
+ // We can't safely call console.error() here because it could *also* throw if overriden.
121
+ // https://github.com/facebook/react/issues/13188
122
+ setTimeout(() => {
123
+ throw e;
124
+ });
125
}
126
}
127
}
packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.internal.js
+4
-5
@@ -144,11 +144,10 @@ describe('ReactIncrementalErrorLogging', () => {
144
145
expect(logCapturedErrorCalls.length).toBe(1);
146
147
- // The error thrown in logCapturedError should also be logged
148
- expect(console.error).toHaveBeenCalledTimes(1);
149
- expect(console.error.calls.argsFor(0)[0].message).toContain(
150
- 'logCapturedError error',
151
- );
147
+ // The error thrown in logCapturedError should be rethrown with a clean stack
148
+ expect(() => {
149
+ jest.runAllTimers();
150
+ }).toThrow('logCapturedError error');
151
} finally {
152
jest.unmock('../ReactFiberErrorLogger');
153
}
scripts/jest/config.build.js
+5
@@ -10,6 +10,11 @@ const packages = readdirSync(packagesRoot).filter(dir => {
10
if (dir.charAt(0) === '.') {
11
return false;
12
}
13
+ if (dir === 'events') {
14
+ // There's an actual Node package called "events"
15
+ // that's used by jsdom so we don't want to alias that.
16
+ return false;
17
+ }
18
const packagePath = join(packagesRoot, dir, 'package.json');
19
return statSync(packagePath).isFile();
20
});