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 FormBuilder and RadioControlRegistry tree-shakable #41126

Closed
wants to merge 2 commits into from
Closed

perf(forms): make FormBuilder and RadioControlRegistry tree-shakable #41126

wants to merge 2 commits into from

Conversation

AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Mar 9, 2021 •
edited

This PR makes the FormBuilder and RadioControlRegistry classes tree-shakable by adding the providedIn property to corresponding @Injectable decorators and excluding these classes from NgModule definitions.

Note: this improvement is a part of #41011.

PR Type

What kind of change does this PR introduce?

  • Other... Please describe: performance optimization.

Does this PR introduce a breaking change?

  • Yes
  • No

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

Presubmit.

@mary-poppins
Copy link

You can preview 5482ea3 at https://pr41126-5482ea3.ngbuilds.io/.
You can preview e19a9dd at https://pr41126-e19a9dd.ngbuilds.io/.

@AndrewKushnir AndrewKushnir changed the title perf(forms): make FormBuilder class tree-shakable perf(forms): make FormBuilder and RadioControlRegistry tree-shakable Mar 9, 2021
@AndrewKushnir AndrewKushnir changed the title perf(forms): make FormBuilder and RadioControlRegistry tree-shakable perf(forms): make FormBuilder and RadioControlRegistry tree-shakable Mar 9, 2021
@mary-poppins
Copy link

You can preview ef68a26 at https://pr41126-ef68a26.ngbuilds.io/.

@mary-poppins
Copy link

You can preview acb48ff at https://pr41126-acb48ff.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 419bbe6 at https://pr41126-419bbe6.ngbuilds.io/.

@AndrewKushnir
Copy link
Contributor Author

Note: this change should land after #41146.

@AndrewKushnir AndrewKushnir mentioned this pull request Mar 12, 2021
Closed
@AndrewKushnir
perf(forms): make FormBuilder class tree-shakable
6442764 
This commit makes the `FormBuilder` class tree-shakable by adding the `providedIn` property to its `@Injectable`
decorator. Now if the `FormBuilder` class is not referenced in application's code, it should not be included into
its production bundle.
@mary-poppins
Copy link

You can preview d35d95a at https://pr41126-d35d95a.ngbuilds.io/.

@AndrewKushnir
perf(forms): make RadioControlRegistry class tree-shakable
d0d2d59 
This commit makes the `RadioControlRegistry` class tree-shakable by adding the `providedIn` property to its
`@Injectable` decorator. Now if the radio buttons are not used in the app (thus no `RadioControlValueAccessor`
directive is initialized), the `RadioControlRegistry` should not be included into application's prod bundle.
@mary-poppins
Copy link

You can preview d0d2d59 at https://pr41126-d0d2d59.ngbuilds.io/.

@AndrewKushnir
Copy link
Contributor Author

Presubmit #2 + Global Presubmit.

AndrewKushnir
AndrewKushnir commented Mar 14, 2021
View reviewed changes
packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json
@@ -450,7 +450,7 @@
"name": "R3ViewContainerRef"
},
{
"name": "RadioControlRegistry"
"name": "RadioControlRegistryModule"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to reviewers: this change indicates that the RadioControlRegistry was tree-shaken away, but the RadioControlRegistryModule shows up here as it's used as the providedIn value (see below). Note: the FormBuilder class is not showing up here since it's used in that demo app (forms_reactive) and the forms_template_driven golden file never contained it. I've verified that both symbols are not present in a simple app that don't use radio buttons and doesn't reference FormsBuilder directly.

@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Mar 14, 2021
@AndrewKushnir AndrewKushnir marked this pull request as ready for review March 14, 2021 02:39
This was referenced Mar 18, 2021
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
@satanTime satanTime mentioned this pull request Apr 11, 2021
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 16, 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

@atscott atscott atscott 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: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@AndrewKushnir @mary-poppins @petebacondarwin @atscott @jelbourn @jessicajaniuk