diff --git a/_tests/test/di/directive_test.dart b/_tests/test/di/directive_test.dart index cc27d0acf7..d2834abfe9 100644 --- a/_tests/test/di/directive_test.dart +++ b/_tests/test/di/directive_test.dart @@ -8,6 +8,8 @@ import 'directive_test.template.dart' as ng_generated; /// Verifies whether injection through directives/components is correct. void main() { + // TODO(matanl): Remove once this is the default. + InjectionError.enableBetterErrors = true; ng_generated.initReflector(); tearDown(disposeAnyRunningTest); @@ -143,6 +145,63 @@ void main() { final fixture = await testBed.create(); expect(fixture.assertOnlyInstance.childView.example, ['A', 'B', 'C']); }); + + test('should throw a readable error message on a 1-node failure', () { + final testBed = new NgTestBed(); + expect( + () => testBed.create(), + throwsA( + predicate( + (e) => '$e'.endsWith('No provider found for $MissingService'), + ), + ), + ); + }); + + test('should throw a readable error message on a 2-node failure', () { + // NOTE: In an ideal scenario, this would throw a better error, i.e. + // InjectsMissingService -> MissingService + // + // ... but this would require enter() and leave() wrapping around the + // successful cases in AppView-local injection (and changes to the + // generated code). + // + // If we end up doing this, we should modify the test accordingly. + final testBed = new NgTestBed(); + expect( + () => testBed.create(), + throwsA( + predicate( + (e) => '$e'.endsWith('No provider found for $MissingService'), + ), + ), + reason: 'AppView does not trace local injections', + ); + }); + + test('should throw a readable erro message on a 2-node/parent failure', () { + // Passes, unlike the missing error case, because the parent injector, in + // this case a ReflectiveInjector, *does* trace the individual calls. + final testBed = new NgTestBed().addProviders([ + new Provider( + InjectsMissingService, + useFactory: (Object willNotBeCalled) => null, + deps: const [ + MissingService, + ], + ) + ]); + expect( + () => testBed.create(), + throwsA( + predicate( + (e) => '$e'.contains('' + 'No provider found for $MissingService: ' + '$InjectsMissingService -> $MissingService.'), + ), + ), + ); + }); } @Component( @@ -511,3 +570,38 @@ class ChildThatInjectsTypedToken { ChildThatInjectsTypedToken(@Inject(listOfStringToken) this.example); } + +class MissingService {} + +@Component( + selector: 'will-fail-injecting-1-node', + template: '', +) +class WillFailInjecting1Node { + WillFailInjecting1Node(MissingService _); +} + +class InjectsMissingService { + InjectsMissingService(MissingService _); +} + +@Component( + selector: 'will-fail-injecting-2-node', + providers: const [ + const Provider( + InjectsMissingService, + ), + ], + template: '', +) +class WillFailInjecting2Node { + WillFailInjecting2Node(InjectsMissingService _); +} + +@Component( + selector: 'will-fail-injecting-2-node', + template: '', +) +class WillFailInjecting2NodeParent { + WillFailInjecting2NodeParent(InjectsMissingService _); +} diff --git a/_tests/test/di/injector_test.dart b/_tests/test/di/injector_test.dart index f77abf1b42..16411678f8 100644 --- a/_tests/test/di/injector_test.dart +++ b/_tests/test/di/injector_test.dart @@ -5,7 +5,6 @@ import 'package:angular/src/di/injector/hierarchical.dart'; import 'package:angular/src/di/injector/injector.dart'; import 'package:test/test.dart'; import 'package:angular/src/di/reflector.dart' as reflector; -import 'package:angular_test/angular_test.dart'; import 'package:_tests/matchers.dart'; import 'injector_test.template.dart' as ng; @@ -68,6 +67,32 @@ void main() { ); }); + test('should throw a readable message with injection fails', () { + // Anything but injector.get(Injector) will fail here. + final injector = new Injector.empty(); + expect( + () => injector.get(ExampleService), + throwsA( + predicate( + (e) => '$e'.endsWith('No provider found for $ExampleService'), + ), + ), + ); + }); + + test('should throw a readable message even with a parent injector', () { + final parent = new Injector.empty(); + final child = new Injector.empty(parent); + expect( + () => child.get(ExampleService), + throwsA( + predicate( + (e) => '$e'.endsWith('No provider found for $ExampleService'), + ), + ), + ); + }); + test('should use orElse if provided', () { i = new Injector.empty(); expect(i.get(ExampleService, 123), 123); @@ -117,6 +142,18 @@ void main() { test('should return itself if Injector is passed', () { expect(i.get(Injector), i); }); + + test('should throw a readable error message on a failure', () { + final injector = new Injector.map({}); + expect( + () => injector.get(ExampleService), + throwsA( + predicate( + (e) => '$e'.endsWith('No provider found for $ExampleService'), + ), + ), + ); + }); }); group('.slowReflective', () { @@ -311,19 +348,67 @@ void main() { expect(injector.get(unnamedTokenOfString), 2); }); - test('should throw the provider token that failed', () { + test('should throw a readable error message on a 1-node failure', () { + final injector = new Injector.slowReflective([]); + expect( + () => injector.get(ExampleService), + throwsA( + predicate( + (e) => '$e'.endsWith('No provider found for $ExampleService'), + ), + ), + ); + }); + + test('should throw a readable error message on a 2-node failure', () { final injector = new Injector.slowReflective([ new Provider( ExampleService, - useFactory: (ExampleService2 e) => null, + useFactory: (Null willNeverBeCalled) => null, deps: const [ExampleService2], ), ]); expect( () => injector.get(ExampleService), - throwsInAngular( + throwsA( predicate( - (e) => '$e'.contains('No provider found for ExampleService2'), + (e) => '$e'.contains('' + 'No provider found for $ExampleService2: ' + '$ExampleService -> $ExampleService2.'), + ), + ), + ); + }); + + test('should throw a readable error message on a 3-node failure', () { + // ExampleService --> + // ExampleService2 --> + // ExampleService3 + ExampleService4 + // + // ... where ExampleService4 is missing. + final injector = new Injector.slowReflective([ + new Provider( + ExampleService, + useFactory: (Null willNeverBeCalled) => null, + deps: const [ExampleService2], + ), + new Provider( + ExampleService2, + useFactory: (Null willNeverBeCalled) => null, + deps: const [ExampleService3, ExampleService4], + ), + new Provider( + ExampleService3, + useValue: new ExampleService3(), + ), + ]); + expect( + () => injector.get(ExampleService), + throwsA( + predicate( + (e) => '$e'.contains('' + 'No provider found for $ExampleService4: ' + '$ExampleService -> $ExampleService2 -> $ExampleService4.'), ), ), ); @@ -385,6 +470,43 @@ void main() { expect(injector.get(unnamedTokenOfDynamic), 5); expect(injector.get(unnamedTokenOfString), 6); }); + + test('should throw a readable error message on a 1-node failure', () { + expect( + () => injector.get(MissingService), + throwsA( + predicate( + (e) => '$e'.endsWith('No provider found for $MissingService'), + ), + ), + ); + }); + + test('should throw a readable error message on a 2-node failure', () { + expect( + () => injector.get(ExampleService3), + throwsA( + predicate( + (e) => '$e'.contains('' + 'No provider found for $MissingService: ' + '$ExampleService3 -> $MissingService.'), + ), + ), + ); + }); + + test('should throw a readable error message on a 3-node failure', () { + expect( + () => injector.get(ExampleService4), + throwsA( + predicate( + (e) => '$e'.contains('' + 'No provider found for $MissingService: ' + '$ExampleService4 -> $ExampleService3 -> $MissingService.'), + ), + ), + ); + }); }); }); } @@ -411,6 +533,12 @@ class ExampleService { class ExampleService2 implements ExampleService {} +class ExampleService3 {} + +class ExampleService4 {} + +class MissingService {} + const stringToken = const OpaqueToken('stringToken'); const numberToken = const OpaqueToken('numberToken'); const booleanToken = const OpaqueToken('booleanToken'); @@ -427,6 +555,9 @@ const typedTokenOfListString = const OpaqueToken>('typedToken'); const unnamedTokenOfDynamic = const OpaqueToken(); const unnamedTokenOfString = const OpaqueToken(); +Null willNeverBeCalled1(Object _) => null; +Null willNeverBeCalled2(Object _, Object __) => null; + @Injector.generate(const [ const Provider(ExampleService, useClass: ExampleService2), const Provider(ExampleService2), @@ -435,6 +566,21 @@ const unnamedTokenOfString = const OpaqueToken(); const Provider(booleanToken, useValue: true), const Provider(simpleConstToken, useValue: const ExampleService()), + // Example of a runtime failure; MissingService + const Provider( + ExampleService3, + useFactory: willNeverBeCalled1, + deps: const [MissingService], + ), + + // Example of a runtime failure; ExampleService3 -> MissingService. + const Provider( + ExampleService4, + useFactory: willNeverBeCalled2, + // Will find ExampleService2, ExampleService3 will fail (see above). + deps: const [ExampleService2, ExampleService3], + ), + // TODO(matanl): Switch to ValueProvider.forToken once supported. const Provider(multiStringToken, useValue: 'A'), const Provider(multiStringToken, useValue: 'B'), diff --git a/angular/CHANGELOG.md b/angular/CHANGELOG.md index 86190d5691..ba3c1bda8b 100644 --- a/angular/CHANGELOG.md +++ b/angular/CHANGELOG.md @@ -2,8 +2,8 @@ * Added `InjectionError` and `NoProviderError`, which _may_ be thrown during dependency injection when `InjectionError.enableBetterErrors` is set to - `true`. This is an experiment, and we not complete this feature (and it could - be rolled back entirely). + `true`. This is an experiment, and we may not complete this feature (and it + could be rolled back entirely). ### Bug fixes diff --git a/angular/lib/src/core/linker/app_view.dart b/angular/lib/src/core/linker/app_view.dart index 4749c159cb..37d375cfc8 100644 --- a/angular/lib/src/core/linker/app_view.dart +++ b/angular/lib/src/core/linker/app_view.dart @@ -6,6 +6,7 @@ import 'package:meta/meta.dart'; import 'package:angular/src/core/app_view_consts.dart'; import 'package:angular/src/core/change_detection/change_detection.dart' show ChangeDetectorRef, ChangeDetectionStrategy, ChangeDetectorState; +import 'package:angular/src/di/errors.dart' as di_errors; import 'package:angular/src/di/injector/element.dart'; import 'package:angular/src/di/injector/injector.dart' show throwIfNotFound, Injector; @@ -296,6 +297,7 @@ abstract class AppView { } dynamic injectorGet(token, int nodeIndex, [notFoundValue = throwIfNotFound]) { + di_errors.debugInjectorEnter(token); var result = _UndefinedInjectorResult; AppView view = this; while (identical(result, _UndefinedInjectorResult)) { @@ -312,6 +314,7 @@ abstract class AppView { nodeIndex = view.viewData.parentIndex; view = view.parentView; } + di_errors.debugInjectorLeave(token); return result; } diff --git a/angular/lib/src/di/errors.dart b/angular/lib/src/di/errors.dart index 1aad120ad7..10efcc3f1f 100644 --- a/angular/lib/src/di/errors.dart +++ b/angular/lib/src/di/errors.dart @@ -1,17 +1,63 @@ import 'package:angular/src/facade/lang.dart'; import 'package:meta/meta.dart'; +/// Current stack of tokens being requested for an injection. +List _tokenStack; + +/// In debug mode, trace entering an injection lookup of [token] in [injector]. +/// +/// For example: +/// ``` +/// dynamic get(Object token) { +/// debugInjectorEnter(token); +/// var result = _getOrThrow(token); +/// debugInjectorLeave(token); +/// return result; +/// } +/// ``` +void debugInjectorEnter(Object token) { + // Tree-shake out in Dart2JS. + if (!assertionsEnabled()) { + return; + } + // Don't affect performance (as much) when this feature isn't enabled. + if (!InjectionError.enableBetterErrors) { + return; + } + if (_tokenStack == null) { + _tokenStack = [token]; + } else { + _tokenStack.add(token); + } +} + +/// In debug mode, trace leaving an injection lookup (successfully). +void debugInjectorLeave(Object token) { + // Tree-shake out in Dart2JS. + if (!assertionsEnabled()) { + return; + } + // Don't affect performance (as much) when this feature isn't enabled. + if (!InjectionError.enableBetterErrors) { + return; + } + _tokenStack.removeLast(); +} + /// Returns an error describing that [token] was not found as a provider. Error noProviderError(Object token) { // Only in developer mode, and only when a flag is set. // There are already users relying on an ArgumentError _always_ being thrown. if (assertionsEnabled() && InjectionError.enableBetterErrors) { - return new NoProviderError._(token); + final error = new NoProviderError._(token, _tokenStack); + // IMPORTANT: Clears the stack after reporting the error. + _tokenStack = null; + return error; } return new ArgumentError(_noProviderError(token)); } -String _noProviderError(Object token) => 'No provider found for $token.'; +String _noProviderError(Object token) => 'No provider found for $token'; /// A class of error that is thrown related to dependency injection. /// @@ -23,17 +69,48 @@ abstract class InjectionError extends AssertionError { /// /// **NOTE**: When assertions are disabled changing this does nothing. @experimental - static bool enableBetterErrors = false; + static bool enableBetterErrors = true; InjectionError._(); } /// Thrown when there is no dependency injection provider found for a [token]. class NoProviderError extends InjectionError { + // Transforms: [A, B, B, C, B] ==> [A, B, C, B]. + static List _withAdjacentDeduped(List input, Object token) { + if (input == null) { + return const []; + } + final output = []; + var lastElement = new Object(); + for (final element in input) { + if (!identical(lastElement, element)) { + output.add(lastElement = element); + } + } + // Remove the last T (== token), to avoid printing T: T -> T. + if (output.isNotEmpty) { + output.removeLast(); + } + return output; + } + + /// Token that failed to be found during lookup. final Object token; - NoProviderError._(this.token) : super._(); + /// Path of tokens traversed until it resulted in [token] failing. + final List path; + + NoProviderError._(this.token, List stack) + : path = _withAdjacentDeduped(stack, token), + super._(); @override - String toString() => _noProviderError(token); + String toString() => path.isEmpty + ? _noProviderError(token) + : _noProviderError(token) + + ': ${path.join(' -> ')} -> $token.\n' + '**NOTE**: This path is not exhaustive, and nodes may be missing ' + 'in between the "->" delimiters. There is ongoing work to improve ' + 'this error message and include all the nodes where possible. '; } diff --git a/angular/lib/src/di/injector/hierarchical.dart b/angular/lib/src/di/injector/hierarchical.dart index f643505fed..8889a92a22 100644 --- a/angular/lib/src/di/injector/hierarchical.dart +++ b/angular/lib/src/di/injector/hierarchical.dart @@ -1,5 +1,6 @@ import 'package:meta/meta.dart'; +import '../errors.dart' as errors; import 'empty.dart'; import 'injector.dart'; @@ -20,10 +21,12 @@ abstract class HierarchicalInjector extends Injector { @override T inject(Object token) { + errors.debugInjectorEnter(token); final result = injectOptional(token); if (identical(result, throwIfNotFound)) { return throwsNotFound(this, token); } + errors.debugInjectorLeave(token); return result; } @@ -32,10 +35,12 @@ abstract class HierarchicalInjector extends Injector { Object token, [ Object orElse = throwIfNotFound, ]) { + errors.debugInjectorEnter(token); var result = injectFromSelfOptional(token, orElse); if (identical(result, orElse)) { result = injectFromAncestryOptional(token, orElse); } + errors.debugInjectorLeave(token); return result; } diff --git a/angular/lib/src/di/injector/injector.dart b/angular/lib/src/di/injector/injector.dart index c5af93f341..8192d65c9d 100644 --- a/angular/lib/src/di/injector/injector.dart +++ b/angular/lib/src/di/injector/injector.dart @@ -92,11 +92,14 @@ abstract class Injector { /// - Throws an error (default behavior). /// /// An injector always returns itself if [Injector] is given as a token. + @mustCallSuper dynamic get(Object token, [Object notFoundValue = throwIfNotFound]) { + errors.debugInjectorEnter(token); final result = injectOptional(token, notFoundValue); if (identical(result, throwIfNotFound)) { return throwsNotFound(this, token); } + errors.debugInjectorLeave(token); return result; } @@ -106,12 +109,6 @@ abstract class Injector { /// final rpcService = injector.inject(); /// ``` /// - /// _or_: - /// - /// ```dart - /// - /// ``` - /// /// **EXPERIMENTAL**: Reified types are currently not supported in all of the /// various Dart runtime implementations (only DDC, not Dart2JS or the VM), so /// [fallbackToken] is currently required to be used. diff --git a/angular/lib/src/di/injector/runtime.dart b/angular/lib/src/di/injector/runtime.dart index e0bf65cfcd..587afa6932 100644 --- a/angular/lib/src/di/injector/runtime.dart +++ b/angular/lib/src/di/injector/runtime.dart @@ -1,6 +1,7 @@ import '../../core/di/decorators.dart'; import '../../core/di/opaque_token.dart'; import '../../facade/lang.dart' show assertionsEnabled; +import '../errors.dart' as errors; import '../providers.dart'; import '../reflector.dart' as reflector; @@ -109,7 +110,14 @@ class _RuntimeInjector extends HierarchicalInjector final resolved = new List(deps.length); for (var i = 0, l = resolved.length; i < l; i++) { final dep = deps[i]; - final result = dep is List ? _resolveMeta(dep) : inject(dep); + Object result; + if (dep is List) { + result = _resolveMeta(dep); + } else { + errors.debugInjectorEnter(dep); + result = inject(dep); + errors.debugInjectorLeave(dep); + } // We don't check to see if this failed otherwise, because this is an // edge case where we just delegate to Function.apply to invoke a factory. if (identical(result, throwIfNotFound)) { @@ -154,6 +162,7 @@ class _RuntimeInjector extends HierarchicalInjector } // TODO(matanl): Assert that there is no invalid combination. Object result; + errors.debugInjectorEnter(token); final orElse = isOptional ? null : throwIfNotFound; if (isSkipSelf) { result = injectFromAncestryOptional(token, orElse); @@ -167,6 +176,7 @@ class _RuntimeInjector extends HierarchicalInjector if (identical(result, throwIfNotFound)) { throwsNotFound(this, token); } + errors.debugInjectorLeave(token); return result; }