iSharkFly-Docs
/
angular-docs-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(forms): more precise control cleanup (#39623) 

Currently when an instance of the `FormControlName` directive is destroyed, the Forms package invokes
the `cleanUpControl` to clear all directive-specific logic (such as validators, onChange handlers,
etc) from a bound control. The logic of the `cleanUpControl` function should revert all setup
performed by the `setUpControl` function. However the `cleanUpControl` is too aggressive and removes
all callbacks related to the onChange and disabled state handling. This is causing problems when
a form control is bound to multiple FormControlName` directives, causing other instances of that
directive to stop working correctly when the first one is destroyed.

This commit updates the cleanup logic to only remove callbacks added while setting up a control
for a given directive instance.

The fix is needed to allow adding `cleanUpControl` function to other places where cleanup is needed
(missing this function calls in some other places causes memory leak issues).

PR Close #39623
This commit is contained in:
Andrew Kushnir 2020-11-09 22:58:19 -08:00 committed by atscott
parent 44763245e1
commit 1bc53eb303
8 changed files with 209 additions and 77 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

126
goldens/circular-deps/packages.json
View File

@ -1002,6 +1002,27 @@
"packages/core/testing/src/r3_test_bed.ts",
"packages/core/testing/src/test_bed.ts"
],
[
"packages/forms/src/directives/abstract_control_directive.ts",
"packages/forms/src/model.ts",
"packages/forms/src/directives/shared.ts"
],
[
"packages/forms/src/directives/abstract_control_directive.ts",
"packages/forms/src/model.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/control_container.ts"
],
[
"packages/forms/src/directives/abstract_control_directive.ts",
"packages/forms/src/model.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/control_container.ts",
"packages/forms/src/directives/form_interface.ts",
"packages/forms/src/directives/ng_control.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/control_container.ts",
@ -1009,7 +1030,10 @@
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/form_interface.ts"
"packages/forms/src/directives/control_container.ts",
"packages/forms/src/directives/form_interface.ts",
"packages/forms/src/model.ts",
"packages/forms/src/directives/shared.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
@ -1017,59 +1041,13 @@
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/model.ts",
"packages/forms/src/directives/shared.ts"
],
[
"packages/forms/src/directives/control_container.ts",
"packages/forms/src/directives/form_interface.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/ng_control.ts",
"packages/forms/src/directives/control_container.ts",
"packages/forms/src/directives/form_interface.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/directives/control_container.ts",
"packages/forms/src/directives/form_interface.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/directives/reactive_directives/form_group_directive.ts",
"packages/forms/src/directives/control_container.ts",
"packages/forms/src/directives/form_interface.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/directives/reactive_directives/form_group_directive.ts",
"packages/forms/src/directives/form_interface.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/directives/reactive_directives/form_group_directive.ts",
"packages/forms/src/directives/reactive_directives/form_control_name.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/directives/reactive_directives/form_group_directive.ts",
"packages/forms/src/directives/reactive_directives/form_control_name.ts",
"packages/forms/src/directives/control_container.ts",
"packages/forms/src/directives/form_interface.ts"
"packages/forms/src/directives/form_interface.ts",
"packages/forms/src/directives/ng_control.ts"
],
[
"packages/forms/src/directives/ng_form.ts",
@ -1087,6 +1065,14 @@
"packages/forms/src/directives/reactive_directives/form_group_directive.ts",
"packages/forms/src/directives/reactive_directives/form_control_name.ts"
],
[
"packages/forms/src/directives/reactive_directives/form_control_directive.ts",
"packages/forms/src/model.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/directives/reactive_directives/form_group_directive.ts",
"packages/forms/src/directives/reactive_directives/form_control_name.ts"
],
[
"packages/forms/src/directives/reactive_directives/form_control_name.ts",
"packages/forms/src/directives/reactive_directives/form_group_directive.ts"
@ -1102,6 +1088,13 @@
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/directives/reactive_directives/form_group_directive.ts"
],
[
"packages/forms/src/directives/reactive_directives/form_control_name.ts",
"packages/forms/src/model.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/directives/reactive_directives/form_group_directive.ts"
],
[
"packages/forms/src/directives/reactive_directives/form_group_directive.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts"
@ -1111,23 +1104,40 @@
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts"
],
[
"packages/forms/src/directives/reactive_directives/form_group_directive.ts",
"packages/forms/src/model.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts"
],
[
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/directives/shared.ts"
],
[
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/model.ts",
"packages/forms/src/directives/shared.ts"
],
[
"packages/forms/src/directives/shared.ts",
"packages/forms/src/model.ts"
],
[
"packages/forms/src/directives/shared.ts",
"packages/forms/src/validators.ts",
"packages/forms/src/directives/validators.ts",
"packages/forms/src/model.ts"
],
[
"packages/forms/src/directives/shared.ts",
"packages/forms/src/validators.ts",
"packages/forms/src/model.ts"
],
[
"packages/forms/src/directives/validators.ts",
"packages/forms/src/validators.ts"
],
[
"packages/forms/src/directives/validators.ts",
"packages/forms/src/validators.ts",
"packages/forms/src/model.ts"
],
[
"packages/language-service/src/completions.ts",
"packages/language-service/src/template.ts",

4
packages/core/test/bundling/forms/bundle.golden_symbols.json
View File

@ -1455,10 +1455,10 @@
"name": "remove"
},
{
"name": "removeDir"
"name": "removeFromArray"
},
{
"name": "removeFromArray"
"name": "removeListItem"
},
{
"name": "renderComponent"

24
packages/forms/src/directives/abstract_control_directive.ts
View File

@ -230,6 +230,30 @@ export abstract class AbstractControlDirective {
return this._composedAsyncValidatorFn || null;
}
/*
* The set of callbacks to be invoked when directive instance is being destroyed.
*/
private _onDestroyCallbacks: (() => void)[] = [];
/**
* Internal function to register callbacks that should be invoked
* when directive instance is being destroyed.
* @internal
*/
_registerOnDestroy(fn: () => void): void {
this._onDestroyCallbacks.push(fn);
}
/**
* Internal function to invoke all registered "on destroy" callbacks.
* Note: calling this function also clears the list of callbacks.
* @internal
*/
_invokeOnDestroyCallbacks(): void {
this._onDestroyCallbacks.forEach(fn => fn());
this._onDestroyCallbacks = [];
}
/**
* @description
* Resets the control with the provided value if the control is present.

4
packages/forms/src/directives/ng_form.ts
View File

@ -16,7 +16,7 @@ import {Form} from './form_interface';
import {NgControl} from './ng_control';
import {NgModel} from './ng_model';
import {NgModelGroup} from './ng_model_group';
import {removeDir, setUpControl, setUpFormContainer, syncPendingControls} from './shared';
import {removeListItem, setUpControl, setUpFormContainer, syncPendingControls} from './shared';
import {AsyncValidator, AsyncValidatorFn, Validator, ValidatorFn} from './validators';
export const formDirectiveProvider: any = {
@ -217,7 +217,7 @@ export class NgForm extends ControlContainer implements Form, AfterViewInit {
if (container) {
container.removeControl(dir.name);
}
removeDir<NgModel>(this._directives, dir);
removeListItem(this._directives, dir);
});
}

4
packages/forms/src/directives/reactive_directives/form_group_directive.ts
View File

@ -13,7 +13,7 @@ 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, removeDir, setUpControl, setUpFormContainer, setUpValidators, syncPendingControls} from '../shared';
import {cleanUpControl, cleanUpValidators, removeListItem, setUpControl, setUpFormContainer, setUpValidators, syncPendingControls} from '../shared';
import {AsyncValidator, AsyncValidatorFn, Validator, ValidatorFn} from '../validators';
import {FormControlName} from './form_control_name';
@ -161,7 +161,7 @@ export class FormGroupDirective extends ControlContainer implements Form, OnChan
* @param dir The `FormControlName` directive instance.
*/
removeControl(dir: FormControlName): void {
removeDir<FormControlName>(this.directives, dir);
removeListItem(this.directives, dir);
}
/**

44
packages/forms/src/directives/shared.ts
View File

@ -47,11 +47,7 @@ export function setUpControl(control: FormControl, dir: NgControl): void {
setUpBlurPipeline(control, dir);
if (dir.valueAccessor!.setDisabledState) {
control.registerOnDisabledChange((isDisabled: boolean) => {
dir.valueAccessor!.setDisabledState!(isDisabled);
});
}
setUpDisabledChangeHandler(control, dir);
}
export function cleanUpControl(control: FormControl|null, dir: NgControl) {
@ -66,7 +62,10 @@ export function cleanUpControl(control: FormControl|null, dir: NgControl) {
cleanUpValidators(control, dir, /* handleOnValidatorChange */ true);
if (control) control._clearChangeFns();
if (control) {
dir._invokeOnDestroyCallbacks();
control._registerOnCollectionChange(() => {});
}
}
function registerOnValidatorChange<V>(validators: (V|Validator)[], onChange: () => void): void {
@ -76,6 +75,28 @@ function registerOnValidatorChange<V>(validators: (V|Validator)[], onChange: ()
});
}
/**
* Sets up disabled change handler function on a given form control if ControlValueAccessor
* associated with a given directive instance supports the `setDisabledState` call.
*
* @param control Form control where disabled change handler should be setup.
* @param dir Corresponding directive instance associated with this control.
*/
export function setUpDisabledChangeHandler(control: FormControl, dir: NgControl): void {
if (dir.valueAccessor!.setDisabledState) {
const onDisabledChange = (isDisabled: boolean) => {
dir.valueAccessor!.setDisabledState!(isDisabled);
};
control.registerOnDisabledChange(onDisabledChange);
// Register a callback function to cleanup disabled change handler
// from a control instance when a directive is destroyed.
dir._registerOnDestroy(() => {
control._unregisterOnDisabledChange(onDisabledChange);
});
}
}
/**
* Sets up sync and async directive validators on provided form control.
* This function merges validators from the directive into the validators of the control.
@ -185,12 +206,19 @@ function updateControl(control: FormControl, dir: NgControl): void {
}
function setUpModelChangePipeline(control: FormControl, dir: NgControl): void {
control.registerOnChange((newValue: any, emitModelEvent: boolean) => {
const onChange = (newValue: any, emitModelEvent: boolean) => {
// control -> view
dir.valueAccessor!.writeValue(newValue);
// control -> ngModel
if (emitModelEvent) dir.viewToModelUpdate(newValue);
};
control.registerOnChange(onChange);
// Register a callback function to cleanup onChange handler
// from a control instance when a directive is destroyed.
dir._registerOnDestroy(() => {
control._unregisterOnChange(onChange);
});
}
@ -287,7 +315,7 @@ export function selectValueAccessor(
return null;
}
export function removeDir<T>(list: T[], el: T): void {
export function removeListItem<T>(list: T[], el: T): void {
const index = list.indexOf(el);
if (index > -1) list.splice(index, 1);
}

16
packages/forms/src/model.ts
View File

@ -9,6 +9,7 @@
import {EventEmitter} from '@angular/core';
import {Observable} from 'rxjs';
import {removeListItem} from './directives/shared';
import {AsyncValidatorFn, ValidationErrors, ValidatorFn} from './directives/validators';
import {composeAsyncValidators, composeValidators, toObservable} from './validators';
@ -1269,12 +1270,11 @@ export class FormControl extends AbstractControl {
}
/**
* Internal function to unregister a change events listener.
* @internal
*/
_clearChangeFns(): void {
this._onChange = [];
this._onDisabledChange = [];
this._onCollectionChange = () => {};
_unregisterOnChange(fn: Function): void {
removeListItem(this._onChange, fn);
}
/**
@ -1286,6 +1286,14 @@ export class FormControl extends AbstractControl {
this._onDisabledChange.push(fn);
}
/**
* Internal function to unregister a disabled event listener.
* @internal
*/
_unregisterOnDisabledChange(fn: (isDisabled: boolean) => void): void {
removeListItem(this._onDisabledChange, fn);
}
/**
* @internal
*/

64
packages/forms/test/reactive_integration_spec.ts
View File

@ -2594,7 +2594,7 @@ import {MyInput, MyInputForm} from './value_accessor_integration_spec';
expectValidatorsToBeCalled(newValidatorSpy, newAsyncValidatorSpy, {ctx: control, count: 1});
});
it('should cleanup validators on a control used for multiple `formControlName` directive',
it('should cleanup validators on a control used for multiple `formControlName` directives',
() => {
const fixture =
initTest(NgForFormControlWithValidators, MyCustomValidator, MyCustomAsyncValidator);
@ -2643,6 +2643,40 @@ import {MyInput, MyInputForm} from './value_accessor_integration_spec';
// of controls (one for each element in the `logins` array).
expectValidatorsToBeCalled(validatorSpy, asyncValidatorSpy, {ctx: newControl, count: 6});
});
it('should cleanup directive-specific callbacks only', () => {
const fixture = initTest(MultipleFormControls, MyCustomValidator, MyCustomAsyncValidator);
fixture.detectChanges();
const sharedControl = fixture.componentInstance.control;
const validatorSpy = validatorSpyOn(MyCustomValidator);
const asyncValidatorSpy = validatorSpyOn(MyCustomAsyncValidator);
sharedControl.setValue('b');
fixture.detectChanges();
// Check that validators were called for each `formControlName` directive instance
// (2 times total).
expectValidatorsToBeCalled(validatorSpy, asyncValidatorSpy, {ctx: sharedControl, count: 2});
// Replace formA with a new instance. This will trigger destroy operation for the
// `formControlName` directive that is bound to the `control` FormControl instance.
const newFormA = new FormGroup({login: new FormControl('new-a')});
fixture.componentInstance.formA = newFormA;
fixture.detectChanges();
validatorSpy.calls.reset();
asyncValidatorSpy.calls.reset();
// Update control with a new value.
sharedControl.setValue('d');
fixture.detectChanges();
// We should still see an update to the second <input>.
expect(fixture.nativeElement.querySelector('#login').value).toBe('d');
expectValidatorsToBeCalled(validatorSpy, asyncValidatorSpy, {ctx: sharedControl, count: 1});
});
});
});
}
@ -2920,6 +2954,34 @@ class FormControlWithValidators {
form = new FormGroup({login: new FormControl('INITIAL')});
}
@Component({
selector: 'ngfor-form-controls-with-validators',
template: `
<div [formGroup]="formA">
<input
type="radio"
formControlName="login"
my-custom-validator
my-custom-async-validator
/>
</div>
<div [formGroup]="formB">
<input
id="login"
type="text"
formControlName="login"
my-custom-validator
my-custom-async-validator
/>
</div>
`
})
class MultipleFormControls {
control = new FormControl('a');
formA = new FormGroup({login: this.control});
formB = new FormGroup({login: this.control});
}
@Component({
selector: 'ngfor-form-controls-with-validators',
template: `