New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(forms): fully support rebinding form group directive #11051

Merged
merged 1 commit into from Aug 25, 2016

Conversation

Projects
None yet
10 participants
@kara
Contributor

kara commented Aug 24, 2016

This PR makes it possible to rebind the top level FormGroup instance passed to the formGroup directive. Previously, when you'd call someMethod():

@Component({
   selector: 'my-comp',
   template: `
      <form [formGroup]="form">
         <input formControlName="name">
      </form>
   `
})
class MyComp {
   form  = new FormGroup({
     name: new FormControl()
   });

  someMethod() {
    this.form = new FormGroup({
      name: new FormControl()
    }); 
   }
}

... the input elements in the UI would become detached from the model because the formControlName directives would still be attached to form control instances from the previous FormGroup. Now you should be able to rebind without this problem.

This has implications for things like FormArrays, where you might have added form controls dynamically. Rebinding the FormGroup can reset these form arrays to their original number of controls.

Fixes #11025 and #10960.

@DanielYKPan

This comment has been minimized.

Show comment
Hide comment
@DanielYKPan

DanielYKPan Aug 25, 2016

The form group rebinding works good.
But what if I make the form group after a service async function returning a value (this value would be used to bind to the form)
`
class AddressFormComponent implements OnInit {

addressForm: FormGroup;
inform: Address;

constructor( private formBuilder: FormBuilder,
             private addressService: AddressService ) {
}

ngOnInit() {
    this.getAddress();
}

getAddress() {
    this.addressService.getAddress().then(
        data => this.onAddressRetrieved(data)
    );
}

private onAddressRetrieved(address: Address) {
    console.log(address);
    this.inform = address;

    this.addressForm = this.formBuilder.group({
        userName: [this.inform.username, Validators.required],
        address: this.formBuilder.group({
            street: this.inform.address.street,
            zip: [this.inform.address.zip, Validators.required)],
            city: this.inform.address.city
        })
    });
}

}
`
Right now i got an error 'formGroup expects a FormGroup instance'.

I guess this is because the view is expecting to get a form group instance, but I make this form group instance after a async service (I have to call this service to initiate the form value).

The form group rebinding works good.
But what if I make the form group after a service async function returning a value (this value would be used to bind to the form)
`
class AddressFormComponent implements OnInit {

addressForm: FormGroup;
inform: Address;

constructor( private formBuilder: FormBuilder,
             private addressService: AddressService ) {
}

ngOnInit() {
    this.getAddress();
}

getAddress() {
    this.addressService.getAddress().then(
        data => this.onAddressRetrieved(data)
    );
}

private onAddressRetrieved(address: Address) {
    console.log(address);
    this.inform = address;

    this.addressForm = this.formBuilder.group({
        userName: [this.inform.username, Validators.required],
        address: this.formBuilder.group({
            street: this.inform.address.street,
            zip: [this.inform.address.zip, Validators.required)],
            city: this.inform.address.city
        })
    });
}

}
`
Right now i got an error 'formGroup expects a FormGroup instance'.

I guess this is because the view is expecting to get a form group instance, but I make this form group instance after a async service (I have to call this service to initiate the form value).

@kara

This comment has been minimized.

Show comment
Hide comment
@kara

kara Aug 25, 2016

Contributor

@DanielYKPan The FormGroup instance must exist from the start. There's no reason to delay its creation. If you want to set value asynchronously, you can do so as soon as the value is available with setValue() or patchValue().

Contributor

kara commented Aug 25, 2016

@DanielYKPan The FormGroup instance must exist from the start. There's no reason to delay its creation. If you want to set value asynchronously, you can do so as soon as the value is available with setValue() or patchValue().

@vicb vicb merged commit 515ff61 into angular:master Aug 25, 2016

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@DanielYKPan

This comment has been minimized.

Show comment
Hide comment
@DanielYKPan

DanielYKPan Aug 26, 2016

@kara Oh, I get it. Thank you.

@kara Oh, I get it. Thank you.

@Keith-Ball

This comment has been minimized.

Show comment
Hide comment
@Keith-Ball

Keith-Ball Aug 30, 2016

I was actually having issues with your solution because I wanted the FormArray to refresh and not the whole group. I figured I could do something along the lines of formArray = new FormArray([]). This seemed to work, but when I would loop over my options it would say that anything past index 0 didn't exist.

<app-form-input *ngFor="let option of options; let i = index;" [name]="option.name" [formControlName]="i" [error]="submitted && !formHandler.find('formArray').at(i).valid"></app-form-input>

The above code broke whenever I switched to having more than one option. I was able to empty the array using the code below, but it does seem a little awkward.

