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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

perf(forms): make built-in ControlValueAccessors more tree-shakable #41146

Closed
wants to merge 1 commit into from
Closed

perf(forms): make built-in ControlValueAccessors more tree-shakable #41146

wants to merge 1 commit into from

Conversation

AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Mar 10, 2021 •
edited

This commit updates Forms code to avoid direct references to all built-in ControlValueAccessor classes, which
prevents their tree-shaking from production builds. Instead, a new static property is added to all built-in
ControlValueAccessors, which is checked when we need to identify whether a given ControlValueAccessors is a
built-in one.

This PR partially implements #41011.

PR Type

What kind of change does this PR introduce?

  • Other... Please describe: performance-related change.

Does this PR introduce a breaking change?

  • Yes
  • No

@google-cla google-cla bot added the cla: yes label Mar 10, 2021
@ngbot ngbot bot modified the milestone: Backlog Mar 10, 2021
@AndrewKushnir
Copy link
Contributor Author

Presubmit.

@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Mar 11, 2021
@AndrewKushnir AndrewKushnir marked this pull request as ready for review March 11, 2021 01:06
@AndrewKushnir AndrewKushnir mentioned this pull request Mar 11, 2021
Closed
3 tasks
petebacondarwin
petebacondarwin reviewed Mar 11, 2021
View reviewed changes
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

An alternative approach would be for all built-in CVAs to extend an internal abstract class. Then you could check for a built-in CVA using instanceof. I feel that this would be a more clean approach than a semi-hidden internal property.

packages/forms/src/directives/checkbox_value_accessor.ts Outdated Show resolved Hide resolved
@AndrewKushnir
Copy link
Contributor Author

Thanks for the review @petebacondarwin 👍 As we discussed, I've updated the code to check whether a given CVA is an instance of a class that directly extends BuiltInControlValueAccessor class. The solution is much more elegant, thanks for the proposal Pete!

jessicajaniuk
jessicajaniuk approved these changes Mar 11, 2021
View reviewed changes
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment •
edited

Choose a reason for hiding this comment

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

LGTM! 🍪

Really simple and huge size savings! Great work!

reviewed-for: public-api
reviewed-for: size-tracking

goldens/size-tracking/integration-payloads.json
@@ -59,7 +59,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 1485,
"main-es2015": 168534,
"main-es2015": 162346,
Copy link
Contributor

Choose a reason for hiding this comment

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

👏🏻

@pullapprove pullapprove bot requested a review from atscott March 11, 2021 23:35
@jessicajaniuk
Copy link
Contributor

Presubmit

@AndrewKushnir
Copy link
Contributor Author

Also started a Global Presubmit to verify that there are no edge cases that might be affected.

@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Mar 12, 2021
@AndrewKushnir AndrewKushnir mentioned this pull request Mar 12, 2021
Closed
petebacondarwin
petebacondarwin approved these changes Mar 12, 2021
View reviewed changes
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

LGTM - one suggestion about the docs.

packages/forms/src/directives/control_value_accessor.ts Show resolved Hide resolved
packages/forms/src/directives/control_value_accessor.ts
@@ -139,4 +148,4 @@ export interface ControlValueAccessor {
* @publicApi
*/
export const NG_VALUE_ACCESSOR =
new InjectionToken<ReadonlyArray<ControlValueAccessor>>('NgValueAccessor');
new InjectionToken<ReadonlyArray<ControlValueAccessor>>('NgValueAccessor');
Copy link
Member

Choose a reason for hiding this comment

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

😱

packages/forms/src/directives/shared.ts
return BUILTIN_ACCESSORS.some(a => valueAccessor.constructor === a);
// Check if a given value accessor is an instance of a class that directly extends
// `BuiltInControlValueAccessor` one.
return Object.getPrototypeOf(valueAccessor.constructor) === BuiltInControlValueAccessor;
Copy link
Member

Choose a reason for hiding this comment

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

🎉

This was referenced Mar 18, 2021
Closed
Closed
Closed
Closed
Merged
Merged
Closed
Closed
Closed
Closed
Merged
Closed
Closed
Closed
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@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 Apr 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@petebacondarwin petebacondarwin petebacondarwin approved these changes

@jelbourn jelbourn jelbourn approved these changes

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker aio: preview area: forms cla: yes cross-cutting: tree-shaking target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@AndrewKushnir @jessicajaniuk @mary-poppins @petebacondarwin @jelbourn