Skip to content

Commit

Permalink
fix(forms): clean up connection between FormControl/FormGroup and cor…
Browse files Browse the repository at this point in the history 
…responding directive instances (#39235)

Prior to this commit, removing `FormControlDirective` and `FormGroupName` directive instances didn't clear
the callbacks previously registered on FromControl/FormGroup class instances. As a result, these callbacks
were executed even after `FormControlDirective` and `FormGroupName` directive instances were destroyed. That was
also causing memory leaks since these callbacks also retained references to DOM elements.

This commit updates the cleanup logic to take care of properly detaching FormControl/FormGroup/FormArray instances
from the view by removing view-specific callback at destroy time.

Closes #20007, #37431, #39590.

PR Close #39235
  • Loading branch information
@AndrewKushnir @josephperrott
AndrewKushnir authored and josephperrott committed Jan 5, 2021
1 parent 3735633 commit a384961
Show file tree
Hide file tree
Showing 6 changed files with 1,524 additions and 89 deletions.
6 changes: 4 additions & 2 deletions goldens/public-api/forms/forms.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ export declare class FormControl extends AbstractControl {
}): void;
}

export declare class FormControlDirective extends NgControl implements OnChanges {
export declare class FormControlDirective extends NgControl implements OnChanges, OnDestroy {
get control(): FormControl;
form: FormControl;
set isDisabled(isDisabled: boolean);
Expand All @@ -247,6 +247,7 @@ export declare class FormControlDirective extends NgControl implements OnChanges
viewModel: any;
constructor(validators: (Validator | ValidatorFn)[], asyncValidators: (AsyncValidator | AsyncValidatorFn)[], valueAccessors: ControlValueAccessor[], _ngModelWarningConfig: string | null);
ngOnChanges(changes: SimpleChanges): void;
ngOnDestroy(): void;
viewToModelUpdate(newValue: any): void;
}

Expand Down Expand Up @@ -295,7 +296,7 @@ export declare class FormGroup extends AbstractControl {
}): void;
}

