From be378d798fd07c5a3dba47b16ba8f08204c76118 Mon Sep 17 00:00:00 2001 From: Aaron Lademann Date: Tue, 15 Oct 2019 11:34:16 -0700 Subject: [PATCH 1/5] Add error logging to ErrorBoundary --- lib/over_react.dart | 2 +- lib/src/component/error_boundary.dart | 75 +++++++++ .../error_boundary.over_react.g.dart | 28 +++- .../component/error_boundary_test.dart | 147 ++++++++++++++++++ 4 files changed, 249 insertions(+), 3 deletions(-) diff --git a/lib/over_react.dart b/lib/over_react.dart index df7412c2a..69c1a7443 100644 --- a/lib/over_react.dart +++ b/lib/over_react.dart @@ -35,7 +35,7 @@ export 'src/component/abstract_transition.dart'; export 'src/component/abstract_transition_props.dart'; export 'src/component/aria_mixin.dart'; export 'src/component/callback_typedefs.dart'; -export 'src/component/error_boundary.dart'; +export 'src/component/error_boundary.dart' hide defaultErrorBoundaryLoggerName; export 'src/component/dom_components.dart'; export 'src/component/dummy_component.dart'; export 'src/component/prop_mixins.dart'; diff --git a/lib/src/component/error_boundary.dart b/lib/src/component/error_boundary.dart index dfffd8f83..9d9029f19 100644 --- a/lib/src/component/error_boundary.dart +++ b/lib/src/component/error_boundary.dart @@ -3,6 +3,7 @@ import 'dart:js_util' as js_util; import 'dart:js_util'; import 'package:js/js.dart'; +import 'package:logging/logging.dart'; import 'package:meta/meta.dart'; import 'package:over_react/over_react.dart'; import 'package:react/react_client.dart'; @@ -76,6 +77,9 @@ typedef _ComponentDidCatchCallback(/*Error||Exception*/dynamic error, /*Componen // TODO: Need to type the second argument once react-dart implements bindings for the ReactJS "errorInfo". typedef ReactElement _FallbackUiRenderer(/*Error||Exception*/dynamic error, /*ComponentStack*/dynamic errorInfo); +@visibleForTesting +const String defaultErrorBoundaryLoggerName = 'over_react.ErrorBoundary'; + /// A higher-order component that will catch ReactJS errors anywhere within the child component tree and /// display a fallback UI instead of the component tree that crashed. /// @@ -160,6 +164,11 @@ class _$ErrorBoundaryProps extends UiProps { /// /// > Default: `const Duration(seconds: 5)` Duration identicalErrorFrequencyTolerance; + + /// The name to use when the component's logger logs an error via [ErrorBoundaryComponent.componentDidCatch]. + /// + /// > Default: 'over_react.ErrorBoundary' + String loggerName; } @State() @@ -189,6 +198,7 @@ class ErrorBoundaryComponent (newProps() ..identicalErrorFrequencyTolerance = Duration(seconds: 5) + ..loggerName = defaultErrorBoundaryLoggerName ); @override @@ -197,6 +207,33 @@ class ErrorBoundaryComponent getProperty(jsErrorInfo, 'componentStack'); + + /// The logger that logs errors when a component somewhere within the React tree wrapped by + /// this [ErrorBoundary] instance throws within the React lifecycle. + Logger get logger => _logger; + Logger _logger; + + String _getLoggerName([T propsMap]) { + propsMap ??= props; + if (propsMap.loggerName != null && propsMap.loggerName.isNotEmpty) return propsMap.loggerName; + + return defaultErrorBoundaryLoggerName; + } + + // ----- [3] ----- // + void _logErrorCaughtByErrorBoundary( + /*Error|Exception*/ dynamic error, + /*ReactErrorInfo*/ dynamic info, { + bool isRecoverable = true, + }) { + String message = isRecoverable + ? 'An error was caught by an ErrorBoundary' + : 'An unrecoverable error was caught by an ErrorBoundary (the entire react tree had to be unmounted)'; + + dynamic stackTrace; + try { + stackTrace = error.stackTrace; + } catch (_) { + // The error / exception doesn't extend from Error or Exception + } + + _logger.severe(message, error, stackTrace); + } } diff --git a/lib/src/component/error_boundary.over_react.g.dart b/lib/src/component/error_boundary.over_react.g.dart index 0aca1ec1a..b5df6405c 100644 --- a/lib/src/component/error_boundary.over_react.g.dart +++ b/lib/src/component/error_boundary.over_react.g.dart @@ -194,6 +194,24 @@ abstract class _$ErrorBoundaryPropsAccessorsMixin set identicalErrorFrequencyTolerance(Duration value) => props[_$key__identicalErrorFrequencyTolerance___$ErrorBoundaryProps] = value; + + /// The name to use when the component's logger logs an error via [ErrorBoundaryComponent.componentDidCatch]. + /// + /// > Default: 'over_react.ErrorBoundary' + /// + /// + @override + String get loggerName => + props[_$key__loggerName___$ErrorBoundaryProps] ?? + null; // Add ` ?? null` to workaround DDC bug: ; + /// The name to use when the component's logger logs an error via [ErrorBoundaryComponent.componentDidCatch]. + /// + /// > Default: 'over_react.ErrorBoundary' + /// + /// + @override + set loggerName(String value) => + props[_$key__loggerName___$ErrorBoundaryProps] = value; /* GENERATED CONSTANTS */ static const PropDescriptor _$prop__onComponentDidCatch___$ErrorBoundaryProps = @@ -208,6 +226,8 @@ abstract class _$ErrorBoundaryPropsAccessorsMixin _$prop__identicalErrorFrequencyTolerance___$ErrorBoundaryProps = const PropDescriptor( _$key__identicalErrorFrequencyTolerance___$ErrorBoundaryProps); + static const PropDescriptor _$prop__loggerName___$ErrorBoundaryProps = + const PropDescriptor(_$key__loggerName___$ErrorBoundaryProps); static const String _$key__onComponentDidCatch___$ErrorBoundaryProps = 'ErrorBoundaryProps.onComponentDidCatch'; static const String _$key__onComponentIsUnrecoverable___$ErrorBoundaryProps = @@ -217,18 +237,22 @@ abstract class _$ErrorBoundaryPropsAccessorsMixin static const String _$key__identicalErrorFrequencyTolerance___$ErrorBoundaryProps = 'ErrorBoundaryProps.identicalErrorFrequencyTolerance'; + static const String _$key__loggerName___$ErrorBoundaryProps = + 'ErrorBoundaryProps.loggerName'; static const List $props = const [ _$prop__onComponentDidCatch___$ErrorBoundaryProps, _$prop__onComponentIsUnrecoverable___$ErrorBoundaryProps, _$prop__fallbackUIRenderer___$ErrorBoundaryProps, - _$prop__identicalErrorFrequencyTolerance___$ErrorBoundaryProps + _$prop__identicalErrorFrequencyTolerance___$ErrorBoundaryProps, + _$prop__loggerName___$ErrorBoundaryProps ]; static const List $propKeys = const [ _$key__onComponentDidCatch___$ErrorBoundaryProps, _$key__onComponentIsUnrecoverable___$ErrorBoundaryProps, _$key__fallbackUIRenderer___$ErrorBoundaryProps, - _$key__identicalErrorFrequencyTolerance___$ErrorBoundaryProps + _$key__identicalErrorFrequencyTolerance___$ErrorBoundaryProps, + _$key__loggerName___$ErrorBoundaryProps ]; } diff --git a/test/over_react/component/error_boundary_test.dart b/test/over_react/component/error_boundary_test.dart index 9c727ba80..98c44dc10 100644 --- a/test/over_react/component/error_boundary_test.dart +++ b/test/over_react/component/error_boundary_test.dart @@ -16,7 +16,9 @@ library error_boundary_test; import 'dart:html'; +import 'package:logging/logging.dart'; import 'package:over_react/over_react.dart'; +import 'package:over_react/src/component/error_boundary.dart' show defaultErrorBoundaryLoggerName; import 'package:over_react_test/over_react_test.dart'; import 'package:test/test.dart'; @@ -117,6 +119,7 @@ void main() { jacket = mount(ErrorBoundary()(dummyChild)); expect(ErrorBoundary(jacket.getProps()).identicalErrorFrequencyTolerance.inSeconds, 5); + expect(ErrorBoundary(jacket.getProps()).loggerName, 'over_react.ErrorBoundary'); }); test('initializes with the expected initial state values', () { @@ -716,6 +719,150 @@ void main() { }); }); + group('logs errors using its `logger`', () { + List> calls; + Logger logger; + List logRecords; + const identicalErrorFrequencyToleranceInMs = 500; + + // Cause an error to be thrown within a ReactJS lifecycle method + void triggerAComponentError() { + queryByTestId(jacket.getInstance(), 'flawedComponent_flawedButton').click(); + } + + void sharedSetup([String loggerName]) { + calls = []; + jacket = mount( + (ErrorBoundary() + ..loggerName = loggerName + ..identicalErrorFrequencyTolerance = const Duration(milliseconds: identicalErrorFrequencyToleranceInMs) + ..onComponentDidCatch = (err, info) { + calls.add({'onComponentDidCatch': [err, info]}); + } + ..onComponentIsUnrecoverable = (err, info) { + calls.add({'onComponentIsUnrecoverable': [err, info]}); + } + )(Flawed()()), + attachedToDocument: true, + ); + + logRecords = []; + logger = jacket.getDartInstance().logger; + expect(logger, isNotNull, reason: 'test setup sanity check'); + logger.onRecord.listen(logRecords.add); + } + + tearDown(() { + logger.clearListeners(); + logger = null; + logRecords = null; + }); + + group('when `props.loggerName` is not set on initial mount', () { + setUp(sharedSetup); + + test('and a component error is caught', () { + triggerAComponentError(); + + expect(logRecords, hasLength(1)); + expect(logRecords.single.level, Level.SEVERE); + expect(logRecords.single.loggerName, defaultErrorBoundaryLoggerName); + expect(logRecords.single.error, calls.single['onComponentDidCatch'][0]); + expect(logRecords.single.message, 'An error was caught by an ErrorBoundary'); + }); + + test('and an unrecoverable component error is caught', () async { + triggerAComponentError(); + await new Future.delayed(const Duration(milliseconds: identicalErrorFrequencyToleranceInMs ~/ 2)); + triggerAComponentError(); + + expect(logRecords, hasLength(2)); + expect(logRecords[1].level, Level.SEVERE); + expect(logRecords[1].loggerName, defaultErrorBoundaryLoggerName); + expect(logRecords[1].error, calls[2]['onComponentIsUnrecoverable'][0]); + expect(logRecords[1].message, + 'An unrecoverable error was caught by an ErrorBoundary (the entire react tree had to be unmounted)'); + }); + + group('but then `props.loggerName` is set', () { + group('to an empty string', () { + setUp(() { + jacket.rerender( + (ErrorBoundary() + ..loggerName = '' + ..identicalErrorFrequencyTolerance = const Duration(milliseconds: identicalErrorFrequencyToleranceInMs) + ..onComponentDidCatch = (err, info) { + calls.add({'onComponentDidCatch': [err, info]}); + } + ..onComponentIsUnrecoverable = (err, info) { + calls.add({'onComponentIsUnrecoverable': [err, info]}); + } + )(Flawed()()) + ); + }); + + test('and a component error is caught', () { + triggerAComponentError(); + + expect(logRecords.single.loggerName, defaultErrorBoundaryLoggerName, + reason: 'The loggerName should fall back to `defaultErrorBoundaryLoggerName` ' + 'if a consumer attempts to set it to an empty string'); + }); + + group('and then to null', () { + setUp(() { + expect(ErrorBoundary(jacket.getProps()).loggerName, isEmpty, reason: 'test setup sanity check'); + + jacket.rerender( + (ErrorBoundary() + ..loggerName = null + ..identicalErrorFrequencyTolerance = const Duration(milliseconds: identicalErrorFrequencyToleranceInMs) + ..onComponentDidCatch = (err, info) { + calls.add({'onComponentDidCatch': [err, info]}); + } + ..onComponentIsUnrecoverable = (err, info) { + calls.add({'onComponentIsUnrecoverable': [err, info]}); + } + )(Flawed()()) + ); + }); + + test('and a component error is caught', () { + triggerAComponentError(); + + expect(logRecords.single.loggerName, defaultErrorBoundaryLoggerName, + reason: 'The loggerName should fall back to `defaultErrorBoundaryLoggerName` ' + 'if a consumer attempts to set it to null'); + }); + }); + }); + + group('to a non-empty string', () { + setUp(() { + jacket.rerender( + (ErrorBoundary() + ..loggerName = 'myCustomErrorLoggerName' + ..identicalErrorFrequencyTolerance = const Duration(milliseconds: identicalErrorFrequencyToleranceInMs) + ..onComponentDidCatch = (err, info) { + calls.add({'onComponentDidCatch': [err, info]}); + } + ..onComponentIsUnrecoverable = (err, info) { + calls.add({'onComponentIsUnrecoverable': [err, info]}); + } + )(Flawed()()) + ); + }); + + test('and a component error is caught', () { + triggerAComponentError(); + + expect(logRecords.single.loggerName, 'myCustomErrorLoggerName'); + }); + }); + }); + }); + }); + group('throws a PropError when', () { test('more than one child is provided', () { expect(() => mount(ErrorBoundary()(dummyChild, dummyChild)), From a40849b3d1000a9b9db1af5120165aae1a9b8922 Mon Sep 17 00:00:00 2001 From: Aaron Lademann Date: Tue, 15 Oct 2019 14:50:10 -0700 Subject: [PATCH 2/5] Add error info to log message --- lib/src/component/error_boundary.dart | 6 +++--- test/over_react/component/error_boundary_test.dart | 6 ++++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/src/component/error_boundary.dart b/lib/src/component/error_boundary.dart index 9d9029f19..a121ec229 100644 --- a/lib/src/component/error_boundary.dart +++ b/lib/src/component/error_boundary.dart @@ -443,12 +443,12 @@ class ErrorBoundaryComponent Date: Tue, 15 Oct 2019 15:01:08 -0700 Subject: [PATCH 3/5] =?UTF-8?q?Don=E2=80=99t=20cache=20logger?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- lib/src/component/error_boundary.dart | 42 +-------- .../component/error_boundary_test.dart | 89 +++++++------------ 2 files changed, 34 insertions(+), 97 deletions(-) diff --git a/lib/src/component/error_boundary.dart b/lib/src/component/error_boundary.dart index a121ec229..656d4efb7 100644 --- a/lib/src/component/error_boundary.dart +++ b/lib/src/component/error_boundary.dart @@ -207,33 +207,6 @@ class ErrorBoundaryComponent getProperty(jsErrorInfo, 'componentStack'); - /// The logger that logs errors when a component somewhere within the React tree wrapped by - /// this [ErrorBoundary] instance throws within the React lifecycle. - Logger get logger => _logger; - Logger _logger; - - String _getLoggerName([T propsMap]) { - propsMap ??= props; - if (propsMap.loggerName != null && propsMap.loggerName.isNotEmpty) return propsMap.loggerName; - - return defaultErrorBoundaryLoggerName; - } + /// The value that will be used when creating a [Logger] to log errors from this component. + String get loggerName => props.loggerName ?? defaultErrorBoundaryLoggerName; // ----- [3] ----- // void _logErrorCaughtByErrorBoundary( @@ -457,6 +421,6 @@ class ErrorBoundaryComponent> calls; - Logger logger; List logRecords; const identicalErrorFrequencyToleranceInMs = 500; @@ -747,14 +746,11 @@ void main() { ); logRecords = []; - logger = jacket.getDartInstance().logger; - expect(logger, isNotNull, reason: 'test setup sanity check'); - logger.onRecord.listen(logRecords.add); + Logger(jacket.getDartInstance().loggerName).onRecord.listen(logRecords.add); } tearDown(() { - logger.clearListeners(); - logger = null; + Logger(jacket.getDartInstance().loggerName).clearListeners(); logRecords = null; }); @@ -786,64 +782,38 @@ void main() { ' \nInfo: ${calls[2]['onComponentIsUnrecoverable'][1]}'); }); - group('but then `props.loggerName` is set', () { - group('to an empty string', () { - setUp(() { - jacket.rerender( - (ErrorBoundary() - ..loggerName = '' - ..identicalErrorFrequencyTolerance = const Duration(milliseconds: identicalErrorFrequencyToleranceInMs) - ..onComponentDidCatch = (err, info) { - calls.add({'onComponentDidCatch': [err, info]}); - } - ..onComponentIsUnrecoverable = (err, info) { - calls.add({'onComponentIsUnrecoverable': [err, info]}); - } - )(Flawed()()) - ); - }); - - test('and a component error is caught', () { - triggerAComponentError(); - - expect(logRecords.single.loggerName, defaultErrorBoundaryLoggerName, - reason: 'The loggerName should fall back to `defaultErrorBoundaryLoggerName` ' - 'if a consumer attempts to set it to an empty string'); - }); - - group('and then to null', () { - setUp(() { - expect(ErrorBoundary(jacket.getProps()).loggerName, isEmpty, reason: 'test setup sanity check'); - - jacket.rerender( - (ErrorBoundary() - ..loggerName = null - ..identicalErrorFrequencyTolerance = const Duration(milliseconds: identicalErrorFrequencyToleranceInMs) - ..onComponentDidCatch = (err, info) { - calls.add({'onComponentDidCatch': [err, info]}); - } - ..onComponentIsUnrecoverable = (err, info) { - calls.add({'onComponentIsUnrecoverable': [err, info]}); - } - )(Flawed()()) - ); - }); + group('but then `props.loggerName` is set to a non-null value', () { + setUp(() { + Logger(jacket.getDartInstance().loggerName).clearListeners(); + + jacket.rerender( + (ErrorBoundary() + ..loggerName = 'myCustomErrorLoggerName' + ..identicalErrorFrequencyTolerance = const Duration(milliseconds: identicalErrorFrequencyToleranceInMs) + ..onComponentDidCatch = (err, info) { + calls.add({'onComponentDidCatch': [err, info]}); + } + ..onComponentIsUnrecoverable = (err, info) { + calls.add({'onComponentIsUnrecoverable': [err, info]}); + } + )(Flawed()()) + ); + Logger(jacket.getDartInstance().loggerName).onRecord.listen(logRecords.add); + }); - test('and a component error is caught', () { - triggerAComponentError(); + test('and a component error is caught', () { + triggerAComponentError(); - expect(logRecords.single.loggerName, defaultErrorBoundaryLoggerName, - reason: 'The loggerName should fall back to `defaultErrorBoundaryLoggerName` ' - 'if a consumer attempts to set it to null'); - }); - }); + expect(logRecords.single.loggerName, 'myCustomErrorLoggerName'); }); - group('to a non-empty string', () { + group('and then to a null value', () { setUp(() { + Logger(jacket.getDartInstance().loggerName).clearListeners(); + jacket.rerender( (ErrorBoundary() - ..loggerName = 'myCustomErrorLoggerName' + ..loggerName = null ..identicalErrorFrequencyTolerance = const Duration(milliseconds: identicalErrorFrequencyToleranceInMs) ..onComponentDidCatch = (err, info) { calls.add({'onComponentDidCatch': [err, info]}); @@ -853,12 +823,15 @@ void main() { } )(Flawed()()) ); + Logger(jacket.getDartInstance().loggerName).onRecord.listen(logRecords.add); }); test('and a component error is caught', () { triggerAComponentError(); - expect(logRecords.single.loggerName, 'myCustomErrorLoggerName'); + expect(logRecords.single.loggerName, defaultErrorBoundaryLoggerName, + reason: 'The loggerName should fall back to `defaultErrorBoundaryLoggerName` ' + 'if a consumer attempts to set it to null'); }); }); }); From 0681787316492cd71fca40a4f18e9187cc997f49 Mon Sep 17 00:00:00 2001 From: Aaron Lademann Date: Tue, 15 Oct 2019 15:22:01 -0700 Subject: [PATCH 4/5] Make ErrorBoundary logging more flexible for consumers --- lib/src/component/error_boundary.dart | 24 ++++- .../error_boundary.over_react.g.dart | 54 ++++++++++- .../component/error_boundary_test.dart | 95 ++++++++++++++++++- 3 files changed, 166 insertions(+), 7 deletions(-) diff --git a/lib/src/component/error_boundary.dart b/lib/src/component/error_boundary.dart index 656d4efb7..ae721313b 100644 --- a/lib/src/component/error_boundary.dart +++ b/lib/src/component/error_boundary.dart @@ -167,8 +167,19 @@ class _$ErrorBoundaryProps extends UiProps { /// The name to use when the component's logger logs an error via [ErrorBoundaryComponent.componentDidCatch]. /// + /// Not used if a custom [logger] is specified. + /// /// > Default: 'over_react.ErrorBoundary' String loggerName; + + /// Whether errors caught by this [ErrorBoundary] should be logged using a [Logger]. + /// + /// > Default: `true` + bool shouldLogErrors; + + /// An optional custom logger instance that will be used to log errors caught by + /// this [ErrorBoundary] when [shouldLogErrors] is true. + Logger logger; } @State() @@ -199,6 +210,7 @@ class ErrorBoundaryComponent (newProps() ..identicalErrorFrequencyTolerance = Duration(seconds: 5) ..loggerName = defaultErrorBoundaryLoggerName + ..shouldLogErrors = true ); @override @@ -307,7 +319,7 @@ class ErrorBoundaryComponent props.loggerName ?? defaultErrorBoundaryLoggerName; + String get loggerName { + if (props.logger != null) return props.logger.name; + + return props.loggerName ?? defaultErrorBoundaryLoggerName; + } // ----- [3] ----- // void _logErrorCaughtByErrorBoundary( @@ -410,6 +426,8 @@ class ErrorBoundaryComponent Default: 'over_react.ErrorBoundary' /// /// @@ -206,12 +208,48 @@ abstract class _$ErrorBoundaryPropsAccessorsMixin null; // Add ` ?? null` to workaround DDC bug: ; /// The name to use when the component's logger logs an error via [ErrorBoundaryComponent.componentDidCatch]. /// + /// Not used if a custom [logger] is specified. + /// /// > Default: 'over_react.ErrorBoundary' /// /// @override set loggerName(String value) => props[_$key__loggerName___$ErrorBoundaryProps] = value; + + /// Whether errors caught by this [ErrorBoundary] should be logged using a [Logger]. + /// + /// > Default: `true` + /// + /// + @override + bool get shouldLogErrors => + props[_$key__shouldLogErrors___$ErrorBoundaryProps] ?? + null; // Add ` ?? null` to workaround DDC bug: ; + /// Whether errors caught by this [ErrorBoundary] should be logged using a [Logger]. + /// + /// > Default: `true` + /// + /// + @override + set shouldLogErrors(bool value) => + props[_$key__shouldLogErrors___$ErrorBoundaryProps] = value; + + /// An optional custom logger instance that will be used to log errors caught by + /// this [ErrorBoundary] when [shouldLogErrors] is true. + /// + /// + @override + Logger get logger => + props[_$key__logger___$ErrorBoundaryProps] ?? + null; // Add ` ?? null` to workaround DDC bug: ; + /// An optional custom logger instance that will be used to log errors caught by + /// this [ErrorBoundary] when [shouldLogErrors] is true. + /// + /// + @override + set logger(Logger value) => + props[_$key__logger___$ErrorBoundaryProps] = value; /* GENERATED CONSTANTS */ static const PropDescriptor _$prop__onComponentDidCatch___$ErrorBoundaryProps = @@ -228,6 +266,10 @@ abstract class _$ErrorBoundaryPropsAccessorsMixin _$key__identicalErrorFrequencyTolerance___$ErrorBoundaryProps); static const PropDescriptor _$prop__loggerName___$ErrorBoundaryProps = const PropDescriptor(_$key__loggerName___$ErrorBoundaryProps); + static const PropDescriptor _$prop__shouldLogErrors___$ErrorBoundaryProps = + const PropDescriptor(_$key__shouldLogErrors___$ErrorBoundaryProps); + static const PropDescriptor _$prop__logger___$ErrorBoundaryProps = + const PropDescriptor(_$key__logger___$ErrorBoundaryProps); static const String _$key__onComponentDidCatch___$ErrorBoundaryProps = 'ErrorBoundaryProps.onComponentDidCatch'; static const String _$key__onComponentIsUnrecoverable___$ErrorBoundaryProps = @@ -239,20 +281,28 @@ abstract class _$ErrorBoundaryPropsAccessorsMixin 'ErrorBoundaryProps.identicalErrorFrequencyTolerance'; static const String _$key__loggerName___$ErrorBoundaryProps = 'ErrorBoundaryProps.loggerName'; + static const String _$key__shouldLogErrors___$ErrorBoundaryProps = + 'ErrorBoundaryProps.shouldLogErrors'; + static const String _$key__logger___$ErrorBoundaryProps = + 'ErrorBoundaryProps.logger'; static const List $props = const [ _$prop__onComponentDidCatch___$ErrorBoundaryProps, _$prop__onComponentIsUnrecoverable___$ErrorBoundaryProps, _$prop__fallbackUIRenderer___$ErrorBoundaryProps, _$prop__identicalErrorFrequencyTolerance___$ErrorBoundaryProps, - _$prop__loggerName___$ErrorBoundaryProps + _$prop__loggerName___$ErrorBoundaryProps, + _$prop__shouldLogErrors___$ErrorBoundaryProps, + _$prop__logger___$ErrorBoundaryProps ]; static const List $propKeys = const [ _$key__onComponentDidCatch___$ErrorBoundaryProps, _$key__onComponentIsUnrecoverable___$ErrorBoundaryProps, _$key__fallbackUIRenderer___$ErrorBoundaryProps, _$key__identicalErrorFrequencyTolerance___$ErrorBoundaryProps, - _$key__loggerName___$ErrorBoundaryProps + _$key__loggerName___$ErrorBoundaryProps, + _$key__shouldLogErrors___$ErrorBoundaryProps, + _$key__logger___$ErrorBoundaryProps ]; } diff --git a/test/over_react/component/error_boundary_test.dart b/test/over_react/component/error_boundary_test.dart index fa8917236..993b57670 100644 --- a/test/over_react/component/error_boundary_test.dart +++ b/test/over_react/component/error_boundary_test.dart @@ -120,6 +120,7 @@ void main() { expect(ErrorBoundary(jacket.getProps()).identicalErrorFrequencyTolerance.inSeconds, 5); expect(ErrorBoundary(jacket.getProps()).loggerName, 'over_react.ErrorBoundary'); + expect(ErrorBoundary(jacket.getProps()).shouldLogErrors, isTrue); }); test('initializes with the expected initial state values', () { @@ -719,7 +720,7 @@ void main() { }); }); - group('logs errors using its `logger`', () { + group('logs errors using a `logger`', () { List> calls; List logRecords; const identicalErrorFrequencyToleranceInMs = 500; @@ -729,11 +730,13 @@ void main() { queryByTestId(jacket.getInstance(), 'flawedComponent_flawedButton').click(); } - void sharedSetup([String loggerName]) { + void sharedSetup({String loggerName, bool shouldLogErrors = true, Logger customLogger}) { calls = []; jacket = mount( (ErrorBoundary() ..loggerName = loggerName + ..shouldLogErrors = shouldLogErrors + ..logger = customLogger ..identicalErrorFrequencyTolerance = const Duration(milliseconds: identicalErrorFrequencyToleranceInMs) ..onComponentDidCatch = (err, info) { calls.add({'onComponentDidCatch': [err, info]}); @@ -746,6 +749,9 @@ void main() { ); logRecords = []; + expect(jacket.getDartInstance().loggerName, customLogger?.name ?? loggerName ?? defaultErrorBoundaryLoggerName, + reason: 'The loggerName getter should return the name of the logger ' + 'that will log component errors if/when they happen.'); Logger(jacket.getDartInstance().loggerName).onRecord.listen(logRecords.add); } @@ -754,6 +760,91 @@ void main() { logRecords = null; }); + group('when `props.shouldLogErrors` is false', () { + test('on first mount', () { + sharedSetup(shouldLogErrors: false); + triggerAComponentError(); + expect(logRecords, isEmpty); + }); + + test('when receiving new props', () async { + sharedSetup(); + triggerAComponentError(); + expect(logRecords, hasLength(1)); + + jacket.rerender( + (ErrorBoundary() + ..shouldLogErrors = false + ..identicalErrorFrequencyTolerance = const Duration(milliseconds: identicalErrorFrequencyToleranceInMs) + ..onComponentDidCatch = (err, info) { + calls.add({'onComponentDidCatch': [err, info]}); + } + ..onComponentIsUnrecoverable = (err, info) { + calls.add({'onComponentIsUnrecoverable': [err, info]}); + } + )(Flawed()()) + ); + await new Future.delayed(const Duration(milliseconds: identicalErrorFrequencyToleranceInMs + 10)); + + triggerAComponentError(); + expect(logRecords, hasLength(1)); + }); + }); + + group('provided via `props.logger`', () { + test('on first mount', () { + sharedSetup(customLogger: Logger('myCustomLoggerLoggerName')); + triggerAComponentError(); + + expect(jacket.getDartInstance().loggerName, 'myCustomLoggerLoggerName', + reason: 'The loggerName getter should return the name of the logger passed in using props.logger'); + + expect(logRecords, hasLength(1)); + expect(logRecords.single.level, Level.SEVERE); + expect(logRecords.single.loggerName, 'myCustomLoggerLoggerName'); + expect(logRecords.single.error, calls.single['onComponentDidCatch'][0]); + expect(logRecords.single.message, 'An error was caught by an ErrorBoundary:' + ' \nInfo: ${calls.single['onComponentDidCatch'][1]}'); + }); + + test('when receiving new props', () { + sharedSetup(); + + jacket.rerender( + (ErrorBoundary() + ..logger = Logger('myCustomLoggerLoggerName') + ..identicalErrorFrequencyTolerance = const Duration(milliseconds: identicalErrorFrequencyToleranceInMs) + ..onComponentDidCatch = (err, info) { + calls.add({'onComponentDidCatch': [err, info]}); + } + ..onComponentIsUnrecoverable = (err, info) { + calls.add({'onComponentIsUnrecoverable': [err, info]}); + } + )(Flawed()()) + ); + + triggerAComponentError(); + + expect(jacket.getDartInstance().loggerName, 'myCustomLoggerLoggerName', + reason: 'The loggerName getter should return the name of the logger passed in using props.logger'); + + expect(logRecords, hasLength(1)); + expect(logRecords.single.level, Level.SEVERE); + expect(logRecords.single.loggerName, 'myCustomLoggerLoggerName'); + expect(logRecords.single.error, calls.single['onComponentDidCatch'][0]); + expect(logRecords.single.message, 'An error was caught by an ErrorBoundary:' + ' \nInfo: ${calls.single['onComponentDidCatch'][1]}'); + }); + + test('and `props.loggerName` is also set', () { + sharedSetup(loggerName: 'somethingElse', customLogger: Logger('myCustomLoggerLoggerName')); + triggerAComponentError(); + + expect(jacket.getDartInstance().loggerName, 'myCustomLoggerLoggerName', + reason: 'The loggerName getter should return the name of the logger passed in using props.logger'); + }); + }); + group('when `props.loggerName` is not set on initial mount', () { setUp(sharedSetup); From c9ca115a0b04fd5a3374f5968ee952095ea94295 Mon Sep 17 00:00:00 2001 From: Aaron Lademann Date: Tue, 15 Oct 2019 16:25:24 -0700 Subject: [PATCH 5/5] Address CR feedback --- lib/src/component/error_boundary.dart | 5 +- .../component/error_boundary_test.dart | 92 ++++--------------- 2 files changed, 18 insertions(+), 79 deletions(-) diff --git a/lib/src/component/error_boundary.dart b/lib/src/component/error_boundary.dart index ae721313b..7cf162cc2 100644 --- a/lib/src/component/error_boundary.dart +++ b/lib/src/component/error_boundary.dart @@ -413,8 +413,7 @@ class ErrorBoundaryComponent getProperty(jsErrorInfo, 'componentStack'); - /// The value that will be used when creating a [Logger] to log errors from this component. - String get loggerName { + String get _loggerName { if (props.logger != null) return props.logger.name; return props.loggerName ?? defaultErrorBoundaryLoggerName; @@ -439,6 +438,6 @@ class ErrorBoundaryComponent