From 4097038d07e90c354166df747ff2dcecb8edd3c8 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 11 Jan 2017 15:59:11 -0800 Subject: [PATCH 1/7] Log all Fiber errors w/ component stack A new module has been added (ReactFiberErrorLogger). By default this module just logs error information (call stack and component stack) to the console to make errors easier to debug. In the future, perhaps we will enable users to inject their own handler for custom processing/logging. --- scripts/fiber/tests-passing.txt | 3 + .../shared/fiber/ReactFiberErrorLogger.js | 26 +++++ .../shared/fiber/ReactFiberScheduler.js | 52 ++++++++-- .../ReactIncrementalErrorHandling-test.js | 94 +++++++++++++++++++ 4 files changed, 165 insertions(+), 10 deletions(-) create mode 100644 src/renderers/shared/fiber/ReactFiberErrorLogger.js diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 8370d371050f..70ffdf911504 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1194,6 +1194,9 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js * does not interrupt unmounting if detaching a ref throws * handles error thrown by host config while working on failed root * handles error thrown by top-level callback +* should log errors that occur during the begin phase +* should log errors that occur during the commit phase +* should ignore errors thrown in log method to prevent cycle src/renderers/shared/fiber/__tests__/ReactIncrementalReflection-test.js * handles isMounted even when the initial render is deferred diff --git a/src/renderers/shared/fiber/ReactFiberErrorLogger.js b/src/renderers/shared/fiber/ReactFiberErrorLogger.js new file mode 100644 index 000000000000..fb1fe9598ea1 --- /dev/null +++ b/src/renderers/shared/fiber/ReactFiberErrorLogger.js @@ -0,0 +1,26 @@ +/** + * Copyright 2013-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + * @providesModule ReactFiberErrorLogger + * @flow + */ + +'use strict'; + +import type { CapturedError } from 'ReactFiberScheduler'; + +function logCapturedError(capturedError : CapturedError) : void { + if (__DEV__) { + // console.log rather than console.error to avoid breaking tests + // (Jest complains about unexpected console.error calls.) + console.log(capturedError.error); + console.log(`Error location: ${capturedError.componentStack}`); + } +} + +exports.logCapturedError = logCapturedError; diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index f92be3bf10b1..0b5edbe23288 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -17,10 +17,19 @@ import type { FiberRoot } from 'ReactFiberRoot'; import type { HostConfig, Deadline } from 'ReactFiberReconciler'; import type { PriorityLevel } from 'ReactPriorityLevel'; +export type CapturedError = { + componentStack: string, + error: Error, +}; + var { popContextProvider, } = require('ReactFiberContext'); const { reset } = require('ReactFiberStack'); +var { + getStackAddendumByWorkInProgressFiber, +} = require('ReactComponentTreeHook'); +var { logCapturedError } = require('ReactFiberErrorLogger'); var ReactFiberBeginWork = require('ReactFiberBeginWork'); var ReactFiberCompleteWork = require('ReactFiberCompleteWork'); @@ -134,7 +143,7 @@ module.exports = function(config : HostConfig | null = null; + let capturedErrors : Map | null = null; // Keep track of which fibers have failed during the current batch of work. // This is a different set than capturedErrors, because it is not reset until // the end of the batch. This is needed to propagate errors correctly if a @@ -876,15 +885,24 @@ module.exports = function(config : HostConfig(config : HostConfig { beforeEach(() => { jest.resetModules(); + jest.mock('ReactFiberErrorLogger'); React = require('React'); ReactNoop = require('ReactNoop'); }); @@ -935,4 +937,96 @@ describe('ReactIncrementalErrorHandling', () => { }); expect(() => ReactNoop.flush()).toThrow('Error!'); }); + + if (ReactDOMFeatureFlags.useFiber) { + describe('ReactFiberErrorLogger', () => { + function normalizeCodeLocInfo(str) { + return str && str.replace(/\(at .+?:\d+\)/g, '(at **)'); + } + + let logCapturedErrorCalls; + let ReactFiberErrorLogger; + + beforeEach(() => { + logCapturedErrorCalls = []; + + ReactFiberErrorLogger = require('ReactFiberErrorLogger'); + ReactFiberErrorLogger.logCapturedError.mockImplementation( + (capturedError) => { + logCapturedErrorCalls.push(capturedError); + } + ); + }); + + it('should log errors that occur during the begin phase', () => { + class ErrorThrowingComponent extends React.Component { + componentWillMount() { + throw Error('componentWillMount error'); + } + render() { + return
; + } + } + + try { + ReactNoop.render(
); + ReactNoop.flushDeferredPri(); + } catch (error) {} + + expect(logCapturedErrorCalls.length).toBe(1); + expect(logCapturedErrorCalls[0].error.message).toBe('componentWillMount error'); + expect(normalizeCodeLocInfo(logCapturedErrorCalls[0].componentStack)).toContain( + ' in ErrorThrowingComponent (at **)\n' + + ' in span (at **)\n' + + ' in div (at **)' + ); + }); + + it('should log errors that occur during the commit phase', () => { + class ErrorThrowingComponent extends React.Component { + componentDidMount() { + throw Error('componentDidMount error'); + } + render() { + return
; + } + } + + try { + ReactNoop.render(
); + ReactNoop.flushDeferredPri(); + } catch (error) {} + + expect(logCapturedErrorCalls.length).toBe(1); + expect(logCapturedErrorCalls[0].error.message).toBe('componentDidMount error'); + expect(normalizeCodeLocInfo(logCapturedErrorCalls[0].componentStack)).toContain( + ' in ErrorThrowingComponent (at **)\n' + + ' in span (at **)\n' + + ' in div (at **)' + ); + }); + + it('should ignore errors thrown in log method to prevent cycle', () => { + class ErrorThrowingComponent extends React.Component { + render() { + throw Error('render error'); + } + } + + ReactFiberErrorLogger.logCapturedError.mockImplementation( + (capturedError) => { + logCapturedErrorCalls.push(capturedError); + throw Error('logCapturedError error'); + } + ); + + try { + ReactNoop.render(
); + ReactNoop.flushDeferredPri(); + } catch (error) {} + + expect(logCapturedErrorCalls.length).toBe(1); + }); + }); + } }); From 52a725cbdc3dfa2121df9c7805ddfb9f4d71e6ae Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 12 Jan 2017 09:58:24 -0800 Subject: [PATCH 2/7] Removed unnecessary conditional in ReactIncrementalErrorHandling-test --- .../ReactIncrementalErrorHandling-test.js | 151 +++++++++--------- 1 file changed, 74 insertions(+), 77 deletions(-) diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js index 2966bdc53c41..41c17ad94946 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js @@ -11,7 +11,6 @@ 'use strict'; -var ReactDOMFeatureFlags = require('ReactDOMFeatureFlags'); var React; var ReactNoop; @@ -938,95 +937,93 @@ describe('ReactIncrementalErrorHandling', () => { expect(() => ReactNoop.flush()).toThrow('Error!'); }); - if (ReactDOMFeatureFlags.useFiber) { - describe('ReactFiberErrorLogger', () => { - function normalizeCodeLocInfo(str) { - return str && str.replace(/\(at .+?:\d+\)/g, '(at **)'); - } + describe('ReactFiberErrorLogger', () => { + function normalizeCodeLocInfo(str) { + return str && str.replace(/\(at .+?:\d+\)/g, '(at **)'); + } - let logCapturedErrorCalls; - let ReactFiberErrorLogger; + let logCapturedErrorCalls; + let ReactFiberErrorLogger; - beforeEach(() => { - logCapturedErrorCalls = []; + beforeEach(() => { + logCapturedErrorCalls = []; - ReactFiberErrorLogger = require('ReactFiberErrorLogger'); - ReactFiberErrorLogger.logCapturedError.mockImplementation( - (capturedError) => { - logCapturedErrorCalls.push(capturedError); - } - ); - }); + ReactFiberErrorLogger = require('ReactFiberErrorLogger'); + ReactFiberErrorLogger.logCapturedError.mockImplementation( + (capturedError) => { + logCapturedErrorCalls.push(capturedError); + } + ); + }); - it('should log errors that occur during the begin phase', () => { - class ErrorThrowingComponent extends React.Component { - componentWillMount() { - throw Error('componentWillMount error'); - } - render() { - return
; - } + it('should log errors that occur during the begin phase', () => { + class ErrorThrowingComponent extends React.Component { + componentWillMount() { + throw Error('componentWillMount error'); } + render() { + return
; + } + } - try { - ReactNoop.render(
); - ReactNoop.flushDeferredPri(); - } catch (error) {} - - expect(logCapturedErrorCalls.length).toBe(1); - expect(logCapturedErrorCalls[0].error.message).toBe('componentWillMount error'); - expect(normalizeCodeLocInfo(logCapturedErrorCalls[0].componentStack)).toContain( - ' in ErrorThrowingComponent (at **)\n' + - ' in span (at **)\n' + - ' in div (at **)' - ); - }); + try { + ReactNoop.render(
); + ReactNoop.flushDeferredPri(); + } catch (error) {} + + expect(logCapturedErrorCalls.length).toBe(1); + expect(logCapturedErrorCalls[0].error.message).toBe('componentWillMount error'); + expect(normalizeCodeLocInfo(logCapturedErrorCalls[0].componentStack)).toContain( + ' in ErrorThrowingComponent (at **)\n' + + ' in span (at **)\n' + + ' in div (at **)' + ); + }); - it('should log errors that occur during the commit phase', () => { - class ErrorThrowingComponent extends React.Component { - componentDidMount() { - throw Error('componentDidMount error'); - } - render() { - return
; - } + it('should log errors that occur during the commit phase', () => { + class ErrorThrowingComponent extends React.Component { + componentDidMount() { + throw Error('componentDidMount error'); } + render() { + return
; + } + } - try { - ReactNoop.render(
); - ReactNoop.flushDeferredPri(); - } catch (error) {} - - expect(logCapturedErrorCalls.length).toBe(1); - expect(logCapturedErrorCalls[0].error.message).toBe('componentDidMount error'); - expect(normalizeCodeLocInfo(logCapturedErrorCalls[0].componentStack)).toContain( - ' in ErrorThrowingComponent (at **)\n' + - ' in span (at **)\n' + - ' in div (at **)' - ); - }); + try { + ReactNoop.render(
); + ReactNoop.flushDeferredPri(); + } catch (error) {} + + expect(logCapturedErrorCalls.length).toBe(1); + expect(logCapturedErrorCalls[0].error.message).toBe('componentDidMount error'); + expect(normalizeCodeLocInfo(logCapturedErrorCalls[0].componentStack)).toContain( + ' in ErrorThrowingComponent (at **)\n' + + ' in span (at **)\n' + + ' in div (at **)' + ); + }); - it('should ignore errors thrown in log method to prevent cycle', () => { - class ErrorThrowingComponent extends React.Component { - render() { - throw Error('render error'); - } + it('should ignore errors thrown in log method to prevent cycle', () => { + class ErrorThrowingComponent extends React.Component { + render() { + throw Error('render error'); } + } - ReactFiberErrorLogger.logCapturedError.mockImplementation( - (capturedError) => { - logCapturedErrorCalls.push(capturedError); - throw Error('logCapturedError error'); - } - ); + ReactFiberErrorLogger.logCapturedError.mockImplementation( + (capturedError) => { + logCapturedErrorCalls.push(capturedError); + throw Error('logCapturedError error'); + } + ); - try { - ReactNoop.render(
); - ReactNoop.flushDeferredPri(); - } catch (error) {} + try { + ReactNoop.render(
); + ReactNoop.flushDeferredPri(); + } catch (error) {} - expect(logCapturedErrorCalls.length).toBe(1); - }); + expect(logCapturedErrorCalls.length).toBe(1); }); - } + }); }); From 1b408ec2f3df8de2f59a5fb05f00684de0f69dea Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 12 Jan 2017 10:24:48 -0800 Subject: [PATCH 3/7] Error logger uses console.error; improved message/format --- .../shared/fiber/ReactFiberErrorLogger.js | 15 ++++++++++----- src/renderers/shared/fiber/ReactFiberScheduler.js | 5 +++++ .../ReactIncrementalErrorHandling-test.js | 8 ++++++++ 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberErrorLogger.js b/src/renderers/shared/fiber/ReactFiberErrorLogger.js index fb1fe9598ea1..6c8c5261431d 100644 --- a/src/renderers/shared/fiber/ReactFiberErrorLogger.js +++ b/src/renderers/shared/fiber/ReactFiberErrorLogger.js @@ -8,7 +8,7 @@ * * @providesModule ReactFiberErrorLogger * @flow - */ + */3 'use strict'; @@ -16,11 +16,16 @@ import type { CapturedError } from 'ReactFiberScheduler'; function logCapturedError(capturedError : CapturedError) : void { if (__DEV__) { - // console.log rather than console.error to avoid breaking tests - // (Jest complains about unexpected console.error calls.) - console.log(capturedError.error); - console.log(`Error location: ${capturedError.componentStack}`); + const { componentName, componentStack, error } = capturedError; + // TODO Link to unstable_handleError() documentation once it exists. + console.error( + `React caught an error thrown by ${componentName}. ` + + `Consider using an error boundary to capture this and other errors.\n\n` + + `${error}\n\n` + + `The error was thrown in the following location: ${componentStack}` + ); } } exports.logCapturedError = logCapturedError; + diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 0b5edbe23288..427fdfdc4548 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -18,6 +18,7 @@ import type { HostConfig, Deadline } from 'ReactFiberReconciler'; import type { PriorityLevel } from 'ReactPriorityLevel'; export type CapturedError = { + componentName: string, componentStack: string, error: Error, }; @@ -36,6 +37,7 @@ var ReactFiberCompleteWork = require('ReactFiberCompleteWork'); var ReactFiberCommitWork = require('ReactFiberCommitWork'); var ReactFiberHostContext = require('ReactFiberHostContext'); var ReactCurrentOwner = require('ReactCurrentOwner'); +var getComponentName = require('getComponentName'); var { cloneFiber } = require('ReactFiber'); @@ -890,6 +892,7 @@ module.exports = function(config : HostConfig(config : HostConfig(config : HostConfig { expect(logCapturedErrorCalls.length).toBe(1); expect(logCapturedErrorCalls[0].error.message).toBe('componentWillMount error'); + expect(logCapturedErrorCalls[0].componentName).toBe('ErrorThrowingComponent'); expect(normalizeCodeLocInfo(logCapturedErrorCalls[0].componentStack)).toContain( ' in ErrorThrowingComponent (at **)\n' + ' in span (at **)\n' + @@ -997,6 +998,7 @@ describe('ReactIncrementalErrorHandling', () => { expect(logCapturedErrorCalls.length).toBe(1); expect(logCapturedErrorCalls[0].error.message).toBe('componentDidMount error'); + expect(logCapturedErrorCalls[0].componentName).toBe('ErrorThrowingComponent'); expect(normalizeCodeLocInfo(logCapturedErrorCalls[0].componentStack)).toContain( ' in ErrorThrowingComponent (at **)\n' + ' in span (at **)\n' + @@ -1005,6 +1007,8 @@ describe('ReactIncrementalErrorHandling', () => { }); it('should ignore errors thrown in log method to prevent cycle', () => { + spyOn(console, 'error'); + class ErrorThrowingComponent extends React.Component { render() { throw Error('render error'); @@ -1024,6 +1028,10 @@ describe('ReactIncrementalErrorHandling', () => { } catch (error) {} expect(logCapturedErrorCalls.length).toBe(1); + + // The error thrown in logCapturedError should also be logged + expect(console.error.calls.count()).toBe(1); + expect(console.error.calls.argsFor(0)[0].message).toContain('logCapturedError error'); }); }); }); From 28342af4caa8711b02100987413f41fc6f2f0f6b Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 12 Jan 2017 13:48:00 -0800 Subject: [PATCH 4/7] Imrpoved error message/instructions --- .../shared/fiber/ReactFiberErrorLogger.js | 48 ++++++++++++++--- .../shared/fiber/ReactFiberScheduler.js | 24 +++++++-- .../ReactIncrementalErrorHandling-test.js | 54 +++++++++++++++++++ 3 files changed, 116 insertions(+), 10 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberErrorLogger.js b/src/renderers/shared/fiber/ReactFiberErrorLogger.js index 6c8c5261431d..90c500bb2726 100644 --- a/src/renderers/shared/fiber/ReactFiberErrorLogger.js +++ b/src/renderers/shared/fiber/ReactFiberErrorLogger.js @@ -8,7 +8,7 @@ * * @providesModule ReactFiberErrorLogger * @flow - */3 + */ 'use strict'; @@ -16,15 +16,51 @@ import type { CapturedError } from 'ReactFiberScheduler'; function logCapturedError(capturedError : CapturedError) : void { if (__DEV__) { - const { componentName, componentStack, error } = capturedError; - // TODO Link to unstable_handleError() documentation once it exists. + const { + componentName, + componentStack, + error, + errorBoundaryName, + errorBoundaryFound, + willRetry, + } = capturedError; + + const componentNameMessage = componentName + ? `React caught an error thrown by ${componentName}.` + : 'React caught an error thrown by one of your components.'; + + let errorBoundaryMessage; + if (errorBoundaryFound) { + // errorBoundaryFound check is sufficient; errorBoundaryName check is to satisfy Flow. + if (willRetry) { + errorBoundaryMessage = + `This error will be handled by the error boundary ${errorBoundaryName || ''}. ` + + `React will try to recreate this component tree from scratch.`; + } else { + errorBoundaryMessage = + `This error was initially handled by the error boundary ${errorBoundaryName || ''}. ` + + `Recreating the tree from scratch failed so React will unmount the tree.`; + } + } else { + // TODO Link to unstable_handleError() documentation once it exists. + errorBoundaryMessage = + 'Consider adding an error boundary to your tree to customize error handling behavior.'; + } + console.error( - `React caught an error thrown by ${componentName}. ` + - `Consider using an error boundary to capture this and other errors.\n\n` + - `${error}\n\n` + + `${componentNameMessage} You should fix this error in your code.\n\n` + + `${errorBoundaryMessage}\n\n` + + `${error.stack}\n\n` + `The error was thrown in the following location: ${componentStack}` ); } + + if (!__DEV__) { + const { error } = capturedError; + console.error( + `React caught an error thrown by one of your components.\n\n${error.stack}` + ); + } } exports.logCapturedError = logCapturedError; diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 427fdfdc4548..658b41c15885 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -18,9 +18,12 @@ import type { HostConfig, Deadline } from 'ReactFiberReconciler'; import type { PriorityLevel } from 'ReactPriorityLevel'; export type CapturedError = { - componentName: string, - componentStack: string, - error: Error, + componentName : ?string, + componentStack : string, + error : Error, + errorBoundaryFound : boolean, + errorBoundaryName : ?string, + willRetry : boolean, }; var { @@ -825,6 +828,12 @@ module.exports = function(config : HostConfig(config : HostConfig(config : HostConfig(config : HostConfig(config : HostConfig { expect(logCapturedErrorCalls.length).toBe(1); expect(logCapturedErrorCalls[0].error.message).toBe('componentWillMount error'); expect(logCapturedErrorCalls[0].componentName).toBe('ErrorThrowingComponent'); + expect(logCapturedErrorCalls[0].errorBoundaryName).toBe(null); + expect(logCapturedErrorCalls[0].errorBoundaryFound).toBe(false); + expect(logCapturedErrorCalls[0].willRetry).toBe(false); expect(normalizeCodeLocInfo(logCapturedErrorCalls[0].componentStack)).toContain( ' in ErrorThrowingComponent (at **)\n' + ' in span (at **)\n' + @@ -999,6 +1002,9 @@ describe('ReactIncrementalErrorHandling', () => { expect(logCapturedErrorCalls.length).toBe(1); expect(logCapturedErrorCalls[0].error.message).toBe('componentDidMount error'); expect(logCapturedErrorCalls[0].componentName).toBe('ErrorThrowingComponent'); + expect(logCapturedErrorCalls[0].errorBoundaryName).toBe(null); + expect(logCapturedErrorCalls[0].errorBoundaryFound).toBe(false); + expect(logCapturedErrorCalls[0].willRetry).toBe(false); expect(normalizeCodeLocInfo(logCapturedErrorCalls[0].componentStack)).toContain( ' in ErrorThrowingComponent (at **)\n' + ' in span (at **)\n' + @@ -1006,6 +1012,54 @@ describe('ReactIncrementalErrorHandling', () => { ); }); + it('should relay info about error boundary and retry attempts if applicable', () => { + class ParentComponent extends React.Component { + render() { + return ; + } + } + + let handleErrorCalls = []; + let renderAttempts = 0; + + class ErrorBoundaryComponent extends React.Component { + unstable_handleError(error) { + handleErrorCalls.push(error); + this.setState({}); // Render again + } + render() { + return ; + } + } + + class ErrorThrowingComponent extends React.Component { + componentDidMount() { + throw Error('componentDidMount error'); + } + render() { + renderAttempts++; + return
; + } + } + + try { + ReactNoop.render(); + ReactNoop.flush(); + } catch (error) {} + + expect(renderAttempts).toBe(2); + expect(handleErrorCalls.length).toBe(1); + expect(logCapturedErrorCalls.length).toBe(2); + expect(logCapturedErrorCalls[0].error.message).toBe('componentDidMount error'); + expect(logCapturedErrorCalls[0].errorBoundaryName).toBe('ErrorBoundaryComponent'); + expect(logCapturedErrorCalls[0].errorBoundaryFound).toBe(true); + expect(logCapturedErrorCalls[0].willRetry).toBe(true); + expect(logCapturedErrorCalls[1].error.message).toBe('componentDidMount error'); + expect(logCapturedErrorCalls[1].errorBoundaryName).toBe('ErrorBoundaryComponent'); + expect(logCapturedErrorCalls[1].errorBoundaryFound).toBe(true); + expect(logCapturedErrorCalls[1].willRetry).toBe(false); + }); + it('should ignore errors thrown in log method to prevent cycle', () => { spyOn(console, 'error'); From 16139522047193e83c7ef5452ec305f05ee4a51d Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 12 Jan 2017 15:10:51 -0800 Subject: [PATCH 5/7] Globally mocked ReactFiberErrorLogger in Jest --- scripts/fiber/tests-passing.txt | 1 + scripts/jest/test-framework-setup.js | 4 ++++ src/renderers/shared/fiber/ReactFiberErrorLogger.js | 7 +++---- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 70ffdf911504..90562d88245c 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1196,6 +1196,7 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js * handles error thrown by top-level callback * should log errors that occur during the begin phase * should log errors that occur during the commit phase +* should relay info about error boundary and retry attempts if applicable * should ignore errors thrown in log method to prevent cycle src/renderers/shared/fiber/__tests__/ReactIncrementalReflection-test.js diff --git a/scripts/jest/test-framework-setup.js b/scripts/jest/test-framework-setup.js index 18543a82485f..6128ed6ed809 100644 --- a/scripts/jest/test-framework-setup.js +++ b/scripts/jest/test-framework-setup.js @@ -10,6 +10,10 @@ jest.mock('ReactDOMFeatureFlags', () => { }); }); +// Error logging varies between Fiber and Stack; +// Rather than fork dozens of tests, mock the error-logging file by default. +jest.mock('ReactFiberErrorLogger'); + var env = jasmine.getEnv(); var callCount = 0; diff --git a/src/renderers/shared/fiber/ReactFiberErrorLogger.js b/src/renderers/shared/fiber/ReactFiberErrorLogger.js index 90c500bb2726..53f9fd17200a 100644 --- a/src/renderers/shared/fiber/ReactFiberErrorLogger.js +++ b/src/renderers/shared/fiber/ReactFiberErrorLogger.js @@ -34,8 +34,8 @@ function logCapturedError(capturedError : CapturedError) : void { // errorBoundaryFound check is sufficient; errorBoundaryName check is to satisfy Flow. if (willRetry) { errorBoundaryMessage = - `This error will be handled by the error boundary ${errorBoundaryName || ''}. ` + - `React will try to recreate this component tree from scratch.`; + `React will try to recreate this component tree from scratch ` + + `using the error boundary you provided, ${errorBoundaryName || ''}.`; } else { errorBoundaryMessage = `This error was initially handled by the error boundary ${errorBoundaryName || ''}. ` + @@ -48,8 +48,7 @@ function logCapturedError(capturedError : CapturedError) : void { } console.error( - `${componentNameMessage} You should fix this error in your code.\n\n` + - `${errorBoundaryMessage}\n\n` + + `${componentNameMessage} You should fix this error in your code. ${errorBoundaryMessage}\n\n` + `${error.stack}\n\n` + `The error was thrown in the following location: ${componentStack}` ); From 88d2d5c39e1a3f44d539fb001b857d67749ef57b Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 12 Jan 2017 15:46:21 -0800 Subject: [PATCH 6/7] Improved test coverage of ReactFiberErrorLogger --- scripts/fiber/tests-passing.txt | 2 +- .../ReactIncrementalErrorHandling-test.js | 149 ++++++++++-------- 2 files changed, 85 insertions(+), 66 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 90562d88245c..357a65c4c47c 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1196,8 +1196,8 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js * handles error thrown by top-level callback * should log errors that occur during the begin phase * should log errors that occur during the commit phase -* should relay info about error boundary and retry attempts if applicable * should ignore errors thrown in log method to prevent cycle +* should relay info about error boundary and retry attempts if applicable src/renderers/shared/fiber/__tests__/ReactIncrementalReflection-test.js * handles isMounted even when the initial render is deferred diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js index cbfad2500ebb..23463cfd23bd 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js @@ -17,7 +17,6 @@ var ReactNoop; describe('ReactIncrementalErrorHandling', () => { beforeEach(() => { jest.resetModules(); - jest.mock('ReactFiberErrorLogger'); React = require('React'); ReactNoop = require('ReactNoop'); }); @@ -938,25 +937,25 @@ describe('ReactIncrementalErrorHandling', () => { }); describe('ReactFiberErrorLogger', () => { + function initReactFiberErrorLoggerMock(mock) { + jest.resetModules(); + if (mock) { + jest.mock('ReactFiberErrorLogger'); + } else { + jest.unmock('ReactFiberErrorLogger'); + } + React = require('React'); + ReactNoop = require('ReactNoop'); + } + function normalizeCodeLocInfo(str) { return str && str.replace(/\(at .+?:\d+\)/g, '(at **)'); } - let logCapturedErrorCalls; - let ReactFiberErrorLogger; - - beforeEach(() => { - logCapturedErrorCalls = []; - - ReactFiberErrorLogger = require('ReactFiberErrorLogger'); - ReactFiberErrorLogger.logCapturedError.mockImplementation( - (capturedError) => { - logCapturedErrorCalls.push(capturedError); - } - ); - }); - it('should log errors that occur during the begin phase', () => { + initReactFiberErrorLoggerMock(); + spyOn(console, 'error'); + class ErrorThrowingComponent extends React.Component { componentWillMount() { throw Error('componentWillMount error'); @@ -971,13 +970,16 @@ describe('ReactIncrementalErrorHandling', () => { ReactNoop.flushDeferredPri(); } catch (error) {} - expect(logCapturedErrorCalls.length).toBe(1); - expect(logCapturedErrorCalls[0].error.message).toBe('componentWillMount error'); - expect(logCapturedErrorCalls[0].componentName).toBe('ErrorThrowingComponent'); - expect(logCapturedErrorCalls[0].errorBoundaryName).toBe(null); - expect(logCapturedErrorCalls[0].errorBoundaryFound).toBe(false); - expect(logCapturedErrorCalls[0].willRetry).toBe(false); - expect(normalizeCodeLocInfo(logCapturedErrorCalls[0].componentStack)).toContain( + expect(console.error.calls.count()).toBe(1); + const errorMessage = console.error.calls.argsFor(0)[0]; + expect(errorMessage).toContain( + 'React caught an error thrown by ErrorThrowingComponent. ' + + 'You should fix this error in your code. ' + + 'Consider adding an error boundary to your tree to customize error handling behavior.' + ); + expect(errorMessage).toContain('Error: componentWillMount error'); + expect(normalizeCodeLocInfo(errorMessage)).toContain( + 'The error was thrown in the following location: \n' + ' in ErrorThrowingComponent (at **)\n' + ' in span (at **)\n' + ' in div (at **)' @@ -985,6 +987,9 @@ describe('ReactIncrementalErrorHandling', () => { }); it('should log errors that occur during the commit phase', () => { + initReactFiberErrorLoggerMock(); + spyOn(console, 'error'); + class ErrorThrowingComponent extends React.Component { componentDidMount() { throw Error('componentDidMount error'); @@ -999,20 +1004,58 @@ describe('ReactIncrementalErrorHandling', () => { ReactNoop.flushDeferredPri(); } catch (error) {} - expect(logCapturedErrorCalls.length).toBe(1); - expect(logCapturedErrorCalls[0].error.message).toBe('componentDidMount error'); - expect(logCapturedErrorCalls[0].componentName).toBe('ErrorThrowingComponent'); - expect(logCapturedErrorCalls[0].errorBoundaryName).toBe(null); - expect(logCapturedErrorCalls[0].errorBoundaryFound).toBe(false); - expect(logCapturedErrorCalls[0].willRetry).toBe(false); - expect(normalizeCodeLocInfo(logCapturedErrorCalls[0].componentStack)).toContain( + expect(console.error.calls.count()).toBe(1); + const errorMessage = console.error.calls.argsFor(0)[0]; + expect(errorMessage).toContain( + 'React caught an error thrown by ErrorThrowingComponent. ' + + 'You should fix this error in your code. ' + + 'Consider adding an error boundary to your tree to customize error handling behavior.' + ); + expect(errorMessage).toContain('Error: componentDidMount error'); + expect(normalizeCodeLocInfo(errorMessage)).toContain( + 'The error was thrown in the following location: \n' + ' in ErrorThrowingComponent (at **)\n' + ' in span (at **)\n' + ' in div (at **)' ); }); + it('should ignore errors thrown in log method to prevent cycle', () => { + initReactFiberErrorLoggerMock(true); + spyOn(console, 'error'); + + class ErrorThrowingComponent extends React.Component { + render() { + throw Error('render error'); + } + } + + const logCapturedErrorCalls = []; + + const ReactFiberErrorLogger = require('ReactFiberErrorLogger'); + ReactFiberErrorLogger.logCapturedError.mockImplementation( + (capturedError) => { + logCapturedErrorCalls.push(capturedError); + throw Error('logCapturedError error'); + } + ); + + try { + ReactNoop.render(
); + ReactNoop.flushDeferredPri(); + } catch (error) {} + + expect(logCapturedErrorCalls.length).toBe(1); + + // The error thrown in logCapturedError should also be logged + expect(console.error.calls.count()).toBe(1); + expect(console.error.calls.argsFor(0)[0].message).toContain('logCapturedError error'); + }); + it('should relay info about error boundary and retry attempts if applicable', () => { + initReactFiberErrorLoggerMock(); + spyOn(console, 'error'); + class ParentComponent extends React.Component { render() { return ; @@ -1049,43 +1092,19 @@ describe('ReactIncrementalErrorHandling', () => { expect(renderAttempts).toBe(2); expect(handleErrorCalls.length).toBe(1); - expect(logCapturedErrorCalls.length).toBe(2); - expect(logCapturedErrorCalls[0].error.message).toBe('componentDidMount error'); - expect(logCapturedErrorCalls[0].errorBoundaryName).toBe('ErrorBoundaryComponent'); - expect(logCapturedErrorCalls[0].errorBoundaryFound).toBe(true); - expect(logCapturedErrorCalls[0].willRetry).toBe(true); - expect(logCapturedErrorCalls[1].error.message).toBe('componentDidMount error'); - expect(logCapturedErrorCalls[1].errorBoundaryName).toBe('ErrorBoundaryComponent'); - expect(logCapturedErrorCalls[1].errorBoundaryFound).toBe(true); - expect(logCapturedErrorCalls[1].willRetry).toBe(false); - }); - - it('should ignore errors thrown in log method to prevent cycle', () => { - spyOn(console, 'error'); - - class ErrorThrowingComponent extends React.Component { - render() { - throw Error('render error'); - } - } - - ReactFiberErrorLogger.logCapturedError.mockImplementation( - (capturedError) => { - logCapturedErrorCalls.push(capturedError); - throw Error('logCapturedError error'); - } + expect(console.error.calls.count()).toBe(2); + expect(console.error.calls.argsFor(0)[0]).toContain( + 'React caught an error thrown by ErrorThrowingComponent. ' + + 'You should fix this error in your code. ' + + 'React will try to recreate this component tree from scratch ' + + 'using the error boundary you provided, ErrorBoundaryComponent.' + ); + expect(console.error.calls.argsFor(1)[0]).toContain( + 'React caught an error thrown by ErrorThrowingComponent. ' + + 'You should fix this error in your code. ' + + 'This error was initially handled by the error boundary ErrorBoundaryComponent. ' + + 'Recreating the tree from scratch failed so React will unmount the tree.' ); - - try { - ReactNoop.render(
); - ReactNoop.flushDeferredPri(); - } catch (error) {} - - expect(logCapturedErrorCalls.length).toBe(1); - - // The error thrown in logCapturedError should also be logged - expect(console.error.calls.count()).toBe(1); - expect(console.error.calls.argsFor(0)[0].message).toContain('logCapturedError error'); }); }); }); From 47a09ec8ce3f36dbabb6a8caf75e7e918a583a5b Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 13 Jan 2017 14:38:29 -0800 Subject: [PATCH 7/7] Minor Flow style tweak --- src/renderers/shared/fiber/ReactFiberErrorLogger.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberErrorLogger.js b/src/renderers/shared/fiber/ReactFiberErrorLogger.js index 53f9fd17200a..6b0d5b9ee778 100644 --- a/src/renderers/shared/fiber/ReactFiberErrorLogger.js +++ b/src/renderers/shared/fiber/ReactFiberErrorLogger.js @@ -30,15 +30,15 @@ function logCapturedError(capturedError : CapturedError) : void { : 'React caught an error thrown by one of your components.'; let errorBoundaryMessage; - if (errorBoundaryFound) { - // errorBoundaryFound check is sufficient; errorBoundaryName check is to satisfy Flow. + // errorBoundaryFound check is sufficient; errorBoundaryName check is to satisfy Flow. + if (errorBoundaryFound && errorBoundaryName) { if (willRetry) { errorBoundaryMessage = `React will try to recreate this component tree from scratch ` + - `using the error boundary you provided, ${errorBoundaryName || ''}.`; + `using the error boundary you provided, ${errorBoundaryName}.`; } else { errorBoundaryMessage = - `This error was initially handled by the error boundary ${errorBoundaryName || ''}. ` + + `This error was initially handled by the error boundary ${errorBoundaryName}. ` + `Recreating the tree from scratch failed so React will unmount the tree.`; } } else {