Skip to content

perf(forms): optimize [field] binding instructions#64351

Closed
leonsenft wants to merge 1 commit intoangular:mainfrom
leonsenft:signal-forms-control-optimize
Closed

perf(forms): optimize [field] binding instructions#64351
leonsenft wants to merge 1 commit intoangular:mainfrom
leonsenft:signal-forms-control-optimize

Conversation

@leonsenft
Copy link
Copy Markdown
Contributor

Caches information about the kind of form control that a TNode represents in TNodeFlags. This avoids redundant computations on subsequent template create and update passes.

Renames the INVALID_CONTROL_HOST error code to
INVALID_FIELD_DIRECTIVE_HOST for clarity and adds a test for it.

@leonsenft leonsenft requested a review from mmalerba October 10, 2025 19:36
@ngbot ngbot bot added this to the Backlog milestone Oct 10, 2025
@pullapprove pullapprove bot requested a review from crisbeto October 10, 2025 19:36
@angular-robot angular-robot bot added the area: performance Issues related to performance label Oct 10, 2025
@leonsenft leonsenft removed the request for review from crisbeto October 10, 2025 19:36
@pullapprove pullapprove bot requested a review from crisbeto October 10, 2025 19:36
if (isComponentHost(tNode)) {
const componentDef = tView.data[componentIndex] as ComponentDef<unknown>;
// TODO: should we check that any additional field state inputs are signal based?
if (hasModelInput(componentDef, 'value')) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we be concerned about property renaming here? I'm guessing no, but it's worth double checking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Are you concerned about the 'value' string literal here? It's the public/template name of the input used as a key for

/**
* A dictionary mapping the inputs' public name to their minified property names
* (along with flags if there are any).
*/
readonly inputs: Record<
string,
[minifiedName: string, flags: InputFlags, transform: InputTransformFunction | null]
>;

My understanding is that this preserves the public/template name so that inputs can be matched at runtime, then this map is used to map to their minified names.

@pullapprove pullapprove bot requested a review from crisbeto October 10, 2025 19:39

if (isComponentHost(tNode)) {
const componentDef = tView.data[componentIndex] as ComponentDef<unknown>;
// TODO: should we check that any additional field state inputs are signal based?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe the previous hacky implementation did, and this one probably should too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, but does it need to? This code works if the inputs aren't signal based since we go through writeToDirectiveInput which handles the disambiguation.

The only reason we might want/need to check is that it ensures the custom types conforms to the FormUiControl interface, but that seems like a guideline anyways since we can't used that to determine whether it's a custom control at runtime anyways.

@pullapprove pullapprove bot requested a review from devversion October 11, 2025 00:56
Copy link
Copy Markdown
Member

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

Caches information about the kind of form control that a `TNode`
represents in `TNodeFlags`. This avoids redundant computations on
subsequent template create and update passes.

Renames the `INVALID_CONTROL_HOST` error code to
`INVALID_FIELD_DIRECTIVE_HOST` for clarity and adds a test for it.
@leonsenft leonsenft force-pushed the signal-forms-control-optimize branch from 78ea6c4 to d5dc8f2 Compare October 13, 2025 17:08
@leonsenft leonsenft added the target: major This PR is targeted for the next major release label Oct 13, 2025
@leonsenft leonsenft removed the request for review from crisbeto October 13, 2025 18:29
@leonsenft leonsenft added the action: merge The PR is ready for merge by the caretaker label Oct 13, 2025
@AndrewKushnir
Copy link
Copy Markdown
Contributor

This PR was merged into the repository. The changes were merged into the following branches:

@leonsenft leonsenft deleted the signal-forms-control-optimize branch October 13, 2025 21:30
@angular-automatic-lock-bot
Copy link
Copy Markdown

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 Nov 13, 2025
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 area: performance Issues related to performance forms: signals target: major This PR is targeted for the next major release

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants