From b9f817eba7dae430f2635dc2a9f217c01c11ff18 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 14 Aug 2018 09:58:53 -0700 Subject: [PATCH] chore(Test): Add more tests, some refactoring to ViewCompiler DI Partial work towards https://github.com/dart-lang/angular/issues/434. It looks like unfortunately completing this fix might be [virtually] infeasible without https://github.com/dart-lang/sdk/issues/34141, though we could potentially use a ternary expression (https://github.com/dart-lang/sdk/issues/34141#issuecomment-412929971). Closes #1568 PiperOrigin-RevId: 208663946 --- _tests/test/di/directive_test.dart | 109 +++++++++++++++++- angular/lib/src/compiler/identifiers.dart | 6 +- .../compiler/view_compiler/event_binder.dart | 8 +- .../view_compiler/ir/provider_source.dart | 10 ++ .../view_compiler/ir/providers_node.dart | 53 +++++++-- .../view_compiler/view_compiler_utils.dart | 5 +- angular/lib/src/di/errors.dart | 11 -- 7 files changed, 175 insertions(+), 27 deletions(-) diff --git a/_tests/test/di/directive_test.dart b/_tests/test/di/directive_test.dart index eb19177bf5..c18c95c571 100644 --- a/_tests/test/di/directive_test.dart +++ b/_tests/test/di/directive_test.dart @@ -181,11 +181,74 @@ void main() { (e) => '$e'.endsWith('No provider found for $MissingService'), ), ), - reason: 'AppView does not trace local injections', + reason: 'View compiler does not trace local injections (#434)', ); }); - test('should throw a readable erro message on a 2-node/parent failure', () { + test('should throw a readable error message on a child directive', () { + // NOTE: In an ideal scenario, this would throw a better error, i.e. + // WillFailInjecting1NodeParent -> 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 = NgTestBed(); + expect( + () => testBed.create(), + throwsA( + predicate( + (e) => '$e'.endsWith('No provider found for $MissingService'), + ), + ), + reason: 'View compiler does not trace local injections (#434)', + ); + }); + + test('should throw a readable error message in an embedded template', () { + // NOTE: In an ideal scenario, this would throw a better error, i.e. + // WillFailInjecting1NodeParent -> 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 = NgTestBed(); + expect( + () => testBed.create(), + throwsA( + predicate( + (e) => '$e'.endsWith('No provider found for $MissingService'), + ), + ), + reason: 'View compiler does not trace local injections (#434)', + ); + }); + + test('should throw a readable error message when quering a child', () { + // 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 = NgTestBed(); + expect( + () => testBed.create(), + throwsA( + predicate( + (e) => '$e'.endsWith('No provider found for $MissingService'), + ), + ), + reason: 'View compiler does not trace local injections (#434)', + ); + }); + + test('should throw a readable error 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 = NgTestBed().addProviders([ @@ -613,6 +676,48 @@ class WillFailInjecting2NodeParent { WillFailInjecting2NodeParent(InjectsMissingService _); } +@Component( + selector: 'will-fail-creating-child', + template: r''' + + ''', + directives: [WillFailInjecting1Node], +) +class WillFailCreatingChild {} + +@Component( + selector: 'will-fail-creating-child', + template: r''' + + ''', + directives: [ + NgIf, + WillFailInjecting1Node, + ], +) +class WillFailCreatingChildInTemplate { + var showChild = true; +} + +@Component( + selector: 'will-fail-querying-service', + template: r''' + + ''', + directives: [ + LazilyProvidesMissingService, + ], +) +class WillFailQueryingServiceInTemplate { + @ViewChild(LazilyProvidesMissingService, read: InjectsMissingService) + InjectsMissingService willFailDuringQuery; +} + +@Directive(selector: 'lazy-provides-missing-service', providers: [ + InjectsMissingService, +]) +class LazilyProvidesMissingService {} + const baseUrl = OpaqueToken('baseUrl'); @Component( diff --git a/angular/lib/src/compiler/identifiers.dart b/angular/lib/src/compiler/identifiers.dart index c7c6d622df..18c9221f21 100644 --- a/angular/lib/src/compiler/identifiers.dart +++ b/angular/lib/src/compiler/identifiers.dart @@ -2,7 +2,6 @@ import "compile_metadata.dart" show CompileIdentifierMetadata, CompileTokenMetadata; final appViewModuleUrl = "asset:angular/lib/src/core/linker/app_view.dart"; -final debugAppViewModuleUrl = "asset:angular/lib/src/debug/debug_app_view.dart"; final appViewUtilsModuleUrl = "asset:angular/lib/src/core/linker/app_view_utils.dart"; final cdModuleUrl = @@ -13,6 +12,7 @@ final ngForUrl = "asset:angular/lib/src/common/directives/ng_for.dart"; final profileRuntimeModuleUrl = "asset:angular/lib/src/debug/profile_runtime.dart"; final runtimeUtilsModuleUrl = "asset:angular/lib/src/runtime.dart"; +final debugInjectorModuleUrl = 'asset:angular/lib/src/di/errors.dart'; class Identifiers { static final appViewUtils = CompileIdentifierMetadata( @@ -106,6 +106,10 @@ class Identifiers { name: "isDevMode", moduleUrl: runtimeUtilsModuleUrl); static final unsafeCast = CompileIdentifierMetadata( name: "unsafeCast", moduleUrl: runtimeUtilsModuleUrl); + static final debugInjectorEnter = CompileIdentifierMetadata( + name: "debugInjectorEnter", moduleUrl: debugInjectorModuleUrl); + static final debugInjectorLeave = CompileIdentifierMetadata( + name: "debugInjectorLeave", moduleUrl: debugInjectorModuleUrl); static final interpolate = [ CompileIdentifierMetadata( diff --git a/angular/lib/src/compiler/view_compiler/event_binder.dart b/angular/lib/src/compiler/view_compiler/event_binder.dart index fd40632ecc..9dcc995256 100755 --- a/angular/lib/src/compiler/view_compiler/event_binder.dart +++ b/angular/lib/src/compiler/view_compiler/event_binder.dart @@ -61,8 +61,12 @@ class CompileEventListener { _eventParam = o.FnParam(EventHandlerVars.event.name, o.importType(null)); } - void _addAction(BoundEventAst hostEvent, CompileDirectiveMetadata directive, - ProviderSource directiveInstance, AnalyzedClass clazz) { + void _addAction( + BoundEventAst hostEvent, + CompileDirectiveMetadata directive, + ProviderSource directiveInstance, + AnalyzedClass clazz, + ) { if (_isTearoff(hostEvent)) { hostEvent = _rewriteTearoff(hostEvent, clazz); } diff --git a/angular/lib/src/compiler/view_compiler/ir/provider_source.dart b/angular/lib/src/compiler/view_compiler/ir/provider_source.dart index a0067d2f40..dc060a534c 100644 --- a/angular/lib/src/compiler/view_compiler/ir/provider_source.dart +++ b/angular/lib/src/compiler/view_compiler/ir/provider_source.dart @@ -10,7 +10,17 @@ abstract class ProviderSource { final CompileTokenMetadata token; final bool eager; final bool multiProvider; + ProviderSource(this.token, {this.eager, this.multiProvider}); o.Expression build(); + + /// Whether a dynamic `injectorGet(...)` is required to resolve this provider. + /// + /// For example: + /// ```dart + /// // DependencyService is dynamically required to resolve MyService. + /// _MyService = MyService(injectorGet(DependencyService)); + /// ``` + bool get hasDynamicDependencies; } diff --git a/angular/lib/src/compiler/view_compiler/ir/providers_node.dart b/angular/lib/src/compiler/view_compiler/ir/providers_node.dart index 1b777e3fa3..5e5dc7bd72 100644 --- a/angular/lib/src/compiler/view_compiler/ir/providers_node.dart +++ b/angular/lib/src/compiler/view_compiler/ir/providers_node.dart @@ -238,24 +238,42 @@ abstract class ProvidersNodeHost { } class BuiltInSource extends ProviderSource { - o.Expression _value; + final o.Expression _value; BuiltInSource(CompileTokenMetadata token, this._value) : super(token); + @override o.Expression build() => _value; + + @override + final hasDynamicDependencies = false; } class LiteralValueSource extends ProviderSource { - o.Expression _value; + final o.Expression _value; LiteralValueSource(CompileTokenMetadata token, this._value) : super(token); + @override o.Expression build() => _value; + + @override + final hasDynamicDependencies = false; +} + +bool _hasDynamicDependencies(Iterable sources) { + for (final source in sources) { + if (source is DynamicProviderSource || source.hasDynamicDependencies) { + return true; + } + } + return false; } class FactoryProviderSource extends ProviderSource { - CompileFactoryMetadata _factory; - List _parameters; + final CompileFactoryMetadata _factory; + final List _parameters; + FactoryProviderSource( CompileTokenMetadata token, this._factory, this._parameters) : super(token); @@ -266,12 +284,15 @@ class FactoryProviderSource extends ProviderSource { for (ProviderSource s in _parameters) paramExpressions.add(s.build()); return o.importExpr(_factory).callFn(paramExpressions); } + + @override + bool get hasDynamicDependencies => _hasDynamicDependencies(_parameters); } class ClassProviderSource extends ProviderSource { - CompileTypeMetadata _classType; - List _parameters; - List _typeArguments; + final CompileTypeMetadata _classType; + final List _parameters; + final List _typeArguments; ClassProviderSource( CompileTokenMetadata token, @@ -288,11 +309,15 @@ class ClassProviderSource extends ProviderSource { return o.importExpr(_classType).instantiate(paramExpressions, type: o.importType(_classType), genericTypes: _typeArguments); } + + @override + bool get hasDynamicDependencies => _hasDynamicDependencies(_parameters); } class FunctionalDirectiveSource extends ProviderSource { - CompileTypeMetadata _classType; - List _parameters; + final CompileTypeMetadata _classType; + final List _parameters; + FunctionalDirectiveSource( CompileTokenMetadata token, this._classType, this._parameters) : super(token); @@ -303,14 +328,18 @@ class FunctionalDirectiveSource extends ProviderSource { for (ProviderSource s in _parameters) paramExpressions.add(s.build()); return o.importExpr(_classType).callFn(paramExpressions); } + + @override + bool get hasDynamicDependencies => _hasDynamicDependencies(_parameters); } /// Source for injectable values that are resolved by /// dynamic lookup (injectorGet). class DynamicProviderSource extends ProviderSource { - ProvidersNode parentProviders; + final ProvidersNode parentProviders; final bool optional; final bool _isAppViewHost; + DynamicProviderSource(this.parentProviders, CompileTokenMetadata token, this.optional, this._isAppViewHost) : super(token); @@ -328,4 +357,8 @@ class DynamicProviderSource extends ProviderSource { } return viewExpr.callMethod('injectorGet', args); } + + // It *might*, but we don't know. + @override + final hasDynamicDependencies = false; } diff --git a/angular/lib/src/compiler/view_compiler/view_compiler_utils.dart b/angular/lib/src/compiler/view_compiler/view_compiler_utils.dart index 649733ccc6..0ba3cc7ef6 100644 --- a/angular/lib/src/compiler/view_compiler/view_compiler_utils.dart +++ b/angular/lib/src/compiler/view_compiler/view_compiler_utils.dart @@ -91,7 +91,10 @@ o.Expression getPropertyInView( } o.Expression injectFromViewParentInjector( - CompileView view, CompileTokenMetadata token, bool optional) { + CompileView view, + CompileTokenMetadata token, + bool optional, +) { o.Expression viewExpr = (view.viewType == ViewType.host) ? o.THIS_EXPR : o.ReadClassMemberExpr('parentView'); diff --git a/angular/lib/src/di/errors.dart b/angular/lib/src/di/errors.dart index 0fb17f8f45..87d028d45c 100644 --- a/angular/lib/src/di/errors.dart +++ b/angular/lib/src/di/errors.dart @@ -1,5 +1,4 @@ import 'package:angular/src/runtime.dart'; -import 'package:meta/meta.dart'; /// Current stack of tokens being requested for an injection. List _tokenStack; @@ -33,10 +32,6 @@ void debugInjectorLeave(Object token) { if (!isDevMode) { return; } - // Don't affect performance (as much) when this feature isn't enabled. - if (!InjectionError.enableBetterErrors) { - return; - } _tokenStack.removeLast(); } @@ -61,12 +56,6 @@ String _noProviderError(Object token) => 'No provider found for $token'; /// builds may be swapped out for less informative errors that cannot be caught; /// do not rely on being able to catch an [InjectionError] at runtime. abstract class InjectionError extends AssertionError { - /// May be set to `true` in order to get more debuggable error messages. - /// - /// **NOTE**: When assertions are disabled changing this does nothing. - @experimental - static bool enableBetterErrors = true; - InjectionError._(); }