diff --git a/integration/_payload-limits.json b/integration/_payload-limits.json index 0be7eba374..6b6bcef962 100644 --- a/integration/_payload-limits.json +++ b/integration/_payload-limits.json @@ -21,7 +21,7 @@ "master": { "uncompressed": { "runtime": 1440, - "main": 207765, + "main": 209904, "polyfills": 38390 } } diff --git a/packages/core/src/di/r3_injector.ts b/packages/core/src/di/r3_injector.ts index b1415fd970..62c0691211 100644 --- a/packages/core/src/di/r3_injector.ts +++ b/packages/core/src/di/r3_injector.ts @@ -467,6 +467,10 @@ export function isTypeProvider(value: SingleProvider): value is TypeProvider { return typeof value === 'function'; } +export function isClassProvider(value: SingleProvider): value is ClassProvider { + return !!(value as StaticClassProvider | ClassProvider).useClass; +} + function hasDeps(value: ClassProvider | ConstructorProvider | StaticClassProvider): value is ClassProvider&{deps: any[]} { return !!(value as any).deps; diff --git a/packages/core/src/render3/di.ts b/packages/core/src/render3/di.ts index 56accc32b5..981a9952c7 100644 --- a/packages/core/src/render3/di.ts +++ b/packages/core/src/render3/di.ts @@ -520,10 +520,6 @@ export function getNodeInjectable( setTNodeAndViewData(tNode, lData); try { value = lData[index] = factory.factory(null, tData, lData, tNode); - const tView = lData[TVIEW]; - if (value && factory.isProvider && value.ngOnDestroy) { - (tView.destroyHooks || (tView.destroyHooks = [])).push(index, value.ngOnDestroy); - } } finally { if (factory.injectImpl) setInjectImplementation(previousInjectImplementation); setIncludeViewProviders(previousIncludeViewProviders); diff --git a/packages/core/src/render3/di_setup.ts b/packages/core/src/render3/di_setup.ts index 9db9bcce86..15286a4b77 100644 --- a/packages/core/src/render3/di_setup.ts +++ b/packages/core/src/render3/di_setup.ts @@ -8,8 +8,8 @@ import {resolveForwardRef} from '../di/forward_ref'; -import {Provider} from '../di/interface/provider'; -import {isTypeProvider, providerToFactory} from '../di/r3_injector'; +import {ClassProvider, Provider} from '../di/interface/provider'; +import {isClassProvider, isTypeProvider, providerToFactory} from '../di/r3_injector'; import {DirectiveDef} from '.'; import {diPublicInInjector, getNodeInjectable, getOrCreateNodeInjectorForNode} from './di'; @@ -81,10 +81,19 @@ function resolveProvider( const cptViewProvidersCount = tNode.providerIndexes >> TNodeProviderIndexes.CptViewProvidersCountShift; + if (isClassProvider(provider) || isTypeProvider(provider)) { + const prototype = ((provider as ClassProvider).useClass || provider).prototype; + const ngOnDestroy = prototype.ngOnDestroy; + + if (ngOnDestroy) { + const tView = lView[TVIEW]; + (tView.destroyHooks || (tView.destroyHooks = [])).push(tInjectables.length, ngOnDestroy); + } + } + if (isTypeProvider(provider) || !provider.multi) { // Single provider case: the factory is created and pushed immediately - const factory = - new NodeInjectorFactory(providerFactory, isViewProvider, true, directiveInject); + const factory = new NodeInjectorFactory(providerFactory, isViewProvider, directiveInject); const existingFactoryIndex = indexOf( token, tInjectables, isViewProvider ? beginIndex : beginIndex + cptViewProvidersCount, endIndex); @@ -246,7 +255,7 @@ function multiFactory( this: NodeInjectorFactory, _: null, tData: TData, lData: LView, tNode: TElementNode) => any, index: number, isViewProvider: boolean, isComponent: boolean, f: () => any): NodeInjectorFactory { - const factory = new NodeInjectorFactory(factoryFn, isViewProvider, true, directiveInject); + const factory = new NodeInjectorFactory(factoryFn, isViewProvider, directiveInject); factory.multi = []; factory.index = index; factory.componentProviders = 0; diff --git a/packages/core/src/render3/instructions.ts b/packages/core/src/render3/instructions.ts index 76576b8045..2c8b009b06 100644 --- a/packages/core/src/render3/instructions.ts +++ b/packages/core/src/render3/instructions.ts @@ -2067,8 +2067,7 @@ function baseResolveDirective( tView: TView, viewData: LView, def: DirectiveDef, directiveFactory: (t: Type| null) => any) { tView.data.push(def); - const nodeInjectorFactory = - new NodeInjectorFactory(directiveFactory, isComponentDef(def), false, null); + const nodeInjectorFactory = new NodeInjectorFactory(directiveFactory, isComponentDef(def), null); tView.blueprint.push(nodeInjectorFactory); viewData.push(nodeInjectorFactory); } diff --git a/packages/core/src/render3/interfaces/injector.ts b/packages/core/src/render3/interfaces/injector.ts index 9b8a6695e8..e3c0e5541e 100644 --- a/packages/core/src/render3/interfaces/injector.ts +++ b/packages/core/src/render3/interfaces/injector.ts @@ -235,10 +235,6 @@ export class NodeInjectorFactory { * Set to `true` if the token is declared in `viewProviders` (or if it is component). */ isViewProvider: boolean, - /** - * Set to `true` if the token is a provider, and not a directive. - */ - public isProvider: boolean, injectImplementation: null|((token: Type|InjectionToken, flags: InjectFlags) => T)) { this.canSeeViewProviders = isViewProvider; this.injectImpl = injectImplementation; diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index 8c02f94fa8..b7a54b693d 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -9,9 +9,9 @@ import {ViewEncapsulation} from '../metadata/view'; import {attachPatchData} from './context_discovery'; -import {callHooks} from './hooks'; import {LContainer, NATIVE, VIEWS, unusedValueExportToPlacateAjd as unused1} from './interfaces/container'; import {ComponentDef} from './interfaces/definition'; +import {NodeInjectorFactory} from './interfaces/injector'; import {TContainerNode, TElementContainerNode, TElementNode, TNode, TNodeFlags, TNodeType, TViewNode, unusedValueExportToPlacateAjd as unused2} from './interfaces/node'; import {unusedValueExportToPlacateAjd as unused3} from './interfaces/projection'; import {ProceduralRenderer3, RComment, RElement, RNode, RText, Renderer3, isProceduralRenderer, unusedValueExportToPlacateAjd as unused4} from './interfaces/renderer'; @@ -495,8 +495,16 @@ function removeListeners(lView: LView): void { function executeOnDestroys(view: LView): void { const tView = view[TVIEW]; let destroyHooks: HookData|null; + if (tView != null && (destroyHooks = tView.destroyHooks) != null) { - callHooks(view, destroyHooks); + for (let i = 0; i < destroyHooks.length; i += 2) { + const context = view[destroyHooks[i] as number]; + + // Only call the destroy hook if the context has been requested. + if (!(context instanceof NodeInjectorFactory)) { + (destroyHooks[i + 1] as() => void).call(context); + } + } } } diff --git a/packages/core/test/acceptance/component_spec.ts b/packages/core/test/acceptance/component_spec.ts new file mode 100644 index 0000000000..c5c608f479 --- /dev/null +++ b/packages/core/test/acceptance/component_spec.ts @@ -0,0 +1,52 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {Component, InjectionToken} from '@angular/core'; +import {TestBed} from '@angular/core/testing'; + + +describe('component', () => { + describe('view destruction', () => { + it('should invoke onDestroy only once when a component is registered as a provider', () => { + const testToken = new InjectionToken('testToken'); + let destroyCalls = 0; + + @Component({ + selector: 'comp-with-on-destroy', + template: '', + providers: [{provide: testToken, useExisting: ParentWithOnDestroy}] + }) + class ParentWithOnDestroy { + ngOnDestroy() { destroyCalls++; } + } + + @Component({selector: 'child', template: ''}) + class ChildComponent { + // We need to inject the parent so the provider is instantiated. + constructor(_parent: ParentWithOnDestroy) {} + } + + @Component({ + template: ` + + + + ` + }) + class App { + } + + TestBed.configureTestingModule({declarations: [App, ParentWithOnDestroy, ChildComponent]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + fixture.destroy(); + + expect(destroyCalls).toBe(1, 'Expected `ngOnDestroy` to only be called once.'); + }); + }); +}); diff --git a/packages/core/test/acceptance/providers_spec.ts b/packages/core/test/acceptance/providers_spec.ts new file mode 100644 index 0000000000..e5c6d0bd4b --- /dev/null +++ b/packages/core/test/acceptance/providers_spec.ts @@ -0,0 +1,184 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {Component, Injectable} from '@angular/core'; +import {TestBed} from '@angular/core/testing'; +import {onlyInIvy} from '@angular/private/testing'; + + +describe('providers', () => { + describe('lifecycles', () => { + it('should inherit ngOnDestroy hooks on providers', () => { + const logs: string[] = []; + + @Injectable() + class SuperInjectableWithDestroyHook { + ngOnDestroy() { logs.push('OnDestroy'); } + } + + @Injectable() + class SubInjectableWithDestroyHook extends SuperInjectableWithDestroyHook { + } + + @Component({template: '', providers: [SubInjectableWithDestroyHook]}) + class App { + constructor(foo: SubInjectableWithDestroyHook) {} + } + + TestBed.configureTestingModule({declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + fixture.destroy(); + + expect(logs).toEqual(['OnDestroy']); + }); + + it('should not call ngOnDestroy for providers that have not been requested', () => { + const logs: string[] = []; + + @Injectable() + class InjectableWithDestroyHook { + ngOnDestroy() { logs.push('OnDestroy'); } + } + + @Component({template: '', providers: [InjectableWithDestroyHook]}) + class App { + } + + TestBed.configureTestingModule({declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + fixture.destroy(); + + expect(logs).toEqual([]); + }); + + it('should only call ngOnDestroy once for multiple instances', () => { + const logs: string[] = []; + + @Injectable() + class InjectableWithDestroyHook { + ngOnDestroy() { logs.push('OnDestroy'); } + } + + @Component({selector: 'my-cmp', template: ''}) + class MyComponent { + constructor(foo: InjectableWithDestroyHook) {} + } + + @Component({ + template: ` + + + `, + providers: [InjectableWithDestroyHook] + }) + class App { + } + + TestBed.configureTestingModule({declarations: [App, MyComponent]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + fixture.destroy(); + + expect(logs).toEqual(['OnDestroy']); + }); + + it('should call ngOnDestroy when providing same token via useClass', () => { + const logs: string[] = []; + + @Injectable() + class InjectableWithDestroyHook { + ngOnDestroy() { logs.push('OnDestroy'); } + } + + @Component({ + template: '', + providers: [{provide: InjectableWithDestroyHook, useClass: InjectableWithDestroyHook}] + }) + class App { + constructor(foo: InjectableWithDestroyHook) {} + } + + TestBed.configureTestingModule({declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + fixture.destroy(); + + expect(logs).toEqual(['OnDestroy']); + }); + + onlyInIvy('Destroy hook of useClass provider is invoked correctly') + .it('should only call ngOnDestroy of value when providing via useClass', () => { + const logs: string[] = []; + + @Injectable() + class InjectableWithDestroyHookToken { + ngOnDestroy() { logs.push('OnDestroy Token'); } + } + + @Injectable() + class InjectableWithDestroyHookValue { + ngOnDestroy() { logs.push('OnDestroy Value'); } + } + + @Component({ + template: '', + providers: [ + {provide: InjectableWithDestroyHookToken, useClass: InjectableWithDestroyHookValue} + ] + }) + class App { + constructor(foo: InjectableWithDestroyHookToken) {} + } + + TestBed.configureTestingModule({declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + fixture.destroy(); + + expect(logs).toEqual(['OnDestroy Value']); + }); + + it('should only call ngOnDestroy of value when providing via useExisting', () => { + const logs: string[] = []; + + @Injectable() + class InjectableWithDestroyHookToken { + ngOnDestroy() { logs.push('OnDestroy Token'); } + } + + @Injectable() + class InjectableWithDestroyHookExisting { + ngOnDestroy() { logs.push('OnDestroy Existing'); } + } + + @Component({ + template: '', + providers: [ + InjectableWithDestroyHookExisting, { + provide: InjectableWithDestroyHookToken, + useExisting: InjectableWithDestroyHookExisting + } + ] + }) + class App { + constructor(foo1: InjectableWithDestroyHookExisting, foo2: InjectableWithDestroyHookToken) { + } + } + + TestBed.configureTestingModule({declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + fixture.destroy(); + + expect(logs).toEqual(['OnDestroy Existing']); + }); + + }); +}); diff --git a/packages/core/test/render3/component_spec.ts b/packages/core/test/render3/component_spec.ts index 96858abf48..863754af0c 100644 --- a/packages/core/test/render3/component_spec.ts +++ b/packages/core/test/render3/component_spec.ts @@ -6,9 +6,9 @@ * found in the LICENSE file at https://angular.io/license */ -import {ViewEncapsulation, defineInjectable, defineInjector} from '../../src/core'; +import {InjectionToken, ViewEncapsulation, defineInjectable, defineInjector} from '../../src/core'; -import {AttributeMarker, ComponentFactory, LifecycleHooksFeature, defineComponent, directiveInject, markDirty, template, getRenderedText} from '../../src/render3/index'; +import {AttributeMarker, ComponentFactory, LifecycleHooksFeature, defineComponent, directiveInject, markDirty, template, getRenderedText, ProvidersFeature} from '../../src/render3/index'; import {bind, container, containerRefreshEnd, containerRefreshStart, element, elementEnd, elementProperty, elementStart, embeddedViewEnd, embeddedViewStart, nextContext, text, textBinding, tick} from '../../src/render3/instructions'; import {ComponentDef, RenderFlags} from '../../src/render3/interfaces/definition'; @@ -669,22 +669,22 @@ describe('recursive components', () => { }); describe('view destruction', () => { - let wasOnDestroyCalled = false; - - class ComponentWithOnDestroy { - static ngComponentDef = defineComponent({ - selectors: [['comp-with-destroy']], - type: ComponentWithOnDestroy, - consts: 0, - vars: 0, - factory: () => new ComponentWithOnDestroy(), - template: (rf: any, ctx: any) => {}, - }); - - ngOnDestroy() { wasOnDestroyCalled = true; } - } - it('should invoke onDestroy when directly destroying a root view', () => { + let wasOnDestroyCalled = false; + + class ComponentWithOnDestroy { + static ngComponentDef = defineComponent({ + selectors: [['comp-with-destroy']], + type: ComponentWithOnDestroy, + consts: 0, + vars: 0, + factory: () => new ComponentWithOnDestroy(), + template: (rf: any, ctx: any) => {}, + }); + + ngOnDestroy() { wasOnDestroyCalled = true; } + } + // This test asserts that the view tree is set up correctly based on the knowledge that this // tree is used during view destruction. If the child view is not correctly attached as a // child of the root view, then the onDestroy hook on the child view will never be called @@ -704,4 +704,5 @@ describe('view destruction', () => { true, 'Expected component onDestroy method to be called when its parent view is destroyed'); }); + }); diff --git a/packages/core/test/render3/providers_spec.ts b/packages/core/test/render3/providers_spec.ts index 4b6732dacf..5de7ee1360 100644 --- a/packages/core/test/render3/providers_spec.ts +++ b/packages/core/test/render3/providers_spec.ts @@ -1355,6 +1355,7 @@ describe('providers', () => { expect(fixture.html).toEqual('
'); expect(logs).toEqual(['Injectable OnDestroy']); }); + }); }); interface ComponentTest { diff --git a/tools/material-ci/angular_material_test_blocklist.js b/tools/material-ci/angular_material_test_blocklist.js index ef89c77317..5994b27134 100644 --- a/tools/material-ci/angular_material_test_blocklist.js +++ b/tools/material-ci/angular_material_test_blocklist.js @@ -237,46 +237,6 @@ window.testBlocklist = { "error": "TypeError: Cannot read property 'accordion' of undefined", "notes": "Unknown" }, - "CdkDrag standalone draggable should enable native drag interactions when there is a drag handle": { - "error": "TypeError: Cannot read property 'removeEventListener' of null", - "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider" - }, - "CdkDrag draggable with a handle should not be able to drag the entire element if it has a handle": { - "error": "TypeError: Cannot read property 'removeEventListener' of null", - "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider" - }, - "CdkDrag draggable with a handle should be able to drag an element using its handle": { - "error": "TypeError: Cannot read property 'removeEventListener' of null", - "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider" - }, - "CdkDrag draggable with a handle should not be able to drag the element if the handle is disabled": { - "error": "TypeError: Cannot read property 'removeEventListener' of null", - "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider" - }, - "CdkDrag draggable with a handle should not be able to drag using the handle if the element is disabled": { - "error": "TypeError: Cannot read property 'removeEventListener' of null", - "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider" - }, - "CdkDrag draggable with a handle should be able to use a handle that was added after init": { - "error": "TypeError: Cannot read property 'removeEventListener' of null", - "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider" - }, - "CdkDrag draggable with a handle should be able to use more than one handle to drag the element": { - "error": "TypeError: Cannot read property 'removeEventListener' of null", - "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider" - }, - "CdkDrag draggable with a handle should be able to drag with a handle that is not a direct descendant": { - "error": "TypeError: Cannot read property 'removeEventListener' of null", - "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider" - }, - "CdkDrag draggable with a handle should disable the tap highlight while dragging via the handle": { - "error": "TypeError: Cannot read property 'removeEventListener' of null", - "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider" - }, - "CdkDrag draggable with a handle should preserve any existing `webkitTapHighlightColor`": { - "error": "TypeError: Cannot read property 'removeEventListener' of null", - "notes": "FW-1010: onDestroy hook is called twice for directives that are also used in a provider" - }, "CdkDrag in a drop container should be able to customize the preview element": { "error": "Error: Expected cdk-drag cdk-drag-preview to contain 'custom-preview'.", "notes": "Unknown"