From c161ed415d3fee5fce278a4b1d3c11b4a15717ce Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Tue, 2 Aug 2016 01:37:42 -0700 Subject: [PATCH] feat(core): support `ngOnDestroy` on providers of a directive. Such providers also become eager as they will be instantiated anyways on destruction. --- ...le_reflector.ts => lifecycle_reflector.ts} | 0 .../compiler/src/metadata_resolver.ts | 2 +- .../compiler/src/provider_analyzer.ts | 9 +++-- .../src/view_compiler/compile_element.ts | 35 +++++++++---------- .../src/view_compiler/event_binder.ts | 6 ++-- .../src/view_compiler/lifecycle_binder.ts | 9 +++-- .../compiler/src/view_compiler/view_binder.ts | 30 ++++++++++------ .../compiler/test/directive_lifecycle_spec.ts | 2 +- .../change_detection_integration_spec.ts | 22 ++++++++++++ .../linker/view_injector_integration_spec.ts | 14 ++++++++ 10 files changed, 87 insertions(+), 42 deletions(-) rename modules/@angular/compiler/src/{directive_lifecycle_reflector.ts => lifecycle_reflector.ts} (100%) diff --git a/modules/@angular/compiler/src/directive_lifecycle_reflector.ts b/modules/@angular/compiler/src/lifecycle_reflector.ts similarity index 100% rename from modules/@angular/compiler/src/directive_lifecycle_reflector.ts rename to modules/@angular/compiler/src/lifecycle_reflector.ts diff --git a/modules/@angular/compiler/src/metadata_resolver.ts b/modules/@angular/compiler/src/metadata_resolver.ts index dadd8c911c..60d54abaf2 100644 --- a/modules/@angular/compiler/src/metadata_resolver.ts +++ b/modules/@angular/compiler/src/metadata_resolver.ts @@ -14,11 +14,11 @@ import {StringMapWrapper} from '../src/facade/collection'; import {assertArrayOfStrings, assertInterpolationSymbols} from './assertions'; import * as cpl from './compile_metadata'; import {CompilerConfig} from './config'; -import {hasLifecycleHook} from './directive_lifecycle_reflector'; import {DirectiveResolver} from './directive_resolver'; import {BaseException} from './facade/exceptions'; import {Type, isArray, isBlank, isPresent, isString, stringify} from './facade/lang'; import {Identifiers, identifierToken} from './identifiers'; +import {hasLifecycleHook} from './lifecycle_reflector'; import {NgModuleResolver} from './ng_module_resolver'; import {PipeResolver} from './pipe_resolver'; import {ElementSchemaRegistry} from './schema/element_schema_registry'; diff --git a/modules/@angular/compiler/src/provider_analyzer.ts b/modules/@angular/compiler/src/provider_analyzer.ts index 2d086fbfe5..c657bba86d 100644 --- a/modules/@angular/compiler/src/provider_analyzer.ts +++ b/modules/@angular/compiler/src/provider_analyzer.ts @@ -478,10 +478,13 @@ function _resolveProviders( sourceSpan)); } if (isBlank(resolvedProvider)) { - const lifecycleHooks = provider.useClass ? provider.useClass.lifecycleHooks : []; + const lifecycleHooks = + provider.token.identifier && provider.token.identifier instanceof CompileTypeMetadata ? + provider.token.identifier.lifecycleHooks : + []; resolvedProvider = new ProviderAst( - provider.token, provider.multi, eager, [provider], providerType, lifecycleHooks, - sourceSpan); + provider.token, provider.multi, eager || lifecycleHooks.length > 0, [provider], + providerType, lifecycleHooks, sourceSpan); targetProvidersByToken.add(provider.token, resolvedProvider); } else { if (!provider.multi) { diff --git a/modules/@angular/compiler/src/view_compiler/compile_element.ts b/modules/@angular/compiler/src/view_compiler/compile_element.ts index 9d5de50a32..b0596ad630 100644 --- a/modules/@angular/compiler/src/view_compiler/compile_element.ts +++ b/modules/@angular/compiler/src/view_compiler/compile_element.ts @@ -43,7 +43,7 @@ export class CompileElement extends CompileNode { public appElement: o.ReadPropExpr; public elementRef: o.Expression; public injector: o.Expression; - private _instances = new CompileIdentifierMap(); + public instances = new CompileIdentifierMap(); private _resolvedProviders: CompileIdentifierMap; private _queryCount = 0; @@ -52,7 +52,6 @@ export class CompileElement extends CompileNode { public contentNodesByNgContentIndex: Array[] = null; public embeddedView: CompileView; - public directiveInstances: o.Expression[]; public referenceTokens: {[key: string]: CompileTokenMetadata}; constructor( @@ -66,10 +65,10 @@ export class CompileElement extends CompileNode { references.forEach(ref => this.referenceTokens[ref.name] = ref.value); this.elementRef = o.importExpr(Identifiers.ElementRef).instantiate([this.renderNode]); - this._instances.add(identifierToken(Identifiers.ElementRef), this.elementRef); + this.instances.add(identifierToken(Identifiers.ElementRef), this.elementRef); this.injector = o.THIS_EXPR.callMethod('injector', [o.literal(this.nodeIndex)]); - this._instances.add(identifierToken(Identifiers.Injector), this.injector); - this._instances.add(identifierToken(Identifiers.Renderer), o.THIS_EXPR.prop('renderer')); + this.instances.add(identifierToken(Identifiers.Injector), this.injector); + this.instances.add(identifierToken(Identifiers.Renderer), o.THIS_EXPR.prop('renderer')); if (this.hasViewContainer || this.hasEmbeddedView || isPresent(this.component)) { this._createAppElement(); } @@ -89,7 +88,7 @@ export class CompileElement extends CompileNode { .toStmt(); this.view.createMethod.addStmt(statement); this.appElement = o.THIS_EXPR.prop(fieldName); - this._instances.add(identifierToken(Identifiers.AppElement), this.appElement); + this.instances.add(identifierToken(Identifiers.AppElement), this.appElement); } public createComponentFactoryResolver(entryComponents: CompileIdentifierMetadata[]) { @@ -139,7 +138,7 @@ export class CompileElement extends CompileNode { beforeChildren(): void { if (this.hasViewContainer) { - this._instances.add( + this.instances.add( identifierToken(Identifiers.ViewContainerRef), this.appElement.prop('vcRef')); } @@ -168,18 +167,16 @@ export class CompileElement extends CompileNode { return convertValueToOutputAst(provider.useValue); } }); - var propName = `_${resolvedProvider.token.name}_${this.nodeIndex}_${this._instances.size}`; + var propName = `_${resolvedProvider.token.name}_${this.nodeIndex}_${this.instances.size}`; var instance = createProviderProperty( propName, resolvedProvider, providerValueExpressions, resolvedProvider.multiProvider, resolvedProvider.eager, this); - this._instances.add(resolvedProvider.token, instance); + this.instances.add(resolvedProvider.token, instance); }); - this.directiveInstances = - this._directives.map((directive) => this._instances.get(identifierToken(directive.type))); - for (var i = 0; i < this.directiveInstances.length; i++) { - var directiveInstance = this.directiveInstances[i]; + for (var i = 0; i < this._directives.length; i++) { var directive = this._directives[i]; + var directiveInstance = this.instances.get(identifierToken(directive.type)); directive.queries.forEach((queryMeta) => { this._addQuery(queryMeta, directiveInstance); }); } var queriesWithReads: _QueryWithRead[] = []; @@ -193,7 +190,7 @@ export class CompileElement extends CompileNode { var token = this.referenceTokens[varName]; var varValue: o.Expression; if (isPresent(token)) { - varValue = this._instances.get(token); + varValue = this.instances.get(token); } else { varValue = this.renderNode; } @@ -207,12 +204,12 @@ export class CompileElement extends CompileNode { var value: o.Expression; if (isPresent(queryWithRead.read.identifier)) { // query for an identifier - value = this._instances.get(queryWithRead.read); + value = this.instances.get(queryWithRead.read); } else { // query for a reference var token = this.referenceTokens[queryWithRead.read.value]; if (isPresent(token)) { - value = this._instances.get(token); + value = this.instances.get(token); } else { value = this.elementRef; } @@ -241,7 +238,7 @@ export class CompileElement extends CompileNode { // Note: afterChildren is called after recursing into children. // This is good so that an injector match in an element that is closer to a requesting element // matches first. - var providerExpr = this._instances.get(resolvedProvider.token); + var providerExpr = this.instances.get(resolvedProvider.token); // Note: view providers are only visible on the injector of that element. // This is not fully correct as the rules during codegen don't allow a directive // to get hold of a view provdier on the same element. We still do this semantic @@ -263,7 +260,7 @@ export class CompileElement extends CompileNode { } getComponent(): o.Expression { - return isPresent(this.component) ? this._instances.get(identifierToken(this.component.type)) : + return isPresent(this.component) ? this.instances.get(identifierToken(this.component.type)) : null; } @@ -342,7 +339,7 @@ export class CompileElement extends CompileNode { resolvedProvider.providerType === ProviderAstType.PrivateService) { return null; } - result = this._instances.get(dep.token); + result = this.instances.get(dep.token); } } return result; diff --git a/modules/@angular/compiler/src/view_compiler/event_binder.ts b/modules/@angular/compiler/src/view_compiler/event_binder.ts index cb6d7a0f59..f48d537f5d 100644 --- a/modules/@angular/compiler/src/view_compiler/event_binder.ts +++ b/modules/@angular/compiler/src/view_compiler/event_binder.ts @@ -9,6 +9,7 @@ import {CompileDirectiveMetadata} from '../compile_metadata'; import {ListWrapper, StringMapWrapper} from '../facade/collection'; import {StringWrapper, isBlank, isPresent} from '../facade/lang'; +import {identifierToken} from '../identifiers'; import * as o from '../output/output_ast'; import {BoundEventAst, DirectiveAst} from '../template_parser/template_ast'; @@ -134,8 +135,9 @@ export function collectEventListeners( compileElement, hostEvent.target, hostEvent.name, eventListeners); listener.addAction(hostEvent, null, null); }); - ListWrapper.forEachWithIndex(dirs, (directiveAst, i) => { - var directiveInstance = compileElement.directiveInstances[i]; + dirs.forEach((directiveAst) => { + var directiveInstance = + compileElement.instances.get(identifierToken(directiveAst.directive.type)); directiveAst.hostEvents.forEach((hostEvent) => { compileElement.view.bindings.push(new CompileBinding(compileElement, hostEvent)); var listener = CompileEventListener.getOrCreate( diff --git a/modules/@angular/compiler/src/view_compiler/lifecycle_binder.ts b/modules/@angular/compiler/src/view_compiler/lifecycle_binder.ts index 0bd426a0cd..af1c2b5104 100644 --- a/modules/@angular/compiler/src/view_compiler/lifecycle_binder.ts +++ b/modules/@angular/compiler/src/view_compiler/lifecycle_binder.ts @@ -9,7 +9,7 @@ import {LifecycleHooks} from '../../core_private'; import {CompileDirectiveMetadata, CompilePipeMetadata} from '../compile_metadata'; import * as o from '../output/output_ast'; -import {DirectiveAst} from '../template_parser/template_ast'; +import {DirectiveAst, ProviderAst} from '../template_parser/template_ast'; import {CompileElement} from './compile_element'; import {CompileView} from './compile_view'; @@ -77,12 +77,11 @@ export function bindDirectiveAfterViewLifecycleCallbacks( } } -export function bindDirectiveDestroyLifecycleCallbacks( - directiveMeta: CompileDirectiveMetadata, directiveInstance: o.Expression, - compileElement: CompileElement) { +export function bindInjectableDestroyLifecycleCallbacks( + provider: ProviderAst, directiveInstance: o.Expression, compileElement: CompileElement) { var onDestroyMethod = compileElement.view.destroyMethod; onDestroyMethod.resetDebugInfo(compileElement.nodeIndex, compileElement.sourceAst); - if (directiveMeta.type.lifecycleHooks.indexOf(LifecycleHooks.OnDestroy) !== -1) { + if (provider.lifecycleHooks.indexOf(LifecycleHooks.OnDestroy) !== -1) { onDestroyMethod.addStmt(directiveInstance.callMethod('ngOnDestroy', []).toStmt()); } } diff --git a/modules/@angular/compiler/src/view_compiler/view_binder.ts b/modules/@angular/compiler/src/view_compiler/view_binder.ts index f83b3c3da6..b10fdf03ce 100644 --- a/modules/@angular/compiler/src/view_compiler/view_binder.ts +++ b/modules/@angular/compiler/src/view_compiler/view_binder.ts @@ -10,9 +10,10 @@ import {ListWrapper,} from '../facade/collection'; import {TemplateAst, TemplateAstVisitor, NgContentAst, EmbeddedTemplateAst, ElementAst, ReferenceAst, VariableAst, BoundEventAst, BoundElementPropertyAst, AttrAst, BoundTextAst, TextAst, DirectiveAst, BoundDirectivePropertyAst, templateVisitAll,} from '../template_parser/template_ast'; import {bindRenderText, bindRenderInputs, bindDirectiveInputs, bindDirectiveHostProps} from './property_binder'; import {bindRenderOutputs, collectEventListeners, bindDirectiveOutputs} from './event_binder'; -import {bindDirectiveAfterContentLifecycleCallbacks, bindDirectiveAfterViewLifecycleCallbacks, bindDirectiveDestroyLifecycleCallbacks, bindPipeDestroyLifecycleCallbacks, bindDirectiveDetectChangesLifecycleCallbacks} from './lifecycle_binder'; +import {bindDirectiveAfterContentLifecycleCallbacks, bindDirectiveAfterViewLifecycleCallbacks, bindInjectableDestroyLifecycleCallbacks, bindPipeDestroyLifecycleCallbacks, bindDirectiveDetectChangesLifecycleCallbacks} from './lifecycle_binder'; import {CompileView} from './compile_view'; import {CompileElement, CompileNode} from './compile_element'; +import {identifierToken} from '../identifiers'; export function bindView(view: CompileView, parsedTemplate: TemplateAst[]): void { var visitor = new ViewBinderVisitor(view); @@ -43,8 +44,9 @@ class ViewBinderVisitor implements TemplateAstVisitor { var eventListeners = collectEventListeners(ast.outputs, ast.directives, compileElement); bindRenderInputs(ast.inputs, compileElement); bindRenderOutputs(eventListeners); - ListWrapper.forEachWithIndex(ast.directives, (directiveAst, index) => { - var directiveInstance = compileElement.directiveInstances[index]; + ast.directives.forEach((directiveAst) => { + var directiveInstance = + compileElement.instances.get(identifierToken(directiveAst.directive.type)); bindDirectiveInputs(directiveAst, directiveInstance, compileElement); bindDirectiveDetectChangesLifecycleCallbacks(directiveAst, directiveInstance, compileElement); @@ -54,14 +56,17 @@ class ViewBinderVisitor implements TemplateAstVisitor { templateVisitAll(this, ast.children, compileElement); // afterContent and afterView lifecycles need to be called bottom up // so that children are notified before parents - ListWrapper.forEachWithIndex(ast.directives, (directiveAst, index) => { - var directiveInstance = compileElement.directiveInstances[index]; + ast.directives.forEach((directiveAst) => { + var directiveInstance = + compileElement.instances.get(identifierToken(directiveAst.directive.type)); bindDirectiveAfterContentLifecycleCallbacks( directiveAst.directive, directiveInstance, compileElement); bindDirectiveAfterViewLifecycleCallbacks( directiveAst.directive, directiveInstance, compileElement); - bindDirectiveDestroyLifecycleCallbacks( - directiveAst.directive, directiveInstance, compileElement); + }); + ast.providers.forEach((providerAst) => { + var providerInstance = compileElement.instances.get(providerAst.token); + bindInjectableDestroyLifecycleCallbacks(providerAst, providerInstance, compileElement); }); return null; } @@ -69,8 +74,9 @@ class ViewBinderVisitor implements TemplateAstVisitor { visitEmbeddedTemplate(ast: EmbeddedTemplateAst, parent: CompileElement): any { var compileElement = this.view.nodes[this._nodeIndex++]; var eventListeners = collectEventListeners(ast.outputs, ast.directives, compileElement); - ListWrapper.forEachWithIndex(ast.directives, (directiveAst, index) => { - var directiveInstance = compileElement.directiveInstances[index]; + ast.directives.forEach((directiveAst) => { + var directiveInstance = + compileElement.instances.get(identifierToken(directiveAst.directive.type)); bindDirectiveInputs(directiveAst, directiveInstance, compileElement); bindDirectiveDetectChangesLifecycleCallbacks(directiveAst, directiveInstance, compileElement); bindDirectiveOutputs(directiveAst, directiveInstance, eventListeners); @@ -78,8 +84,10 @@ class ViewBinderVisitor implements TemplateAstVisitor { directiveAst.directive, directiveInstance, compileElement); bindDirectiveAfterViewLifecycleCallbacks( directiveAst.directive, directiveInstance, compileElement); - bindDirectiveDestroyLifecycleCallbacks( - directiveAst.directive, directiveInstance, compileElement); + }); + ast.providers.forEach((providerAst) => { + var providerInstance = compileElement.instances.get(providerAst.token); + bindInjectableDestroyLifecycleCallbacks(providerAst, providerInstance, compileElement); }); bindView(compileElement.embeddedView, ast.children); return null; diff --git a/modules/@angular/compiler/test/directive_lifecycle_spec.ts b/modules/@angular/compiler/test/directive_lifecycle_spec.ts index aec62af0d7..8dbb69c17f 100644 --- a/modules/@angular/compiler/test/directive_lifecycle_spec.ts +++ b/modules/@angular/compiler/test/directive_lifecycle_spec.ts @@ -8,7 +8,7 @@ import {beforeEach, xdescribe, ddescribe, describe, expect, iit, inject, it,} from '@angular/core/testing/testing_internal'; -import {hasLifecycleHook} from '@angular/compiler/src/directive_lifecycle_reflector'; +import {hasLifecycleHook} from '@angular/compiler/src/lifecycle_reflector'; import {LifecycleHooks} from '@angular/core/src/metadata/lifecycle_hooks'; export function main() { diff --git a/modules/@angular/core/test/linker/change_detection_integration_spec.ts b/modules/@angular/core/test/linker/change_detection_integration_spec.ts index 6148c8cc9c..af4ad1af9e 100644 --- a/modules/@angular/core/test/linker/change_detection_integration_spec.ts +++ b/modules/@angular/core/test/linker/change_detection_integration_spec.ts @@ -1026,6 +1026,20 @@ export function main() { 'pipeWithOnDestroy.ngOnDestroy' ]); })); + + it('should call ngOnDestroy on an injectable class', fakeAsync(() => { + var ctx = createCompFixture( + '
', TestComponent, + tcb.overrideProviders(TestDirective, [InjectableWithLifecycle])); + ctx.debugElement.children[0].injector.get(InjectableWithLifecycle); + ctx.detectChanges(false); + + ctx.destroy(); + + expect(directiveLog.filter(['ngOnDestroy'])).toEqual([ + 'dir.ngOnDestroy', 'injectable.ngOnDestroy' + ]); + })); }); }); @@ -1386,6 +1400,14 @@ class TestDirective implements OnInit, DoCheck, OnChanges, AfterContentInit, Aft } } +@Injectable() +class InjectableWithLifecycle { + name = 'injectable'; + constructor(public log: DirectiveLog) {} + + ngOnDestroy() { this.log.add(this.name, 'ngOnDestroy'); } +} + @Directive({selector: '[orderCheck0]'}) class OrderCheckDirective0 { private _name: string; diff --git a/modules/@angular/core/test/linker/view_injector_integration_spec.ts b/modules/@angular/core/test/linker/view_injector_integration_spec.ts index ebe3b7b79e..7c767a36ae 100644 --- a/modules/@angular/core/test/linker/view_injector_integration_spec.ts +++ b/modules/@angular/core/test/linker/view_injector_integration_spec.ts @@ -384,6 +384,20 @@ export function main() { expect(created).toBe(true); })); + it('should instantiate providers with a lifecycle hook eagerly', fakeAsync(() => { + var created = false; + class SomeInjectable { + constructor() { created = true; } + ngOnDestroy() {} + } + + var el = createComp( + '
', + tcb.overrideProviders(SimpleDirective, [SomeInjectable])); + + expect(created).toBe(true); + })); + it('should instantiate view providers lazily', fakeAsync(() => { var created = false; var el = createComp(