cargdev/angular
1
0
Fork 0
Code Issues 5 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(core): ngOnDestroy on multi providers called with incorrect context (#35840)

Browse Source 
Currently destroy hooks are stored in memory as `[1, hook, 5, hook]` where
the numbers represent the index at which to find the context and `hook` is
the function to be invoked. This breaks down for `multi` providers,
because the value at the index will be an array of providers, resulting in
the hook being invoked with an array of all the multi provider values,
rather than the provider that was destroyed. In ViewEngine `ngOnDestroy`
wasn't being called for `multi` providers at all.

These changes fix the issue by changing the structure of the destroy hooks to `[1, hook, 5, [0, hook, 3, hook]]` where the indexes inside the inner array point to the provider inside of the multi provider array. Note that this is slightly different from the original design which called for the structure to be `[1, hook, 5, [hook, hook]`, because in the process of implementing it, I realized that we wouldn't get passing the correct context if only some of the `multi` providers have `ngOnDestroy` and others don't.

I've run the newly-added `view_destroy_hooks` benchmark against these changes and compared it to master. The difference seems to be insignificant (between 1% and 2% slower).

Fixes #35231.

PR Close #35840
This commit is contained in:
crisbeto
2020-03-03 21:05:26 +01:00
committed by Kara Erickson
parent 915a6c17b9
commit af4269471d
8 changed files with 579 additions and 296 deletions
Download Patch File Download Diff File Expand all files Collapse all files

241
packages/core/test/acceptance/providers_spec.ts
View File

@ -6,16 +6,14 @@
* found in the LICENSE file at https://angular.io/license
*/
import {Component, Directive, Inject, Injectable, InjectionToken, Injector, NgModule, Optional, forwardRef} from '@angular/core';
import {TestBed, async, inject} from '@angular/core/testing';
import {Component, Directive, forwardRef, Inject, Injectable, InjectionToken, Injector, NgModule, Optional} from '@angular/core';
import {async, inject, TestBed} from '@angular/core/testing';
import {By} from '@angular/platform-browser';
import {expect} from '@angular/platform-browser/testing/src/matchers';
import {onlyInIvy} from '@angular/private/testing';
import {modifiedInIvy, onlyInIvy} from '@angular/private/testing';
describe('providers', () => {
describe('inheritance', () => {
it('should NOT inherit providers', () => {
const SOME_DIRS = new InjectionToken('someDirs');
@ -52,7 +50,6 @@ describe('providers', () => {
expect(otherDir.dirs.length).toEqual(1);
expect(otherDir.dirs[0] instanceof SubDirective).toBe(true);
});
});
describe('lifecycles', () => {
@ -61,7 +58,9 @@ describe('providers', () => {
@Injectable()
class SuperInjectableWithDestroyHook {
ngOnDestroy() { logs.push('OnDestroy'); }
ngOnDestroy() {
logs.push('OnDestroy');
}
}
@Injectable()
@ -86,7 +85,9 @@ describe('providers', () => {
@Injectable()
class InjectableWithDestroyHook {
ngOnDestroy() { logs.push('OnDestroy'); }
ngOnDestroy() {
logs.push('OnDestroy');
}
}
@Component({template: '', providers: [InjectableWithDestroyHook]})
@ -106,7 +107,9 @@ describe('providers', () => {
@Injectable()
class InjectableWithDestroyHook {
ngOnDestroy() { logs.push('OnDestroy'); }
ngOnDestroy() {
logs.push('OnDestroy');
}
}
@Component({selector: 'my-cmp', template: ''})
@ -137,7 +140,9 @@ describe('providers', () => {
@Injectable()
class InjectableWithDestroyHook {
ngOnDestroy() { logs.push('OnDestroy'); }
ngOnDestroy() {
logs.push('OnDestroy');
}
}
@Component({
@ -162,12 +167,16 @@ describe('providers', () => {
@Injectable()
class InjectableWithDestroyHookToken {
ngOnDestroy() { logs.push('OnDestroy Token'); }
ngOnDestroy() {
logs.push('OnDestroy Token');
}
}
@Injectable()
class InjectableWithDestroyHookValue {
ngOnDestroy() { logs.push('OnDestroy Value'); }
ngOnDestroy() {
logs.push('OnDestroy Value');
}
}
@Component({
@ -193,21 +202,23 @@ describe('providers', () => {
@Injectable()
class InjectableWithDestroyHookToken {
ngOnDestroy() { logs.push('OnDestroy Token'); }
ngOnDestroy() {
logs.push('OnDestroy Token');
}
}
@Injectable()
class InjectableWithDestroyHookExisting {
ngOnDestroy() { logs.push('OnDestroy Existing'); }
ngOnDestroy() {
logs.push('OnDestroy Existing');
}
}
@Component({
template: '',
providers: [
InjectableWithDestroyHookExisting, {
provide: InjectableWithDestroyHookToken,
useExisting: InjectableWithDestroyHookExisting
}
InjectableWithDestroyHookExisting,
{provide: InjectableWithDestroyHookToken, useExisting: InjectableWithDestroyHookExisting}
]
})
class App {
@ -225,30 +236,40 @@ describe('providers', () => {
it('should invoke ngOnDestroy with the correct context when providing a type provider multiple times on the same node',
() => {
const resolvedServices: (DestroyService | undefined)[] = [];
const destroyContexts: (DestroyService | undefined)[] = [];
const resolvedServices: (DestroyService|undefined)[] = [];
const destroyContexts: (DestroyService|undefined)[] = [];
let parentService: DestroyService|undefined;
let childService: DestroyService|undefined;
@Injectable()
class DestroyService {
constructor() { resolvedServices.push(this); }
ngOnDestroy() { destroyContexts.push(this); }
constructor() {
resolvedServices.push(this);
}
ngOnDestroy() {
destroyContexts.push(this);
}
}
@Directive({selector: '[dir-one]', providers: [DestroyService]})
class DirOne {
constructor(service: DestroyService) { childService = service; }
constructor(service: DestroyService) {
childService = service;
}
}
@Directive({selector: '[dir-two]', providers: [DestroyService]})
class DirTwo {
constructor(service: DestroyService) { childService = service; }
constructor(service: DestroyService) {
childService = service;
}
}
@Component({template: '<div dir-one dir-two></div>', providers: [DestroyService]})
class App {
constructor(service: DestroyService) { parentService = service; }
constructor(service: DestroyService) {
parentService = service;
}
}
TestBed.configureTestingModule({declarations: [App, DirOne, DirTwo]});
@ -266,28 +287,36 @@ describe('providers', () => {
onlyInIvy('Destroy hook of useClass provider is invoked correctly')
.it('should invoke ngOnDestroy with the correct context when providing a class provider multiple times on the same node',
() => {
const resolvedServices: (DestroyService | undefined)[] = [];
const destroyContexts: (DestroyService | undefined)[] = [];
const resolvedServices: (DestroyService|undefined)[] = [];
const destroyContexts: (DestroyService|undefined)[] = [];
const token = new InjectionToken<any>('token');
let parentService: DestroyService|undefined;
let childService: DestroyService|undefined;
@Injectable()
class DestroyService {
constructor() { resolvedServices.push(this); }
ngOnDestroy() { destroyContexts.push(this); }
constructor() {
resolvedServices.push(this);
}
ngOnDestroy() {
destroyContexts.push(this);
}
}
@Directive(
{selector: '[dir-one]', providers: [{provide: token, useClass: DestroyService}]})
class DirOne {
constructor(@Inject(token) service: DestroyService) { childService = service; }
constructor(@Inject(token) service: DestroyService) {
childService = service;
}
}
@Directive(
{selector: '[dir-two]', providers: [{provide: token, useClass: DestroyService}]})
class DirTwo {
constructor(@Inject(token) service: DestroyService) { childService = service; }
constructor(@Inject(token) service: DestroyService) {
childService = service;
}
}
@Component({
@ -295,7 +324,9 @@ describe('providers', () => {
providers: [{provide: token, useClass: DestroyService}]
})
class App {
constructor(@Inject(token) service: DestroyService) { parentService = service; }
constructor(@Inject(token) service: DestroyService) {
parentService = service;
}
}
TestBed.configureTestingModule({declarations: [App, DirOne, DirTwo]});
@ -310,21 +341,151 @@ describe('providers', () => {
expect(destroyContexts).toEqual([parentService, childService]);
});
onlyInIvy('ngOnDestroy hooks for multi providers were not supported in ViewEngine')
.describe('ngOnDestroy on multi providers', () => {
it('should invoke ngOnDestroy on multi providers with the correct context', () => {
const destroyCalls: any[] = [];
const SERVICES = new InjectionToken<any>('SERVICES');
@Injectable()
class DestroyService {
ngOnDestroy() {
destroyCalls.push(this);
}
}
@Injectable()
class OtherDestroyService {
ngOnDestroy() {
destroyCalls.push(this);
}
}
@Component({
template: '<div></div>',
providers: [
{provide: SERVICES, useClass: DestroyService, multi: true},
{provide: SERVICES, useClass: OtherDestroyService, multi: true},
]
})
class App {
constructor(@Inject(SERVICES) s: any) {}
}
TestBed.configureTestingModule({declarations: [App]});
const fixture = TestBed.createComponent(App);
fixture.detectChanges();
fixture.destroy();
expect(destroyCalls).toEqual([
jasmine.any(DestroyService), jasmine.any(OtherDestroyService)
]);
});
it('should invoke destroy hooks on multi providers with the correct context, if only some have a destroy hook',
() => {
const destroyCalls: any[] = [];
const SERVICES = new InjectionToken<any>('SERVICES');
@Injectable()
class Service1 {
}
@Injectable()
class Service2 {
ngOnDestroy() {
destroyCalls.push(this);
}
}
@Injectable()
class Service3 {
}
@Injectable()
class Service4 {
ngOnDestroy() {
destroyCalls.push(this);
}
}
@Component({
template: '<div></div>',
providers: [
{provide: SERVICES, useClass: Service1, multi: true},
{provide: SERVICES, useClass: Service2, multi: true},
{provide: SERVICES, useClass: Service3, multi: true},
{provide: SERVICES, useClass: Service4, multi: true},
]
})
class App {
constructor(@Inject(SERVICES) s: any) {}
}
TestBed.configureTestingModule({declarations: [App]});
const fixture = TestBed.createComponent(App);
fixture.detectChanges();
fixture.destroy();
expect(destroyCalls).toEqual([jasmine.any(Service2), jasmine.any(Service4)]);
});
it('should not invoke ngOnDestroy on multi providers created via useFactory', () => {
let destroyCalls = 0;
const SERVICES = new InjectionToken<any>('SERVICES');
@Injectable()
class DestroyService {
ngOnDestroy() {
destroyCalls++;
}
}
@Injectable()
class OtherDestroyService {
ngOnDestroy() {
destroyCalls++;
}
}
@Component({
template: '<div></div>',
providers: [
{provide: SERVICES, useFactory: () => new DestroyService(), multi: true},
{provide: SERVICES, useFactory: () => new OtherDestroyService(), multi: true},
]
})
class App {
constructor(@Inject(SERVICES) s: any) {}
}
TestBed.configureTestingModule({declarations: [App]});
const fixture = TestBed.createComponent(App);
fixture.detectChanges();
fixture.destroy();
expect(destroyCalls).toBe(0);
});
});
modifiedInIvy('ViewEngine did not support destroy hooks on multi providers')
.it('should not invoke ngOnDestroy on multi providers', () => {
// TODO(FW-1866): currently we only assert that the hook was called,
// but we should also be checking that the correct context was passed in.
let destroyCalls = 0;
const SERVICES = new InjectionToken<any>('SERVICES');
@Injectable()
class DestroyService {
ngOnDestroy() { destroyCalls++; }
ngOnDestroy() {
destroyCalls++;
}
}
@Injectable()
class OtherDestroyService {
ngOnDestroy() { destroyCalls++; }
ngOnDestroy() {
destroyCalls++;
}
}
@Component({
@ -343,13 +504,11 @@ describe('providers', () => {
fixture.detectChanges();
fixture.destroy();
expect(destroyCalls).toBe(2);
expect(destroyCalls).toBe(0);
});
});
describe('components and directives', () => {
class MyService {
value = 'some value';
}
@ -409,7 +568,6 @@ describe('providers', () => {
});
describe('forward refs', () => {
it('should support forward refs in provider deps', () => {
class MyService {
constructor(public dep: {value: string}) {}
@ -444,7 +602,6 @@ describe('providers', () => {
});
it('should support forward refs in useClass when impl version is also provided', () => {
@Injectable({providedIn: 'root', useClass: forwardRef(() => SomeProviderImpl)})
abstract class SomeProvider {
}
@ -490,11 +647,9 @@ describe('providers', () => {
expect(fixture.componentInstance.foo).toBeAnInstanceOf(SomeProviderImpl);
});
});
describe('flags', () => {
class MyService {
constructor(public value: OtherService|null) {}
}