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
feat(forms): add FormBuilder.record()
method
#46485
Conversation
There was a problem hiding this 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
* * `updateOn`: The event upon which the control should be updated (options: 'change' | 'blur' | ||
* | submit'). | ||
*/ | ||
record<T>(controls: {[key: string]: T}, options: AbstractControlOptions|null = null): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
.
ffeb711
to
aec7b0b
Compare
aec7b0b
to
7a5c5f7
Compare
There was a problem hiding this 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
@AndrewKushnir Can I get your review on this when you have time? I'm hoping to merge this for 14.1. |
There was a problem hiding this 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).
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`
7a5c5f7
to
1d8acec
Compare
There was a problem hiding this 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
There was a problem hiding this 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
This PR was merged into the repository by commit 426af91. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
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:
This works for both the
FormBuilder
and theNonNullableFormBuilder
Does this PR introduce a breaking change?
Other information