Skip to content

feat(forms): add FormBuilder.record() method#46485

Closed
cexbrayat wants to merge 1 commit intoangular:mainfrom
cexbrayat:feat/form-builder-record
Closed

feat(forms): add FormBuilder.record() method#46485
cexbrayat wants to merge 1 commit intoangular:mainfrom
cexbrayat:feat/form-builder-record

Conversation

@cexbrayat
Copy link
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

The new FormRecord entity introduced in Angular v14 does not have its builder method.

What is the new behavior?

This commit adds it, allowing to write:

const fb = new FormBuilder();
fb.record({ a: 'one' });

This works for both the FormBuilder and the NonNullableFormBuilder

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@cexbrayat cexbrayat requested a review from dylhunn June 24, 2022 08:17
@dylhunn dylhunn added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: review The PR is still awaiting reviews from at least one requested reviewer area: forms target: minor This PR is targeted for the next minor release forms: strictly typed labels Jun 24, 2022
@ngbot ngbot bot modified the milestone: Backlog Jun 24, 2022
Copy link
Contributor

@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.

One more thing, can you also accept your API changes once all code changes are made?

yarn bazel run //packages/forms:forms_api.accept  

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason for this compile error is that the type constraint on the FormRecord class is overly specific. I sent more details on Slack.

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed on Slack, I relaxed the type of FormRecord from FormRecord<TControl extends AbstractControl<ɵValue<TControl>, ɵRawValue<TControl>> = AbstractControl> to FormRecord<TControl extends AbstractControl = AbstractControl>.

@cexbrayat cexbrayat force-pushed the feat/form-builder-record branch 2 times, most recently from ffeb711 to aec7b0b Compare July 13, 2022 09:08
@cexbrayat cexbrayat marked this pull request as ready for review July 13, 2022 09:09
@cexbrayat cexbrayat force-pushed the feat/form-builder-record branch from aec7b0b to 7a5c5f7 Compare July 13, 2022 09:30
Copy link
Contributor

@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.

reviewed-for: fw-forms, fw-core, public-api

@dylhunn dylhunn requested review from AndrewKushnir and removed request for dylhunn July 13, 2022 16:25
@dylhunn
Copy link
Contributor

dylhunn commented Jul 13, 2022

@AndrewKushnir Can I get your review on this when you have time? I'm hoping to merge this for 14.1.

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.

@cexbrayat the change looks great, just one comment about a potential refactoring to reduce the amount of code (and an app bundle size eventually).

@pullapprove pullapprove bot requested a review from alxhub July 13, 2022 18:43
The new `FormRecord` entity introduced in Angular v14 does not have its builder method.
This commit adds it, allowing to write:

```
const fb = new FormBuilder();
fb.record({ a: 'one' });
```

This works for both the `FormBuilder` and the `NonNullableFormBuilder`
@cexbrayat cexbrayat force-pushed the feat/form-builder-record branch from 7a5c5f7 to 1d8acec Compare July 14, 2022 06:56
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

@dylhunn dylhunn added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 15, 2022
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: size-tracking

@jessicajaniuk
Copy link
Contributor

This PR was merged into the repository by commit 426af91.

@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 Aug 15, 2022
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 forms: strictly typed target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants