diff --git a/goldens/public-api/forms/forms.d.ts b/goldens/public-api/forms/forms.d.ts index 24129a61570b..3e99da38cc6b 100644 --- a/goldens/public-api/forms/forms.d.ts +++ b/goldens/public-api/forms/forms.d.ts @@ -100,6 +100,8 @@ export declare class AbstractFormGroupDirective extends ControlContainer impleme get control(): FormGroup; get formDirective(): Form | null; get path(): string[]; + get refCount(): number; + addRef(): void; ngOnDestroy(): void; ngOnInit(): void; } diff --git a/packages/core/src/di/r3_injector.ts b/packages/core/src/di/r3_injector.ts index ab941aa62e3a..067437a5c241 100644 --- a/packages/core/src/di/r3_injector.ts +++ b/packages/core/src/di/r3_injector.ts @@ -542,7 +542,7 @@ function isExistingProvider(value: SingleProvider): value is ExistingProvider { return !!(value && (value as ExistingProvider).useExisting); } -function isFactoryProvider(value: SingleProvider): value is FactoryProvider { +export function isFactoryProvider(value: SingleProvider): value is FactoryProvider { return !!(value && (value as FactoryProvider).useFactory); } diff --git a/packages/core/src/render3/di_setup.ts b/packages/core/src/render3/di_setup.ts index 0791a67c5580..836827367d1f 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 {ClassProvider, Provider} from '../di/interface/provider'; -import {isClassProvider, isTypeProvider, providerToFactory} from '../di/r3_injector'; +import {ClassProvider, Provider, TypeProvider} from '../di/interface/provider'; +import {isClassProvider, isFactoryProvider, isTypeProvider, providerToFactory} from '../di/r3_injector'; import {assertDefined} from '../util/assert'; import {diPublicInInjector, getNodeInjectable, getOrCreateNodeInjectorForNode} from './di'; @@ -179,6 +179,18 @@ function resolveProvider( } } +/** + * Function is used as a destroy hook for Factory Providers. This function is invoked in a context + * of a provider instance, checks whether there is an `ngOnDestroy` function and invokes it if it's + * present. + */ +function ngOnDestroyForFactoryProviders(this: any) { + const onDestroy = this && this.ngOnDestroy; + if (onDestroy) { + onDestroy.call(this); + } +} + /** * Registers the `ngOnDestroy` hook of a provider, if the provider supports destroy hooks. * @param tView `TView` in which to register the hook. @@ -190,27 +202,36 @@ function resolveProvider( function registerDestroyHooksIfSupported( tView: TView, provider: Exclude, contextIndex: number, indexInFactory?: number) { - const providerIsTypeProvider = isTypeProvider(provider); - if (providerIsTypeProvider || isClassProvider(provider)) { - const prototype = ((provider as ClassProvider).useClass || provider).prototype; - const ngOnDestroy = prototype.ngOnDestroy; - if (ngOnDestroy) { - const hooks = tView.destroyHooks || (tView.destroyHooks = []); + let providerIsClassProvider = false; + let ngOnDestroy; + if (isTypeProvider(provider)) { + ngOnDestroy = (provider as TypeProvider).prototype.ngOnDestroy; + } else if (isClassProvider(provider)) { + providerIsClassProvider = true; + ngOnDestroy = (provider as ClassProvider).useClass.prototype.ngOnDestroy; + } else if (isFactoryProvider(provider)) { + // For factory providers, since we do not have a way to identify whether there is an + // `ngOnDestroy` hook before calling a factory function, we register a special function + // that would be invoked at destroy time and check whether `ngOnDestroy` is present on a + // provider instance (and call the function if it exists). + ngOnDestroy = ngOnDestroyForFactoryProviders; + } + if (ngOnDestroy) { + const hooks = tView.destroyHooks || (tView.destroyHooks = []); - if (!providerIsTypeProvider && ((provider as ClassProvider)).multi) { - ngDevMode && - assertDefined( - indexInFactory, 'indexInFactory when registering multi factory destroy hook'); - const existingCallbacksIndex = hooks.indexOf(contextIndex); + if (providerIsClassProvider && ((provider as ClassProvider)).multi) { + ngDevMode && + assertDefined( + indexInFactory, 'indexInFactory when registering multi factory destroy hook'); + const existingCallbacksIndex = hooks.indexOf(contextIndex); - if (existingCallbacksIndex === -1) { - hooks.push(contextIndex, [indexInFactory, ngOnDestroy]); - } else { - (hooks[existingCallbacksIndex + 1] as DestroyHookData).push(indexInFactory!, ngOnDestroy); - } + if (existingCallbacksIndex === -1) { + hooks.push(contextIndex, [indexInFactory, ngOnDestroy]); } else { - hooks.push(contextIndex, ngOnDestroy); + (hooks[existingCallbacksIndex + 1] as DestroyHookData).push(indexInFactory!, ngOnDestroy); } + } else { + hooks.push(contextIndex, ngOnDestroy); } } } diff --git a/packages/core/src/view/provider.ts b/packages/core/src/view/provider.ts index b696f9bfb8fa..8abd9db59ea4 100644 --- a/packages/core/src/view/provider.ts +++ b/packages/core/src/view/provider.ts @@ -256,8 +256,24 @@ function _createProviderInstance(view: ViewData, def: NodeDef): any { return createClass( view, def.parent!, allowPrivateServices, providerDef!.value, providerDef!.deps); case NodeFlags.TypeFactoryProvider: - return callFactory( + let injectable: any; + injectable = callFactory( view, def.parent!, allowPrivateServices, providerDef!.value, providerDef!.deps); + // Update def flags if factory injectable has ngOnDestroy function. + if (injectable != null && typeof injectable === 'object' && + !(def.flags & NodeFlags.OnDestroy) && typeof injectable.ngOnDestroy === 'function') { + def.flags |= NodeFlags.OnDestroy; + view.def.nodeFlags |= NodeFlags.OnDestroy; + let parent: NodeDef|null = def.parent; + if (parent) { + parent.directChildFlags |= NodeFlags.OnDestroy; + while (parent !== null && !(parent.childFlags & NodeFlags.OnDestroy)) { + parent.childFlags |= NodeFlags.OnDestroy; + parent = parent.parent; + } + } + } + return injectable; case NodeFlags.TypeUseExistingProvider: return resolveDep(view, def.parent!, allowPrivateServices, providerDef!.deps[0]); case NodeFlags.TypeValueProvider: @@ -573,6 +589,9 @@ function callProviderLifecycles( provider.ngAfterViewChecked(); } if (lifecycles & NodeFlags.OnDestroy) { - provider.ngOnDestroy(); + const onDestroy = provider && provider.ngOnDestroy; + if (onDestroy) { + onDestroy.call(provider); + } } } diff --git a/packages/core/test/acceptance/providers_spec.ts b/packages/core/test/acceptance/providers_spec.ts index ec0be8cf2e38..b46e685042ec 100644 --- a/packages/core/test/acceptance/providers_spec.ts +++ b/packages/core/test/acceptance/providers_spec.ts @@ -7,7 +7,7 @@ */ import {CommonModule} from '@angular/common'; -import {Component, Directive, forwardRef, Inject, Injectable, InjectionToken, Injector, NgModule, Optional} from '@angular/core'; +import {Component, Directive, ElementRef, forwardRef, Inject, Injectable, InjectionToken, Injector, NgModule, Optional} from '@angular/core'; import {inject, TestBed, waitForAsync} from '@angular/core/testing'; import {By} from '@angular/platform-browser'; import {expect} from '@angular/platform-browser/testing/src/matchers'; @@ -508,6 +508,280 @@ describe('providers', () => { expect(destroyCalls).toBe(0); }); + it('should invoke ngOnDestroy for useFactory providers provided in Component', () => { + const logs: string[] = []; + + @Injectable() + class InjectableWithDestroyHook { + ngOnDestroy() { + logs.push('OnDestroy'); + } + } + + @Component({ + template: '', + providers: [{ + provide: InjectableWithDestroyHook, + useFactory: () => new InjectableWithDestroyHook(), + }], + }) + class App { + constructor(public injectableWithDestroyHook: InjectableWithDestroyHook) {} + } + + TestBed.configureTestingModule({declarations: [App]}); + + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + fixture.destroy(); + + expect(logs).toEqual(['OnDestroy']); + }); + + it('should invoke ngOnDestroy for useFactory providers provided in Directive', () => { + const logs: string[] = []; + + @Injectable() + class InjectableWithDestroyHook { + ngOnDestroy() { + logs.push('ngOnDestroy'); + } + } + + @Directive({ + selector: '[dir]', + providers: [{ + provide: InjectableWithDestroyHook, + useFactory: () => new InjectableWithDestroyHook(), + }], + }) + class MyDirective { + constructor( + public elRef: ElementRef, public injectableWithDestroyHook: InjectableWithDestroyHook) { + } + } + + @Component({ + selector: 'my-comp', + template: '
', + }) + class MyComponent { + } + + TestBed.configureTestingModule({ + declarations: [MyDirective, MyComponent], + }); + + const fixture = TestBed.createComponent(MyComponent); + fixture.detectChanges(); + fixture.destroy(); + + expect(logs).toEqual(['ngOnDestroy']); + }); + + it('should invoke ngOnDestroy for useFactory providers deeply nested in a template', () => { + const logs: string[] = []; + + @Injectable() + class InjectableWithDestroyHook { + ngOnDestroy() { + logs.push('OnDestroy'); + } + } + + @Component({ + selector: 'deep-cmp', + template: '', + providers: [{ + provide: InjectableWithDestroyHook, + useFactory: () => new InjectableWithDestroyHook(), + }], + }) + class DeepCmp { + constructor(public injectableWithDestroyHook: InjectableWithDestroyHook) {} + } + + @Component({ + template: '
', + }) + class App { + } + + TestBed.configureTestingModule({declarations: [App, DeepCmp]}); + + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + fixture.destroy(); + + expect(logs).toEqual(['OnDestroy']); + }); + + it('should not invoke ngOnDestroy for nullish useFactory providers even the first provided instance in non nullish and implements ngOnDestroy', + () => { + const logs: string[] = []; + + abstract class Service {} + + @Injectable() + class InjectableWithDestroyHook extends Service { + ngOnDestroy() { + logs.push('OnDestroy'); + } + } + + let first = true; + + @Component({ + selector: 'cmp', + template: '', + providers: [{ + provide: Service, + useFactory: () => { + if (first) { + first = false; + return new InjectableWithDestroyHook(); + } else { + return null; + } + }, + }], + }) + class Cmp { + constructor(public service: Service) {} + } + + @Component({ + template: '', + }) + class App { + } + + TestBed.configureTestingModule({declarations: [App, Cmp]}); + + const fixture1 = TestBed.createComponent(App); + fixture1.detectChanges(); + fixture1.destroy(); + + const fixture2 = TestBed.createComponent(App); + fixture2.detectChanges(); + fixture2.destroy(); + + expect(logs).toEqual(['OnDestroy']); + }); + + it('should not invoke ngOnDestroy for useFactory providers that don\'t implement it even the first provided instance does implement ngOnDestroy', + () => { + const logs: string[] = []; + + abstract class Service {} + + @Injectable() + class InjectableWithDestroyHook extends Service { + ngOnDestroy() { + logs.push('OnDestroy'); + } + } + + @Injectable() + class InjectableWithoutDestroyHook extends Service { + } + + let first = true; + + @Component({ + selector: 'cmp', + template: '', + providers: [{ + provide: Service, + useFactory: () => { + if (first) { + first = false; + return new InjectableWithDestroyHook(); + } else { + return new InjectableWithoutDestroyHook(); + } + }, + }], + }) + class Cmp { + constructor(public service: Service) {} + } + + @Component({ + template: '', + }) + class App { + } + + TestBed.configureTestingModule({declarations: [App, Cmp]}); + + const fixture1 = TestBed.createComponent(App); + fixture1.detectChanges(); + fixture1.destroy(); + + const fixture2 = TestBed.createComponent(App); + fixture2.detectChanges(); + fixture2.destroy(); + + expect(logs).toEqual(['OnDestroy']); + }); + + it('should invoke ngOnDestroy for useFactory providers even if initially provided implementation does not implement ngOnDestroy', + () => { + const logs: string[] = []; + + abstract class Service {} + + @Injectable() + class InjectableWithDestroyHook extends Service { + ngOnDestroy() { + logs.push('OnDestroy'); + } + } + @Injectable() + class InjectableWithoutDestroyHook extends Service { + } + + let first = true; + + @Component({ + selector: 'cmp', + template: '', + providers: [{ + provide: Service, + useFactory: () => { + if (first) { + first = false; + return new InjectableWithoutDestroyHook(); + } else { + return new InjectableWithDestroyHook(); + } + }, + }], + }) + class Cmp { + constructor(public service: Service) {} + } + + @Component({ + template: '', + }) + class App { + } + + TestBed.configureTestingModule({declarations: [App, Cmp]}); + + const fixture1 = TestBed.createComponent(App); + fixture1.detectChanges(); + fixture1.destroy(); + + const fixture2 = TestBed.createComponent(App); + fixture2.detectChanges(); + fixture2.destroy(); + + expect(logs).toEqual(['OnDestroy']); + }); + it('should call ngOnDestroy if host component is destroyed', () => { const logs: string[] = []; diff --git a/packages/core/test/bundling/forms/bundle.golden_symbols.json b/packages/core/test/bundling/forms/bundle.golden_symbols.json index 8535a9574416..13c6b771f256 100644 --- a/packages/core/test/bundling/forms/bundle.golden_symbols.json +++ b/packages/core/test/bundling/forms/bundle.golden_symbols.json @@ -1208,6 +1208,9 @@ { "name": "isEmptyInputValue" }, + { + "name": "isFactoryProvider" + }, { "name": "isForwardRef" }, @@ -1388,6 +1391,9 @@ { "name": "ngOnChangesSetInput" }, + { + "name": "ngOnDestroyForFactoryProviders" + }, { "name": "noSideEffects" }, @@ -1733,4 +1739,4 @@ { "name": "ɵɵtext" } -] \ No newline at end of file +] diff --git a/packages/core/test/test_bed_spec.ts b/packages/core/test/test_bed_spec.ts index e1d78c0cba56..816ac04f7bc8 100644 --- a/packages/core/test/test_bed_spec.ts +++ b/packages/core/test/test_bed_spec.ts @@ -290,6 +290,52 @@ describe('TestBed', () => { expect(hello.nativeElement).toHaveText('Hello injected World a second time!'); }); + it('should call ngOnDestroy for a service that was overridden with useFactory', () => { + const logs: string[] = []; + + @Injectable() + class MyService { + public name = 'MyService'; + ngOnDestroy() { + logs.push('MyService.ngOnDestroy'); + } + } + + class FakeService { + name = 'FakeService'; + ngOnDestroy() { + logs.push('FakeService.ngOnDestroy'); + } + } + + @Component({ + selector: 'my-comp', + template: `{{ myService.name }}`, + providers: [MyService], + }) + class MyComponent { + constructor(public myService: MyService) {} + } + + TestBed.configureTestingModule({ + declarations: [MyComponent], + }); + + TestBed.overrideProvider(MyService, { + useFactory: () => new FakeService(), + }); + + const fixture = TestBed.createComponent(MyComponent); + fixture.detectChanges(); + + const service = fixture.componentInstance.myService; + expect(service.name).toBe('FakeService'); + + fixture.destroy(); + + expect(logs).toEqual(['FakeService.ngOnDestroy']); + }); + it('should not call ngOnDestroy for a service that was overridden', () => { SimpleService.ngOnDestroyCalls = 0; diff --git a/packages/forms/src/directives/abstract_form_group_directive.ts b/packages/forms/src/directives/abstract_form_group_directive.ts index d5497344ecbe..6ce3054023e3 100644 --- a/packages/forms/src/directives/abstract_form_group_directive.ts +++ b/packages/forms/src/directives/abstract_form_group_directive.ts @@ -33,8 +33,19 @@ export class AbstractFormGroupDirective extends ControlContainer implements OnIn // TODO(issue/24571): remove '!'. _parent!: ControlContainer; + /** + * @description + * Counts the number of references it has. + * Used for correctly cleanup on ngOnDestroy when useFactory returns the same instance multiple + * times. + * + * @internal + */ + private _refCount: number = 0; + /** @nodoc */ ngOnInit(): void { + this._refCount++; this._checkParentType(); // Register the group with its parent group. this.formDirective!.addFormGroup(this); @@ -42,7 +53,8 @@ export class AbstractFormGroupDirective extends ControlContainer implements OnIn /** @nodoc */ ngOnDestroy(): void { - if (this.formDirective) { + this._refCount--; + if (this.formDirective && this._refCount === 0) { // Remove the group from its parent group. this.formDirective.removeFormGroup(this); } @@ -72,6 +84,22 @@ export class AbstractFormGroupDirective extends ControlContainer implements OnIn return this._parent ? this._parent.formDirective : null; } + /** + * @description + * Returns the current number of references to this directive. + */ + get refCount() { + return this._refCount; + } + + /** + * @description + * Adds a reference into the total reference count. + */ + addRef() { + this._refCount++; + } + /** @internal */ _checkParentType(): void {} } diff --git a/packages/forms/test/reactive_integration_spec.ts b/packages/forms/test/reactive_integration_spec.ts index 389042c4bad2..c87a75de49d2 100644 --- a/packages/forms/test/reactive_integration_spec.ts +++ b/packages/forms/test/reactive_integration_spec.ts @@ -6,11 +6,10 @@ * found in the LICENSE file at https://angular.io/license */ -import {ɵgetDOM as getDOM} from '@angular/common'; -import {Component, Directive, forwardRef, Input, Type} from '@angular/core'; +import {CommonModule, ɵgetDOM as getDOM} from '@angular/common'; +import {Component, Directive, forwardRef, Input, SkipSelf, Type} from '@angular/core'; import {ComponentFixture, fakeAsync, TestBed, tick} from '@angular/core/testing'; -import {expect} from '@angular/core/testing/src/testing_internal'; -import {AbstractControl, AsyncValidator, AsyncValidatorFn, COMPOSITION_BUFFER_MODE, FormArray, FormControl, FormControlDirective, FormControlName, FormGroup, FormGroupDirective, FormsModule, NG_ASYNC_VALIDATORS, NG_VALIDATORS, ReactiveFormsModule, Validator, Validators} from '@angular/forms'; +import {AbstractControl, AsyncValidator, AsyncValidatorFn, COMPOSITION_BUFFER_MODE, ControlContainer, FormArray, FormControl, FormControlDirective, FormControlName, FormGroup, FormGroupDirective, FormsModule, NG_ASYNC_VALIDATORS, NG_VALIDATORS, NgModelGroup, ReactiveFormsModule, Validator, Validators} from '@angular/forms'; import {By} from '@angular/platform-browser/src/dom/debug/by'; import {dispatchEvent, sortedClassList} from '@angular/platform-browser/testing/src/browser_util'; import {merge, NEVER, of, timer} from 'rxjs'; @@ -2678,6 +2677,37 @@ import {MyInput, MyInputForm} from './value_accessor_integration_spec'; expectValidatorsToBeCalled(validatorSpy, asyncValidatorSpy, {ctx: sharedControl, count: 1}); }); }); + + describe('ngModelGroup provider cleanup', () => { + it('should perform the cleanup when there are no references to it', () => { + TestBed.configureTestingModule({ + declarations: [AppComponentNestedForm, NestedForm], + imports: [CommonModule, FormsModule] + }); + const fixture = TestBed.createComponent(AppComponentNestedForm); + fixture.detectChanges(); + + const nestedFormDebugElement = fixture.debugElement.query(By.directive(NestedForm)); + const ngModelGroup = nestedFormDebugElement.injector.get(NgModelGroup); + expect(ngModelGroup.refCount).toBe(3); + + // Destroy First NestedForm component (ngModelGroup should not perform cleanup) + fixture.componentInstance.hasFirstNestedForm = false; + fixture.detectChanges(); + expect(ngModelGroup.refCount).toBe(2); + + // Change fields on Second NestedForm, should work correctly since cleanup should have not + // be performed + fixture.componentInstance.changeFields(); + fixture.detectChanges(); + + // Destroy parent group + fixture.componentInstance.hasParentModelGroup = false; + fixture.detectChanges(); + // Destroying parent group means that all references to ngModelGroup have to be gone. + expect(ngModelGroup.refCount).toBe(0); + }); + }); }); } @@ -3023,4 +3053,52 @@ class MyCustomAsyncValidator implements AsyncValidator { validate(control: AbstractControl) { return Promise.resolve(null); } -} \ No newline at end of file +} + +@Component({ + selector: 'nested-form', + template: ` + +
+ +
+ + `, + viewProviders: [{ + provide: ControlContainer, + useFactory: (ngModelGroup: NgModelGroup) => { + ngModelGroup.addRef(); + return ngModelGroup; + }, + deps: [[new SkipSelf(), NgModelGroup]] + }] +}) +export class NestedForm { + @Input() fields: string[] = []; + + constructor(private controlContainer: ControlContainer) {} +} + +@Component({ + selector: 'my-app', + template: ` +
+
+ + +
+
+
+ Form value: |{{ form.value | json }}| + `, +}) +export class AppComponentNestedForm { + group = {}; + fields = ['a', 'b']; + hasParentModelGroup = true; + hasFirstNestedForm = true; + + changeFields() { + this.fields = ['c', 'd']; + } +}