From 8a9fe49a2abe6499b2c84fe22f6214e2a7b9befc Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Fri, 12 Feb 2021 16:26:08 -0800 Subject: [PATCH] fix(forms): properly handle the change to the FormGroup shape (#40829) Currently the code in the `FormGroupDirective` assumes that the shape of the underlying `FormGroup` never changes and `FormControl`s are not replaced with other types. In practice this is possible and Forms code should be able to process such changes in FormGroup shape. This commit adds extra check to the `FormGroupDirective` class to avoid applying FormControl-specific to other types. Fixes #13788. PR Close #40829 --- .../form_group_directive.ts | 19 +- .../forms/test/reactive_integration_spec.ts | 183 ++++++++++++++++++ 2 files changed, 197 insertions(+), 5 deletions(-) diff --git a/packages/forms/src/directives/reactive_directives/form_group_directive.ts b/packages/forms/src/directives/reactive_directives/form_group_directive.ts index e8fefe0f74..fb339db997 100644 --- a/packages/forms/src/directives/reactive_directives/form_group_directive.ts +++ b/packages/forms/src/directives/reactive_directives/form_group_directive.ts @@ -295,13 +295,22 @@ export class FormGroupDirective extends ControlContainer implements Form, OnChan /** @internal */ _updateDomValue() { this.directives.forEach(dir => { - const newCtrl: any = this.form.get(dir.path); - if (dir.control !== newCtrl) { + const oldCtrl = dir.control; + const newCtrl = this.form.get(dir.path); + if (oldCtrl !== newCtrl) { // Note: the value of the `dir.control` may not be defined, for example when it's a first // `FormControl` that is added to a `FormGroup` instance (via `addControl` call). - cleanUpControl(dir.control || null, dir); - if (newCtrl) setUpControl(newCtrl, dir); - (dir as {control: FormControl}).control = newCtrl; + cleanUpControl(oldCtrl || null, dir); + + // Check whether new control at the same location inside the corresponding `FormGroup` is an + // instance of `FormControl` and perform control setup only if that's the case. + // Note: we don't need to clear the list of directives (`this.directives`) here, it would be + // taken care of in the `removeControl` method invoked when corresponding `formControlName` + // directive instance is being removed (invoked from `FormControlName.ngOnDestroy`). + if (newCtrl instanceof FormControl) { + setUpControl(newCtrl, dir); + (dir as {control: FormControl}).control = newCtrl; + } } }); diff --git a/packages/forms/test/reactive_integration_spec.ts b/packages/forms/test/reactive_integration_spec.ts index 63c1993b06..7cecc14201 100644 --- a/packages/forms/test/reactive_integration_spec.ts +++ b/packages/forms/test/reactive_integration_spec.ts @@ -663,6 +663,189 @@ const ValueAccessorB = createControlValueAccessor('[cva-b]'); expect(input.nativeElement.getAttribute('disabled')).toBe(null); }); }); + + describe('dynamic change of FormGroup and FormArray shapes', () => { + it('should handle FormControl and FormGroup swap', () => { + @Component({ + template: ` +
+ + + + +
+ ` + }) + class App { + showAsGroup = false; + form!: FormGroup; + + useStandaloneControl() { + this.showAsGroup = false; + this.form = new FormGroup({ + name: new FormControl('standalone'), + }); + } + + useControlInsideGroup() { + this.showAsGroup = true; + this.form = new FormGroup({ + name: new FormGroup({ + control: new FormControl('inside-group'), + }) + }); + } + } + + const fixture = initTest(App); + fixture.componentInstance.useStandaloneControl(); + fixture.detectChanges(); + + let input = fixture.nativeElement.querySelector('input'); + expect(input.id).toBe('standalone-id'); + expect(input.value).toBe('standalone'); + + // Replace `FormControl` with `FormGroup` at the same location + // in data model and trigger change detection. + fixture.componentInstance.useControlInsideGroup(); + fixture.detectChanges(); + + input = fixture.nativeElement.querySelector('input'); + expect(input.id).toBe('inside-group-id'); + expect(input.value).toBe('inside-group'); + + // Swap `FormGroup` with `FormControl` back at the same location + // in data model and trigger change detection. + fixture.componentInstance.useStandaloneControl(); + fixture.detectChanges(); + + input = fixture.nativeElement.querySelector('input'); + expect(input.id).toBe('standalone-id'); + expect(input.value).toBe('standalone'); + }); + + it('should handle FormControl and FormArray swap', () => { + @Component({ + template: ` +
+ + + + +
+ ` + }) + class App { + showAsArray = false; + form!: FormGroup; + + useStandaloneControl() { + this.showAsArray = false; + this.form = new FormGroup({ + name: new FormControl('standalone'), + }); + } + + useControlInsideArray() { + this.showAsArray = true; + this.form = new FormGroup({ + name: new FormArray([ + new FormControl('inside-array') // + ]) + }); + } + } + + const fixture = initTest(App); + fixture.componentInstance.useStandaloneControl(); + fixture.detectChanges(); + + let input = fixture.nativeElement.querySelector('input'); + expect(input.id).toBe('standalone-id'); + expect(input.value).toBe('standalone'); + + // Replace `FormControl` with `FormArray` at the same location + // in data model and trigger change detection. + fixture.componentInstance.useControlInsideArray(); + fixture.detectChanges(); + + input = fixture.nativeElement.querySelector('input'); + expect(input.id).toBe('inside-array-id'); + expect(input.value).toBe('inside-array'); + + // Swap `FormArray` with `FormControl` back at the same location + // in data model and trigger change detection. + fixture.componentInstance.useStandaloneControl(); + fixture.detectChanges(); + + input = fixture.nativeElement.querySelector('input'); + expect(input.id).toBe('standalone-id'); + expect(input.value).toBe('standalone'); + }); + + it('should handle FormGroup and FormArray swap', () => { + @Component({ + template: ` +
+ + + + + + +
+ ` + }) + class App { + showAsArray = false; + form!: FormGroup; + + useControlInsideGroup() { + this.showAsArray = false; + this.form = new FormGroup({ + name: new FormGroup({ + control: new FormControl('inside-group'), + }) + }); + } + + useControlInsideArray() { + this.showAsArray = true; + this.form = new FormGroup({ + name: new FormArray([ + new FormControl('inside-array') // + ]) + }); + } + } + + const fixture = initTest(App); + fixture.componentInstance.useControlInsideGroup(); + fixture.detectChanges(); + + let input = fixture.nativeElement.querySelector('input'); + expect(input.id).toBe('inside-group-id'); + expect(input.value).toBe('inside-group'); + + // Replace `FormGroup` with `FormArray` at the same location + // in data model and trigger change detection. + fixture.componentInstance.useControlInsideArray(); + fixture.detectChanges(); + + input = fixture.nativeElement.querySelector('input'); + expect(input.id).toBe('inside-array-id'); + expect(input.value).toBe('inside-array'); + + // Swap `FormArray` with `FormGroup` back at the same location + // in data model and trigger change detection. + fixture.componentInstance.useControlInsideGroup(); + fixture.detectChanges(); + + input = fixture.nativeElement.querySelector('input'); + expect(input.id).toBe('inside-group-id'); + expect(input.value).toBe('inside-group'); + }); + }); }); describe('user input', () => {