From 43349dd37331f9a48bf94c2e527b574cf8a9cea9 Mon Sep 17 00:00:00 2001 From: Kara Date: Wed, 27 Jul 2016 10:59:40 -0700 Subject: [PATCH] fix(forms): improve ngModel error messages (#10314) --- .../forms/src/directives/error_examples.ts | 64 ++++++++++ .../@angular/forms/src/directives/ng_model.ts | 31 +++-- .../forms/src/directives/ng_model_group.ts | 10 ++ .../reactive_directives/form_array_name.ts | 24 +--- .../reactive_directives/form_control_name.ts | 25 ++-- .../form_group_directive.ts | 5 +- .../reactive_directives/form_group_name.ts | 21 +--- .../forms/src/directives/reactive_errors.ts | 63 ++++++++++ .../src/directives/template_driven_errors.ts | 61 +++++++++ .../forms/test/reactive_integration_spec.ts | 118 ++++++++++++++---- 10 files changed, 331 insertions(+), 91 deletions(-) create mode 100644 modules/@angular/forms/src/directives/error_examples.ts create mode 100644 modules/@angular/forms/src/directives/reactive_errors.ts create mode 100644 modules/@angular/forms/src/directives/template_driven_errors.ts diff --git a/modules/@angular/forms/src/directives/error_examples.ts b/modules/@angular/forms/src/directives/error_examples.ts new file mode 100644 index 0000000000..a59059ab69 --- /dev/null +++ b/modules/@angular/forms/src/directives/error_examples.ts @@ -0,0 +1,64 @@ +/** + * @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 + */ + +export const FormErrorExamples = { + formControlName: ` +
+ +
+ + In your class: + + this.myGroup = new FormGroup({ + firstName: new FormControl() + });`, + + formGroupName: ` +
+
+ +
+
+ + In your class: + + this.myGroup = new FormGroup({ + person: new FormGroup({ firstName: new FormControl() }) + });`, + + formArrayName: ` +
+
+
+ +
+
+
+ + In your class: + + this.cityArray = new FormArray([new FormControl('SF')]); + this.myGroup = new FormGroup({ + cities: this.cityArray + });`, + + ngModelGroup: ` +
+
+ +
+
`, + + ngModelWithFormGroup: ` +
+ + +
+ ` +}; +  \ No newline at end of file diff --git a/modules/@angular/forms/src/directives/ng_model.ts b/modules/@angular/forms/src/directives/ng_model.ts index 7e07820e1d..a80a840e9a 100644 --- a/modules/@angular/forms/src/directives/ng_model.ts +++ b/modules/@angular/forms/src/directives/ng_model.ts @@ -13,10 +13,14 @@ import {BaseException} from '../facade/exceptions'; import {FormControl} from '../model'; import {NG_ASYNC_VALIDATORS, NG_VALIDATORS} from '../validators'; +import {AbstractFormGroupDirective} from './abstract_form_group_directive'; import {ControlContainer} from './control_container'; import {ControlValueAccessor, NG_VALUE_ACCESSOR} from './control_value_accessor'; import {NgControl} from './ng_control'; +import {NgForm} from './ng_form'; +import {NgModelGroup} from './ng_model_group'; import {composeAsyncValidators, composeValidators, controlPath, isPropertyUpdated, selectValueAccessor, setUpControl} from './shared'; +import {TemplateDrivenErrors} from './template_driven_errors'; import {AsyncValidatorFn, ValidatorFn} from './validators'; export const formControlBinding: any = @@ -75,7 +79,7 @@ export class NgModel extends NgControl implements OnChanges, } ngOnChanges(changes: SimpleChanges) { - this._checkName(); + this._checkForErrors(); if (!this._registered) this._setUpControl(); if (isPropertyUpdated(changes, this.viewModel)) { @@ -120,17 +124,28 @@ export class NgModel extends NgControl implements OnChanges, this._control.updateValueAndValidity({emitEvent: false}); } + private _checkForErrors(): void { + if (!this._isStandalone()) { + this._checkParentType(); + } + this._checkName(); + } + + private _checkParentType(): void { + if (!(this._parent instanceof NgModelGroup) && + this._parent instanceof AbstractFormGroupDirective) { + TemplateDrivenErrors.formGroupNameException(); + } else if ( + !(this._parent instanceof NgModelGroup) && !(this._parent instanceof NgForm)) { + TemplateDrivenErrors.modelParentException(); + } + } + private _checkName(): void { if (this.options && this.options.name) this.name = this.options.name; if (!this._isStandalone() && !this.name) { - throw new BaseException( - `If ngModel is used within a form tag, either the name attribute must be set - or the form control must be defined as 'standalone' in ngModelOptions. - - Example 1: - Example 2: - `); + TemplateDrivenErrors.missingNameException(); } } diff --git a/modules/@angular/forms/src/directives/ng_model_group.ts b/modules/@angular/forms/src/directives/ng_model_group.ts index b2e3484d53..282fbd70ae 100644 --- a/modules/@angular/forms/src/directives/ng_model_group.ts +++ b/modules/@angular/forms/src/directives/ng_model_group.ts @@ -8,10 +8,13 @@ import {Directive, Host, Inject, Input, OnDestroy, OnInit, Optional, Self, SkipSelf, forwardRef} from '@angular/core'; +import {BaseException} from '../facade/exceptions'; import {NG_ASYNC_VALIDATORS, NG_VALIDATORS} from '../validators'; import {AbstractFormGroupDirective} from './abstract_form_group_directive'; import {ControlContainer} from './control_container'; +import {NgForm} from './ng_form'; +import {TemplateDrivenErrors} from './template_driven_errors'; export const modelGroupProvider: any = /*@ts2dart_const*/ /* @ts2dart_Provider */ { @@ -69,4 +72,11 @@ export class NgModelGroup extends AbstractFormGroupDirective implements OnInit, this._validators = validators; this._asyncValidators = asyncValidators; } + + /** @internal */ + _checkParentType(): void { + if (!(this._parent instanceof NgModelGroup) && !(this._parent instanceof NgForm)) { + TemplateDrivenErrors.modelGroupParentException(); + } + } } diff --git a/modules/@angular/forms/src/directives/reactive_directives/form_array_name.ts b/modules/@angular/forms/src/directives/reactive_directives/form_array_name.ts index 064667c163..9287c64ae5 100644 --- a/modules/@angular/forms/src/directives/reactive_directives/form_array_name.ts +++ b/modules/@angular/forms/src/directives/reactive_directives/form_array_name.ts @@ -12,6 +12,7 @@ import {BaseException} from '../../facade/exceptions'; import {FormArray} from '../../model'; import {NG_ASYNC_VALIDATORS, NG_VALIDATORS} from '../../validators'; import {ControlContainer} from '../control_container'; +import {ReactiveErrors} from '../reactive_errors'; import {composeAsyncValidators, composeValidators, controlPath} from '../shared'; import {AsyncValidatorFn, ValidatorFn} from '../validators'; @@ -101,28 +102,7 @@ export class FormArrayName extends ControlContainer implements OnInit, OnDestroy private _checkParentType(): void { if (!(this._parent instanceof FormGroupName) && !(this._parent instanceof FormGroupDirective)) { - this._throwParentException(); + ReactiveErrors.arrayParentException(); } } - - private _throwParentException(): void { - throw new BaseException(`formArrayName must be used with a parent formGroup directive. - You'll want to add a formGroup directive and pass it an existing FormGroup instance - (you can create one in your class). - - Example: -
-
-
- -
-
-
- - In your class: - this.cityArray = new FormArray([new FormControl('SF')]); - this.myGroup = new FormGroup({ - cities: this.cityArray - });`); - } } diff --git a/modules/@angular/forms/src/directives/reactive_directives/form_control_name.ts b/modules/@angular/forms/src/directives/reactive_directives/form_control_name.ts index a8b3ced01a..3a32bd02a0 100644 --- a/modules/@angular/forms/src/directives/reactive_directives/form_control_name.ts +++ b/modules/@angular/forms/src/directives/reactive_directives/form_control_name.ts @@ -12,9 +12,11 @@ import {EventEmitter, ObservableWrapper} from '../../facade/async'; import {BaseException} from '../../facade/exceptions'; import {FormControl} from '../../model'; import {NG_ASYNC_VALIDATORS, NG_VALIDATORS} from '../../validators'; +import {AbstractFormGroupDirective} from '../abstract_form_group_directive'; import {ControlContainer} from '../control_container'; import {ControlValueAccessor, NG_VALUE_ACCESSOR} from '../control_value_accessor'; import {NgControl} from '../ng_control'; +import {ReactiveErrors} from '../reactive_errors'; import {composeAsyncValidators, composeValidators, controlPath, isPropertyUpdated, selectValueAccessor} from '../shared'; import {AsyncValidatorFn, ValidatorFn} from '../validators'; @@ -153,26 +155,13 @@ export class FormControlName extends NgControl implements OnChanges, OnDestroy { private _checkParentType(): void { if (!(this._parent instanceof FormGroupName) && + this._parent instanceof AbstractFormGroupDirective) { + ReactiveErrors.ngModelGroupException(); + } else if ( + !(this._parent instanceof FormGroupName) && !(this._parent instanceof FormGroupDirective) && !(this._parent instanceof FormArrayName)) { - this._throwParentException(); + ReactiveErrors.controlParentException(); } } - - private _throwParentException(): void { - throw new BaseException( - `formControlName must be used with a parent formGroup directive. - You'll want to add a formGroup directive and pass it an existing FormGroup instance - (you can create one in your class). - - Example: -
- -
- - In your class: - this.myGroup = new FormGroup({ - firstName: new FormControl() - });`); - } } diff --git a/modules/@angular/forms/src/directives/reactive_directives/form_group_directive.ts b/modules/@angular/forms/src/directives/reactive_directives/form_group_directive.ts index 13cca21f37..9d572c39cf 100644 --- a/modules/@angular/forms/src/directives/reactive_directives/form_group_directive.ts +++ b/modules/@angular/forms/src/directives/reactive_directives/form_group_directive.ts @@ -17,6 +17,7 @@ import {NG_ASYNC_VALIDATORS, NG_VALIDATORS, Validators} from '../../validators'; import {ControlContainer} from '../control_container'; import {Form} from '../form_interface'; import {NgControl} from '../ng_control'; +import {ReactiveErrors} from '../reactive_errors'; import {composeAsyncValidators, composeValidators, setUpControl, setUpFormContainer} from '../shared'; import {FormArrayName} from './form_array_name'; @@ -199,9 +200,7 @@ export class FormGroupDirective extends ControlContainer implements Form, private _checkFormPresent() { if (isBlank(this.form)) { - throw new BaseException(`formGroup expects a FormGroup instance. Please pass one in. - Example:
- `); + ReactiveErrors.missingFormException(); } } } diff --git a/modules/@angular/forms/src/directives/reactive_directives/form_group_name.ts b/modules/@angular/forms/src/directives/reactive_directives/form_group_name.ts index bfe00f0975..79521e4670 100644 --- a/modules/@angular/forms/src/directives/reactive_directives/form_group_name.ts +++ b/modules/@angular/forms/src/directives/reactive_directives/form_group_name.ts @@ -12,6 +12,7 @@ import {BaseException} from '../../facade/exceptions'; import {NG_ASYNC_VALIDATORS, NG_VALIDATORS} from '../../validators'; import {AbstractFormGroupDirective} from '../abstract_form_group_directive'; import {ControlContainer} from '../control_container'; +import {ReactiveErrors} from '../reactive_errors'; import {FormGroupDirective} from './form_group_directive'; @@ -86,25 +87,7 @@ export class FormGroupName extends AbstractFormGroupDirective implements OnInit, /** @internal */ _checkParentType(): void { if (!(this._parent instanceof FormGroupName) && !(this._parent instanceof FormGroupDirective)) { - this._throwParentException(); + ReactiveErrors.groupParentException(); } } - - private _throwParentException() { - throw new BaseException(`formGroupName must be used with a parent formGroup directive. - You'll want to add a formGroup directive and pass it an existing FormGroup instance - (you can create one in your class). - - Example: -
-
- -
-
- - In your class: - this.myGroup = new FormGroup({ - person: new FormGroup({ firstName: new FormControl() }) - });`); - } } diff --git a/modules/@angular/forms/src/directives/reactive_errors.ts b/modules/@angular/forms/src/directives/reactive_errors.ts new file mode 100644 index 0000000000..72cb455f79 --- /dev/null +++ b/modules/@angular/forms/src/directives/reactive_errors.ts @@ -0,0 +1,63 @@ +/** + * @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 {BaseException} from '../facade/exceptions'; +import {FormErrorExamples as Examples} from './error_examples'; + +export class ReactiveErrors { + static controlParentException(): void { + throw new BaseException( + `formControlName must be used with a parent formGroup directive. You'll want to add a formGroup + directive and pass it an existing FormGroup instance (you can create one in your class). + + Example: + + ${Examples.formControlName}`); + } + + static ngModelGroupException(): void { + throw new BaseException( + `formControlName cannot be used with an ngModelGroup parent. It is only compatible with parents + that also have a "form" prefix: formGroupName, formArrayName, or formGroup. + + Option 1: Update the parent to be formGroupName (reactive form strategy) + + ${Examples.formGroupName} + + Option 2: Use ngModel instead of formControlName (template-driven strategy) + + ${Examples.ngModelGroup}`); + } + static missingFormException(): void { + throw new BaseException(`formGroup expects a FormGroup instance. Please pass one in. + + Example: + + ${Examples.formControlName}`); + } + + static groupParentException(): void { + throw new BaseException( + `formGroupName must be used with a parent formGroup directive. You'll want to add a formGroup + directive and pass it an existing FormGroup instance (you can create one in your class). + + Example: + + ${Examples.formGroupName}`); + } + + static arrayParentException(): void { + throw new BaseException( + `formArrayName must be used with a parent formGroup directive. You'll want to add a formGroup + directive and pass it an existing FormGroup instance (you can create one in your class). + + Example: + + ${Examples.formArrayName}`); + } +} \ No newline at end of file diff --git a/modules/@angular/forms/src/directives/template_driven_errors.ts b/modules/@angular/forms/src/directives/template_driven_errors.ts new file mode 100644 index 0000000000..627be901aa --- /dev/null +++ b/modules/@angular/forms/src/directives/template_driven_errors.ts @@ -0,0 +1,61 @@ +/** + * @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 {BaseException} from '../facade/exceptions'; +import {FormErrorExamples as Examples} from './error_examples'; + +export class TemplateDrivenErrors { + static modelParentException(): void { + throw new BaseException(` + ngModel cannot be used to register form controls with a parent formGroup directive. Try using + formGroup's partner directive "formControlName" instead. Example: + + ${Examples.formControlName} + + Or, if you'd like to avoid registering this form control, indicate that it's standalone in ngModelOptions: + + Example: + + ${Examples.ngModelWithFormGroup}`); + } + + static formGroupNameException(): void { + throw new BaseException(` + ngModel cannot be used to register form controls with a parent formGroupName or formArrayName directive. + + Option 1: Use formControlName instead of ngModel (reactive strategy): + + ${Examples.formGroupName} + + Option 2: Update ngModel's parent be ngModelGroup (template-driven strategy): + + ${Examples.ngModelGroup}`); + } + + static missingNameException() { + throw new BaseException( + `If ngModel is used within a form tag, either the name attribute must be set or the form + control must be defined as 'standalone' in ngModelOptions. + + Example 1: + Example 2: `); + } + + static modelGroupParentException() { + throw new BaseException(` + ngModelGroup cannot be used with a parent formGroup directive. + + Option 1: Use formGroupName instead of ngModelGroup (reactive strategy): + + ${Examples.formGroupName} + + Option 2: Use a regular form tag instead of the formGroup directive (template-driven strategy): + + ${Examples.ngModelGroup}`); + } +} \ No newline at end of file diff --git a/modules/@angular/forms/test/reactive_integration_spec.ts b/modules/@angular/forms/test/reactive_integration_spec.ts index 492eb481e4..3d9276c9dd 100644 --- a/modules/@angular/forms/test/reactive_integration_spec.ts +++ b/modules/@angular/forms/test/reactive_integration_spec.ts @@ -1162,6 +1162,40 @@ export function main() { }); })); + it('should throw if formControlName is used with NgForm', + inject( + [TestComponentBuilder, AsyncTestCompleter], + (tcb: TestComponentBuilder, async: AsyncTestCompleter) => { + const t = ` + +
`; + + tcb.overrideTemplate(MyComp8, t).createAsync(MyComp8).then((fixture) => { + expect(() => fixture.detectChanges()) + .toThrowError(new RegExp( + `formControlName must be used with a parent formGroup directive.`)); + async.done(); + }); + })); + + it('should throw if formControlName is used with NgModelGroup', + inject( + [TestComponentBuilder, AsyncTestCompleter], + (tcb: TestComponentBuilder, async: AsyncTestCompleter) => { + const t = `
+
+ +
+
`; + + tcb.overrideTemplate(MyComp8, t).createAsync(MyComp8).then((fixture) => { + expect(() => fixture.detectChanges()) + .toThrowError( + new RegExp(`formControlName cannot be used with an ngModelGroup parent.`)); + async.done(); + }); + })); + it('should throw if formGroupName is used without a control container', inject( [TestComponentBuilder, AsyncTestCompleter], @@ -1178,6 +1212,24 @@ export function main() { }); })); + it('should throw if formGroupName is used with NgForm', + inject( + [TestComponentBuilder, AsyncTestCompleter], + (tcb: TestComponentBuilder, async: AsyncTestCompleter) => { + const t = `
+
+ +
+
`; + + tcb.overrideTemplate(MyComp8, t).createAsync(MyComp8).then((fixture) => { + expect(() => fixture.detectChanges()) + .toThrowError(new RegExp( + `formGroupName must be used with a parent formGroup directive.`)); + async.done(); + }); + })); + it('should throw if formArrayName is used without a control container', inject( [TestComponentBuilder, AsyncTestCompleter], @@ -1194,58 +1246,82 @@ export function main() { }); })); - it('should throw if formControlName is used with the wrong control container', + it('should throw if ngModel is used alone under formGroup', inject( [TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, async: AsyncTestCompleter) => { - const t = `
- -
`; + const t = `
+ +
`; tcb.overrideTemplate(MyComp8, t).createAsync(MyComp8).then((fixture) => { + fixture.debugElement.componentInstance.myGroup = new FormGroup({}); + ; expect(() => fixture.detectChanges()) .toThrowError(new RegExp( - `formControlName must be used with a parent formGroup directive.`)); + `ngModel cannot be used to register form controls with a parent formGroup directive.`)); async.done(); }); })); - it('should throw if formGroupName is used with the wrong control container', + it('should not throw if ngModel is used alone under formGroup with standalone: true', inject( [TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, async: AsyncTestCompleter) => { - const t = `
-
- -
-
`; + const t = `
+ +
`; tcb.overrideTemplate(MyComp8, t).createAsync(MyComp8).then((fixture) => { - expect(() => fixture.detectChanges()) - .toThrowError(new RegExp( - `formGroupName must be used with a parent formGroup directive.`)); + fixture.debugElement.componentInstance.myGroup = new FormGroup({}); + expect(() => fixture.detectChanges()).not.toThrowError(); async.done(); }); })); - it('should throw if formArrayName is used with the wrong control container', + it('should throw if ngModel is used alone with formGroupName', inject( [TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, async: AsyncTestCompleter) => { - const t = `
-
- -
-
`; + const t = `
+
+ +
+
`; + + const myGroup = new FormGroup({person: new FormGroup({})}); tcb.overrideTemplate(MyComp8, t).createAsync(MyComp8).then((fixture) => { + fixture.debugElement.componentInstance.myGroup = + new FormGroup({person: new FormGroup({})}); + expect(() => fixture.detectChanges()) .toThrowError(new RegExp( - `formArrayName must be used with a parent formGroup directive.`)); + `ngModel cannot be used to register form controls with a parent formGroupName or formArrayName directive.`)); async.done(); }); })); + it('should throw if ngModelGroup is used with formGroup', + inject( + [TestComponentBuilder, AsyncTestCompleter], + (tcb: TestComponentBuilder, async: AsyncTestCompleter) => { + const t = `
+
+ +
+
`; + + tcb.overrideTemplate(MyComp8, t).createAsync(MyComp8).then((fixture) => { + fixture.debugElement.componentInstance.myGroup = new FormGroup({}); + expect(() => fixture.detectChanges()) + .toThrowError(new RegExp( + `ngModelGroup cannot be used with a parent formGroup directive`)); + async.done(); + }); + })); + + it('should throw if radio button name does not match formControlName attr', inject( [TestComponentBuilder, AsyncTestCompleter],