export declare class FormGroupDirective extends ControlContainer implements Form, OnChanges {
export declare class FormGroupDirective extends ControlContainer implements Form, OnChanges, OnDestroy {
get control(): FormGroup;
directives: FormControlName[];
form: FormGroup;
Expand All @@ -311,6 +312,7 @@ export declare class FormGroupDirective extends ControlContainer implements Form
getFormArray(dir: FormArrayName): FormArray;
getFormGroup(dir: FormGroupName): FormGroup;
ngOnChanges(changes: SimpleChanges): void;
ngOnDestroy(): void;
onReset(): void;
onSubmit($event: Event): boolean;
removeControl(dir: FormControlName): void;
Expand Down
3 changes: 3 additions & 0 deletions packages/core/test/bundling/forms/bundle.golden_symbols.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -770,6 +770,9 @@
{
"name": "classIndexOf"
},
{
"name": "cleanUpControl"
},
{
"name": "cleanUpValidators"
},
Expand Down
17 changes: 14 additions & 3 deletions packages/forms/src/directives/reactive_directives/form_control_directive.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Directive, EventEmitter, forwardRef, Inject, InjectionToken, Input, OnChanges, Optional, Output, Self, SimpleChanges} from '@angular/core';
import {Directive, EventEmitter, forwardRef, Inject, InjectionToken, Input, OnChanges, OnDestroy, Optional, Output, Self, SimpleChanges} from '@angular/core';

import {FormControl} from '../../model';
import {NG_ASYNC_VALIDATORS, NG_VALIDATORS} from '../../validators';
import {ControlValueAccessor, NG_VALUE_ACCESSOR} from '../control_value_accessor';
import {NgControl} from '../ng_control';
import {ReactiveErrors} from '../reactive_errors';
import {_ngModelWarning, isPropertyUpdated, selectValueAccessor, setUpControl} from '../shared';
import {_ngModelWarning, cleanUpControl, isPropertyUpdated, selectValueAccessor, setUpControl} from '../shared';
import {AsyncValidator, AsyncValidatorFn, Validator, ValidatorFn} from '../validators';


Expand Down Expand Up @@ -51,7 +51,7 @@ export const formControlBinding: any = {
* @publicApi
*/
@Directive({selector: '[formControl]', providers: [formControlBinding], exportAs: 'ngForm'})
export class FormControlDirective extends NgControl implements OnChanges {
export class FormControlDirective extends NgControl implements OnChanges, OnDestroy {
/**
* Internal reference to the view model value.
* @nodoc
Expand Down Expand Up @@ -118,6 +118,10 @@ export class FormControlDirective extends NgControl implements OnChanges {
/** @nodoc */
ngOnChanges(changes: SimpleChanges): void {
if (this._isControlChanged(changes)) {
const previousForm = changes['form'].previousValue;
if (previousForm) {
cleanUpControl(previousForm, this, /* validateControlPresenceOnChange */ false);
}
setUpControl(this.form, this);
if (this.control.disabled && this.valueAccessor!.setDisabledState) {
this.valueAccessor!.setDisabledState!(true);
Expand All @@ -133,6 +137,13 @@ export class FormControlDirective extends NgControl implements OnChanges {
}
}

/** @nodoc */
ngOnDestroy() {
if (this.form) {
cleanUpControl(this.form, this, /* validateControlPresenceOnChange */ false);
}
}

/**
* @description
* Returns an array that represents the path from the top-level form to this control.
Expand Down
79 changes: 64 additions & 15 deletions packages/forms/src/directives/reactive_directives/form_group_directive.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Directive, EventEmitter, forwardRef, Inject, Input, OnChanges, Optional, Output, Self, SimpleChanges} from '@angular/core';
import {Directive, EventEmitter, forwardRef, Inject, Input, OnChanges, OnDestroy, Optional, Output, Self, SimpleChanges} from '@angular/core';

import {FormArray, FormControl, FormGroup} from '../../model';
import {NG_ASYNC_VALIDATORS, NG_VALIDATORS} from '../../validators';
import {ControlContainer} from '../control_container';
import {Form} from '../form_interface';
import {ReactiveErrors} from '../reactive_errors';
import {cleanUpControl, cleanUpValidators, removeListItem, setUpControl, setUpFormContainer, setUpValidators, syncPendingControls} from '../shared';
import {cleanUpControl, cleanUpFormContainer, cleanUpValidators, removeListItem, setUpControl, setUpFormContainer, setUpValidators, syncPendingControls} from '../shared';
import {AsyncValidator, AsyncValidatorFn, Validator, ValidatorFn} from '../validators';

import {FormControlName} from './form_control_name';
Expand Down Expand Up @@ -53,7 +53,7 @@ export const formDirectiveProvider: any = {
host: {'(submit)': 'onSubmit($event)', '(reset)': 'onReset()'},
exportAs: 'ngForm'
})
export class FormGroupDirective extends ControlContainer implements Form, OnChanges {
export class FormGroupDirective extends ControlContainer implements Form, OnChanges, OnDestroy {
/**
* @description
* Reports whether the form submission has been triggered.
Expand All @@ -66,6 +66,12 @@ export class FormGroupDirective extends ControlContainer implements Form, OnChan
*/
private _oldForm: FormGroup|undefined;

/**
* Callback that should be invoked when controls in FormGroup or FormArray collection change
* (added or removed). This callback triggers corresponding DOM updates.
*/
private readonly _onCollectionChange = () => this._updateDomValue();

/**
* @description
* Tracks the list of added `FormControlName` instances
Expand Down Expand Up @@ -104,6 +110,23 @@ export class FormGroupDirective extends ControlContainer implements Form, OnChan
}
}

/** @nodoc */
ngOnDestroy() {
if (this.form) {
cleanUpValidators(this.form, this, /* handleOnValidatorChange */ false);

// Currently the `onCollectionChange` callback is rewritten each time the
// `_registerOnCollectionChange` function is invoked. The implication is that cleanup should
// happen *only* when the `onCollectionChange` callback was set by this directive instance.
// Otherwise it might cause overriding a callback of some other directive instances. We should
// consider updating this logic later to make it similar to how `onChange` callbacks are
// handled, see https://github.com/angular/angular/issues/39732 for additional info.
if (this.form._onCollectionChange === this._onCollectionChange) {
this.form._registerOnCollectionChange(() => {});
}
}
}

/**
* @description
* Returns this directive's instance.
Expand Down Expand Up @@ -161,6 +184,7 @@ export class FormGroupDirective extends ControlContainer implements Form, OnChan
* @param dir The `FormControlName` directive instance.
*/
removeControl(dir: FormControlName): void {
cleanUpControl(dir.control || null, dir, /* validateControlPresenceOnChange */ false);
removeListItem(this.directives, dir);
}

Expand All @@ -170,17 +194,18 @@ export class FormGroupDirective extends ControlContainer implements Form, OnChan
* @param dir The `FormGroupName` directive instance.
*/
addFormGroup(dir: FormGroupName): void {
const ctrl: any = this.form.get(dir.path);
setUpFormContainer(ctrl, dir);
ctrl.updateValueAndValidity({emitEvent: false});
this._setUpFormContainer(dir);
}

/**
* No-op method to remove the form group.
* Performs the necessary cleanup when a `FormGroupName` directive instance is removed from the
* view.
*
* @param dir The `FormGroupName` directive instance.
*/
removeFormGroup(dir: FormGroupName): void {}
removeFormGroup(dir: FormGroupName): void {
this._cleanUpFormContainer(dir);
}

/**
* @description
Expand All @@ -193,22 +218,23 @@ export class FormGroupDirective extends ControlContainer implements Form, OnChan
}

/**
* Adds a new `FormArrayName` directive instance to the form.
* Performs the necessary setup when a `FormArrayName` directive instance is added to the view.
*
* @param dir The `FormArrayName` directive instance.
*/
addFormArray(dir: FormArrayName): void {
const ctrl: any = this.form.get(dir.path);
setUpFormContainer(ctrl, dir);
ctrl.updateValueAndValidity({emitEvent: false});
this._setUpFormContainer(dir);
}

/**
* No-op method to remove the form array.
* Performs the necessary cleanup when a `FormArrayName` directive instance is removed from the
* view.
*
* @param dir The `FormArrayName` directive instance.
*/
removeFormArray(dir: FormArrayName): void {}
removeFormArray(dir: FormArrayName): void {
this._cleanUpFormContainer(dir);
}

/**
* @description
Expand Down Expand Up @@ -281,8 +307,31 @@ export class FormGroupDirective extends ControlContainer implements Form, OnChan
this.form._updateTreeValidity({emitEvent: false});
}

private _setUpFormContainer(dir: FormArrayName|FormGroupName): void {
const ctrl: any = this.form.get(dir.path);
setUpFormContainer(ctrl, dir);
// NOTE: this operation looks unnecessary in case no new validators were added in
// `setUpFormContainer` call. Consider updating this code to match the logic in
// `_cleanUpFormContainer` function.
ctrl.updateValueAndValidity({emitEvent: false});
}

private _cleanUpFormContainer(dir: FormArrayName|FormGroupName): void {
if (this.form) {
const ctrl: any = this.form.get(dir.path);
if (ctrl) {
const isControlUpdated = cleanUpFormContainer(ctrl, dir);
if (isControlUpdated) {
// Run validity check only in case a control was updated (i.e. view validators were
// removed) as removing view validators might cause validity to change.
ctrl.updateValueAndValidity({emitEvent: false});
}
}
}
}

private _updateRegistrations() {
this.form._registerOnCollectionChange(() => this._updateDomValue());
this.form._registerOnCollectionChange(this._onCollectionChange);
if (this._oldForm) {
this._oldForm._registerOnCollectionChange(() => {});
}
Expand Down
63 changes: 57 additions & 6 deletions packages/forms/src/directives/shared.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@ export function controlPath(name: string|null, parent: ControlContainer): string
return [...parent.path!, name!];
}

/**
* Links a Form control and a Form directive by setting up callbacks (such as `onChange`) on both
* instances. This function is typically invoked when form directive is being initialized.
*
* @param control Form control instance that should be linked.
* @param dir Directive that should be linked with a given control.
*/
export function setUpControl(control: FormControl, dir: NgControl): void {
if (typeof ngDevMode === 'undefined' || ngDevMode) {
if (!control) _throwError(dir, 'Cannot find control with');
Expand All @@ -48,9 +55,22 @@ export function setUpControl(control: FormControl, dir: NgControl): void {
setUpDisabledChangeHandler(control, dir);
}

export function cleanUpControl(control: FormControl|null, dir: NgControl) {
/**
* Reverts configuration performed by the `setUpControl` control function.
* Effectively disconnects form control with a given form directive.
* This function is typically invoked when corresponding form directive is being destroyed.
*
* @param control Form control which should be cleaned up.
* @param dir Directive that should be disconnected from a given control.
* @param validateControlPresenceOnChange Flag that indicates whether onChange handler should
* contain asserts to verify that it's not called once directive is destroyed. We need this flag
* to avoid potentially breaking changes caused by better control cleanup introduced in #39235.
*/
export function cleanUpControl(
control: FormControl|null, dir: NgControl,
validateControlPresenceOnChange: boolean = true): void {
const noop = () => {
if (typeof ngDevMode === 'undefined' || ngDevMode) {
if (validateControlPresenceOnChange && (typeof ngDevMode === 'undefined' || ngDevMode)) {
_noControlError(dir);
}
};
Expand Down Expand Up @@ -146,25 +166,35 @@ export function setUpValidators(
* @param dir Directive instance that contains validators to be removed.
* @param handleOnValidatorChange Flag that determines whether directive validators should also be
* cleaned up to stop handling validator input change (if previously configured to do so).
* @returns true if a control was updated as a result of this action.
*/
export function cleanUpValidators(
control: AbstractControl|null, dir: AbstractControlDirective,
handleOnValidatorChange: boolean): void {
handleOnValidatorChange: boolean): boolean {
let isControlUpdated = false;
if (control !== null) {
if (dir.validator !== null) {
const validators = getControlValidators(control);
if (Array.isArray(validators) && validators.length > 0) {
// Filter out directive validator function.
control.setValidators(validators.filter(validator => validator !== dir.validator));
const updatedValidators = validators.filter(validator => validator !== dir.validator);
if (updatedValidators.length !== validators.length) {
isControlUpdated = true;
control.setValidators(updatedValidators);
}
}
}

if (dir.asyncValidator !== null) {
const asyncValidators = getControlAsyncValidators(control);
if (Array.isArray(asyncValidators) && asyncValidators.length > 0) {
// Filter out directive async validator function.
control.setAsyncValidators(
asyncValidators.filter(asyncValidator => asyncValidator !== dir.asyncValidator));
const updatedAsyncValidators =
asyncValidators.filter(asyncValidator => asyncValidator !== dir.asyncValidator);
if (updatedAsyncValidators.length !== asyncValidators.length) {
isControlUpdated = true;
control.setAsyncValidators(updatedAsyncValidators);
}
}
}
}
Expand All @@ -175,6 +205,8 @@ export function cleanUpValidators(
registerOnValidatorChange<ValidatorFn>(dir._rawValidators, noop);
registerOnValidatorChange<AsyncValidatorFn>(dir._rawAsyncValidators, noop);
}

return isControlUpdated;
}

function setUpViewChangePipeline(control: FormControl, dir: NgControl): void {
Expand Down Expand Up @@ -220,13 +252,32 @@ function setUpModelChangePipeline(control: FormControl, dir: NgControl): void {
});
}

/**
* Links a FormGroup or FormArray instance and corresponding Form directive by setting up validators
* present in the view.
*
* @param control FormGroup or FormArray instance that should be linked.
* @param dir Directive that provides view validators.
*/
export function setUpFormContainer(
control: FormGroup|FormArray, dir: AbstractFormGroupDirective|FormArrayName) {
if (control == null && (typeof ngDevMode === 'undefined' || ngDevMode))
_throwError(dir, 'Cannot find control with');
setUpValidators(control, dir, /* handleOnValidatorChange */ false);
}

/**
* Reverts the setup performed by the `setUpFormContainer` function.
*
* @param control FormGroup or FormArray instance that should be cleaned up.
* @param dir Directive that provided view validators.
* @returns true if a control was updated as a result of this action.
*/
export function cleanUpFormContainer(
control: FormGroup|FormArray, dir: AbstractFormGroupDirective|FormArrayName): boolean {
return cleanUpValidators(control, dir, /* handleOnValidatorChange */ false);
}

function _noControlError(dir: NgControl) {
return _throwError(dir, 'There is no FormControl instance attached to form control element with');
}
Expand Down

0 comments on commit a384961

Please sign in to comment.