Skip to content

feat(forms): Add a FormRecord type.#45607

Closed
dylhunn wants to merge 1 commit intoangular:masterfrom
dylhunn:formrecord
Closed

feat(forms): Add a FormRecord type.#45607
dylhunn wants to merge 1 commit intoangular:masterfrom
dylhunn:formrecord

Conversation

@dylhunn
Copy link
Contributor

@dylhunn dylhunn commented Apr 12, 2022

As part of the typed forms RFC, we proposed the creation of a new FormRecord type, to support dynamic groups with homogenous values. This PR introduces FormRecord, as a subclass of FormGroup.

Issue #13721

@dylhunn dylhunn added action: review The PR is still awaiting reviews from at least one requested reviewer area: forms target: major This PR is targeted for the next major release forms: Controls API Issues related to AbstractControl, FormControl, FormGroup, FormArray. cross-cutting: types labels Apr 12, 2022
@dylhunn dylhunn added this to the v14-candidates milestone Apr 12, 2022
@dylhunn dylhunn requested a review from AndrewKushnir April 12, 2022 22:43
@kbrilla
Copy link
Contributor

kbrilla commented Apr 13, 2022

  • What is gonna be the difference between FormGroup<Record<string, TValue>> and FormRecord<TValue>? If there is none why actually extend from AbstractControl and not just make it a type alias like FormRecord = FormGroup<Record<string, TValue>>;
  • Right now FormRecord only takes TControl for the value type of controls but what if I want to narrow allowed control names to some type like type allowedKeys = 'first' | 'second' or keyof someType to get something like FormGroup<Record<allowedKeys , TValue>> ?

sorry for probably a lot of oversimplifications

ps. I saw that it is supposed to be open ended so mayby more like FormGroup<Partial<Record<allowedKeys , TValue>>>would be more precise?

@dylhunn
Copy link
Contributor Author

dylhunn commented Apr 13, 2022

@criskrzysiu

What is gonna be the difference between FormGroup<Record<string, TValue>> and FormRecord?

There is very little difference -- the types are a bit more permissive around removing and adding controls, in a way that's difficult to implement if everything just used FormGroup.

why actually extend from AbstractControl

We must extend FormGroup because there are some backwards-compatibility warts around .parent, which returns FormGroup|FormArray.

make it a type alias like FormRecord = FormGroup<Record<string, TValue>>;

By having a separate type, we can relax some of the constraints. e.g. addControl takes a string instead of a keyof TControl, and you can always call removeControl.

@kbrilla
Copy link
Contributor

kbrilla commented Apr 13, 2022

Thank You for the explanation

By having a separate type, we can relax some of the constraints. e.g. addControl takes a string instead of a keyof TControl, and you can always call removeControl.
Still I think that not being to put a constraint on the name of the controls is actually a step down.

But still as it is not super simple could I ask for a nice example for when to use FormGroup<Partial<Record<string, TValue>>> and FormRecord<TValue> in the docs? I think the subtleties will be the most difficult and confusing parts that would be error prone

@dylhunn
Copy link
Contributor Author

dylhunn commented Apr 13, 2022

@criskrzysiu

could I ask for a nice example for when to use FormGroup<Partial<Record<string, TValue>>> and FormRecord in the docs?

Always use FormRecord, never use FormGroup<Record<...>>. I haven't written all the docs yet, but I will do so in the coming weeks, including this topic!

As part of the typed forms RFC, we proposed the creation of a new FormRecord type, to support dynamic groups with homogenous values. This PR introduces FormRecord, as a subclass of FormGroup.
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.

Reviewed-for: public-api

Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

@dylhunn dylhunn added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 14, 2022
@ngbot
Copy link

ngbot bot commented Apr 14, 2022

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "ci/circleci: legacy-unit-tests-saucelabs" is failing
    pending status "google3" is pending
    pending 2 pending code reviews

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@dylhunn
Copy link
Contributor Author

dylhunn commented Apr 14, 2022

This PR was merged into the repository by commit e0a2248.

@dylhunn dylhunn closed this in e0a2248 Apr 14, 2022
@Harpush
Copy link

Harpush commented Apr 21, 2022

@criskrzysiu

could I ask for a nice example for when to use FormGroup<Partial<Record<string, TValue>>> and FormRecord in the docs?

Always use FormRecord, never use FormGroup<Record<...>>. I haven't written all the docs yet, but I will do so in the coming weeks, including this topic!

Unless the form group record type is not string but say an enum I believe... Am I correct?

@kbrilla
Copy link
Contributor

kbrilla commented Apr 21, 2022 via email

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 10, 2022
@dylhunn dylhunn deleted the formrecord branch October 17, 2022 13:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: forms cross-cutting: types forms: Controls API Issues related to AbstractControl, FormControl, FormGroup, FormArray. forms: strictly typed target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants