Skip to content
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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

feat(forms): AddFormArrayDirective #55880

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat(forms): AddFormArrayDirective #55880

wants to merge 2 commits into from

Conversation

JeanMeche
Copy link
Member

@JeanMeche JeanMeche commented May 20, 2024 •
edited
Loading

TheFormArrayDirectivewill allow to have aFormArrayas a top-level form object.

  • NgControlStatusGroupdirective will be applied to theFormArrayDirective
  • NgFormwill still create aFormGroup

Usage:

@Component({
selector: 'form-array-comp',
template: `
<form [formArray]= "form" >
@for(_ of controls; track $index) {
<input [formControlName]= "$index" >
}
</form>`,
})
class FormArrayComp {
controls = [new FormControl('fish'), new FormControl('cat'), new FormControl('dog')];
form = new FormArray(this.controls);
}

Fixes#30264

BREAKING CHANGE: This new directive will conflict with existing FormArray directives or formArray inputs on the same element.

@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label May 20, 2024
@ngbot ngbot bot added this to theBacklogmilestone May 20, 2024
@JeanMeche JeanMeche force-pushed the feat/formArrayDirective branch 10 times, most recently from 35741f0 to 2d950c1 Compare May 20, 2024 20:30
@JeanMeche JeanMeche marked this pull request as ready for review May 20, 2024 21:40
@pullapprove pullapprove bot requested review from atscottand dylhunn May 20, 2024 21:40
@JeanMeche JeanMeche added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jun 9, 2024
@dylhunn dylhunn requested review from AndrewKushnir and removed request for atscott June 17, 2024 15:52
AndrewKushnir
AndrewKushnir reviewed Jun 17, 2024
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@JeanMecheI had a quick look and the changes look good. I want to ask if you could split the commit into 2:

  • firstrefactorcommit performs a refactoring and extracts out the common logic into the abstract form directive class
  • the secondfeatcommit that adds a new directive

That should help to make the change more incremental and help with more detailed code review.

@AndrewKushnir AndrewKushnir added the target: minor This PR is targeted for the next minor release label Jun 17, 2024
@JeanMeche JeanMeche force-pushed the feat/formArrayDirective branch 2 times, most recently from 7a959ab to e7d47d9 Compare June 18, 2024 10:26
dylhunn
dylhunn approved these changes Jul 1, 2024
View reviewed changes
Copy link
Collaborator

@dylhunn dylhunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks for splitting up the change in this way! Let's merge thisafterthe RC, so we can get it on the 18.2 track, and we'll have plenty of time to receive feedback from GDEs and other early users.

@dylhunn dylhunn requested review from AndrewKushnir and removed request for atscott July 1, 2024 02:51
AndrewKushnir
AndrewKushnir reviewed Jul 9, 2024
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The change looks good, thanks for splitting the change into 2 commits 👍

I've left a few comments, but a bigger question that I have is about potential collisions with existing[formArray]usages in templates:

  • as a custom directive
  • as an input to a component

I've checked internal codebase and there are ~75 instances of the[formArray]in templates. Adding a new directive to theReactiveFormsModulewill cause conflicts with those locations.

I think we'd need to do some research to find a way to avoid those conflicts or do some cleanup internally. cc@dylhunnto get your opinion on this as well.

packages/forms/src/directives/reactive_directives/form_group_name.ts Outdated Show resolved Hide resolved
packages/forms/test/reactive_integration_spec.ts
@@ -5812,6 +5825,132 @@ describe('reactive forms integration tests', () => {
fixture.destroy();
}).not.toThrow();
});

describe('formArray support', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'm curious if we may want to add a check that[formArray]and[formGroup]should not be present on the same element? Can we have valid use-cases where it might be needed (e.g. they bind to different data structures)?

packages/forms/src/directives/reactive_directives/form_array_directive.ts Outdated Show resolved Hide resolved
packages/forms/src/directives/reactive_directives/form_group_directive.ts Outdated Show resolved Hide resolved
@pullapprove pullapprove bot requested a review fromalxhub July 9, 2024 01:38
@JeanMeche JeanMeche added breaking changes target: major This PR is targeted for the next major release and removed target: minor This PR is targeted for the next minor release labels Jul 25, 2024
@JeanMeche
Copy link
Member Author

I'm moving this to major / breaking change as it was discovered that there were already usages of a formArray directive (or formArray as input) inside Google.

@JeanMeche
refactor(forms): extract shared logic fromFormGroupDirective
0a02ed1 
Ahead of the implementation of `FormArrayDirective`, extract the shared logic into an abstract class.
@JeanMeche
feat(forms): Add FormArrayDirective
949e2e0 
The `FormArrayDirective` will allow to have a `FormArray` as a top-level form object.

* `NgControlStatusGroup` directive will be applied to the `FormArrayDirective`
* `NgForm` will still create a `FormGroup`

Fixesangular#30264

BREAKING CHANGE: This new directive will conflict with existing FormArray directives or formArray inputs on the same element.
@angular-robot angular-robot bot added the detected: breaking change PR contains a commit with a breaking change label Jul 25, 2024
JeanMeche added a commit to JeanMeche/angular that referenced this pull request Sep 26, 2024
@JeanMeche
refactor(forms): Expose a union type for parent form containers.
6674bf7 
This commit exposes a new type to the public api.
It is intented to also include the upcomming `FormArrayDirective` (angular#55880)
@JeanMeche JeanMeche mentioned this pull request Sep 26, 2024
Open
JeanMeche added a commit to JeanMeche/angular that referenced this pull request Sep 26, 2024
@JeanMeche
refactor(forms): Expose a union type for parent form containers.
ed7e7f7 
This commit exposes a new type to the public api.
It is intented to also include the upcomming `FormArrayDirective` (angular#55880)
@alxhub alxhub removed this from thev19 Feature Freeze Candidatesmilestone Oct 21, 2024
@ngbot ngbot bot added this to theBacklogmilestone Oct 21, 2024
@alxhub alxhub modified the milestones: Backlog, v19.1 candidates Oct 21, 2024
@ngbot ngbot bot added this to theBacklogmilestone Oct 21, 2024
@alxhub alxhub modified the milestones: Backlog, v20 candidates Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndrewKushnir AndrewKushnir AndrewKushnir left review comments

@dylhunn dylhunn Awaiting requested review from dylhunn

@thePunderWoman thePunderWoman Awaiting requested review from thePunderWoman

@alxhub alxhub Awaiting requested review from alxhub

Assignees
No one assigned
Labels
action: review The PR is still awaiting reviews from at least one requested reviewer area: forms breaking changes detected: breaking change PR contains a commit with a breaking change detected: feature PR contains a feature commit target: major This PR is targeted for the next major release
Projects
None yet
Milestone
v20 candidates
Development

Successfully merging this pull request may close these issues.

Supporting [formArray] without a [formGroup] parent
4 participants
@JeanMeche @dylhunn @AndrewKushnir @alxhub