oldOptions.forEach(o => this.formArray.removeAt(0))
newOptions.forEach(o => this.formArray.push(newFormControl('', Validators.required))

I was actually having issues with your solution because I wanted the FormArray to refresh and not the whole group. I figured I could do something along the lines of formArray = new FormArray([]). This seemed to work, but when I would loop over my options it would say that anything past index 0 didn't exist.

<app-form-input *ngFor="let option of options; let i = index;" [name]="option.name" [formControlName]="i" [error]="submitted && !formHandler.find('formArray').at(i).valid"></app-form-input>

The above code broke whenever I switched to having more than one option. I was able to empty the array using the code below, but it does seem a little awkward.

oldOptions.forEach(o => this.formArray.removeAt(0))
newOptions.forEach(o => this.formArray.push(newFormControl('', Validators.required))

@kara

This comment has been minimized.

Show comment
Hide comment
@kara

kara Aug 30, 2016

Contributor

@Keith-Ball This PR only applies to rebinding the top-level form group, not nested groups or arrays. That is definitely something we'd like to support down the line, but it's not implemented yet. If you are passionate about that feature, feel free to open an issue with an example plunker. The PR isn't really the right place to track.

Rereading your comment though, it sounds like you are reassigning the form array in your component class, but not in your actual parent FormGroup. If you do that, the parent form won't have a reference to the correct form array because you've reassigned it. What we are planning to support is something like: this.form.setControl('name', new FormArray([]));, where name is the name of your FormArray.

Contributor

kara commented Aug 30, 2016

@Keith-Ball This PR only applies to rebinding the top-level form group, not nested groups or arrays. That is definitely something we'd like to support down the line, but it's not implemented yet. If you are passionate about that feature, feel free to open an issue with an example plunker. The PR isn't really the right place to track.

Rereading your comment though, it sounds like you are reassigning the form array in your component class, but not in your actual parent FormGroup. If you do that, the parent form won't have a reference to the correct form array because you've reassigned it. What we are planning to support is something like: this.form.setControl('name', new FormArray([]));, where name is the name of your FormArray.

@ThunderRoid

This comment has been minimized.

Show comment
Hide comment
@ThunderRoid

ThunderRoid Nov 18, 2016

This is how i dd it

emailControls: FormArray;
...
...
...
var len = this.emailControls.length;
if (len > 1)
  for (var i = len - 1; i > 0; i--)
    this.emailControls.removeAt(i);

ThunderRoid commented Nov 18, 2016

This is how i dd it

emailControls: FormArray;
...
...
...
var len = this.emailControls.length;
if (len > 1)
  for (var i = len - 1; i > 0; i--)
    this.emailControls.removeAt(i);
@steve3d

This comment has been minimized.

Show comment
Hide comment
@steve3d

steve3d Feb 22, 2017

emailControls: FormArray;
...
while (emailControls.length) {
    emailControls.removeAt(emailControls.length - 1);
}

@ThunderRoid 's solution will always leave the first item in array.

steve3d commented Feb 22, 2017

emailControls: FormArray;
...
while (emailControls.length) {
    emailControls.removeAt(emailControls.length - 1);
}

@ThunderRoid 's solution will always leave the first item in array.

@andreipivoda

This comment has been minimized.

Show comment
Hide comment
@andreipivoda

andreipivoda Feb 28, 2017

while (emailControls.length) {
emailControls.removeAt(0);
}
should do the job of clearing the array

while (emailControls.length) {
emailControls.removeAt(0);
}
should do the job of clearing the array

pratheekhegde added a commit to swayamprabha/sptt-erp-ui that referenced this pull request Apr 7, 2017

pratheekhegde added a commit to swayamprabha/sptt-erp-ui that referenced this pull request Apr 7, 2017

@Keith-Ball

This comment has been minimized.

Show comment
Hide comment
@Keith-Ball

Keith-Ball Jul 11, 2017

@Javirln Because the conditional should be >= 0 rather than > 0

# When i is zero, condition will be false and it will break the loop
# Even though 0 is still a valid index to loop over in this case
  for (var i = len - 1; i > 0; i--)
    this.emailControls.removeAt(i);

Keith-Ball commented Jul 11, 2017

@Javirln Because the conditional should be >= 0 rather than > 0

# When i is zero, condition will be false and it will break the loop
# Even though 0 is still a valid index to loop over in this case
  for (var i = len - 1; i > 0; i--)
    this.emailControls.removeAt(i);
@JoeKahl

This comment has been minimized.

Show comment
Hide comment
@JoeKahl

JoeKahl Aug 31, 2017

This was very helpful. Thank you. Below is my code.
I opted to keep one occurrence of each because the form requires one of each.

`
// Wipe out any extra occurrences and reset the form.
var index: number;
let phoneGroup = this.form.controls['phoneGroup'];
for (index = phoneGroup.length - 1; index > 0; index--) {
// Remove all but one occurrence and then add back only what the model dictates.
phoneGroup.removeAt(index);
}

let emailGroup = <FormArray> this.form.controls['emailGroup'];
for (index = emailGroup.length - 1; index > 0; index--) {
  // Remove all but one occurrence and then add back only what the model dictates.
  emailGroup.removeAt(index);
}

let addressGroup = <FormArray> this.form.controls['addressGroup'];
for (index = addressGroup.length - 1; index > 0; index--) {
  // Remove all but one occurrence and then add back only what the model dictates.
  addressGroup.removeAt(index);
}

this.form.reset();

`

JoeKahl commented Aug 31, 2017

This was very helpful. Thank you. Below is my code.
I opted to keep one occurrence of each because the form requires one of each.

`
// Wipe out any extra occurrences and reset the form.
var index: number;
let phoneGroup = this.form.controls['phoneGroup'];
for (index = phoneGroup.length - 1; index > 0; index--) {
// Remove all but one occurrence and then add back only what the model dictates.
phoneGroup.removeAt(index);
}

let emailGroup = <FormArray> this.form.controls['emailGroup'];
for (index = emailGroup.length - 1; index > 0; index--) {
  // Remove all but one occurrence and then add back only what the model dictates.
  emailGroup.removeAt(index);
}

let addressGroup = <FormArray> this.form.controls['addressGroup'];
for (index = addressGroup.length - 1; index > 0; index--) {
  // Remove all but one occurrence and then add back only what the model dictates.
  addressGroup.removeAt(index);
}

this.form.reset();

`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment