Skip to content

Enable tree-shaking of more services #18962

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

Closed
wants to merge 5 commits into from
Closed

Conversation

ranma42
Copy link

@ranma42 ranma42 commented Mar 31, 2020

This branch is an attempt to improve tree-shaking of several services.

I started by removing the services that are providedIn: 'root' from the providers of any module, as that should not be needed and actually prevents them from being removed by tree-shaking.

I am investigating whether the remaining services (such as MatDialog and Overlay) need to be provided explicitly or if they can also become tree-shakable.

This branch aims at fixing #11581 and #18944

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 31, 2020
@ranma42 ranma42 changed the title Enable tree-shaking of services Enable tree-shaking of more services Mar 31, 2020
@PierreDuc
Copy link

PierreDuc commented Apr 1, 2020

Be aware that mixing providedIn: 'root' and declaring them in modules will actually cause two (or more) instances of the service to be instantiated, so they are no longer acting as singletons. Maybe some code is relying on this (I sure hope not though)

@ranma42
Copy link
Author

ranma42 commented Apr 1, 2020

@PierreDuc this branch is actively avoiding the issue you describe by removing the modules that are already providedIn: 'root' from NgModule.providers.

The build failure seems to be related to the testing which on CI is currently targeting the VE (while IIUC yarn test runs on Ivy).

@crisbeto
Copy link
Member

crisbeto commented Apr 5, 2020

The ViewEngine failures look legitimate. FYI, you can run tests locally against ViewEngine by passing in --config=view-engine.

@ranma42
Copy link
Author

ranma42 commented Apr 5, 2020

What is the current plan regarding ViewEngine and Ivy?
Is angular/components going to move to Ivy (soonish)?
If these providers are being kept for compatibility with ViewEngine, is there any place tracking them (and possibly their impact on the build output size)?

As a side note, the documentation does not mention --config=view-engine, so I was assuming that yarn test would suffice. Should that be updated?

@crisbeto
Copy link
Member

crisbeto commented Apr 5, 2020

We support both Ivy and ViewEngine and we'll continue to do so as long as people are able to switch back to ViewEngine.

@ranma42
Copy link
Author

ranma42 commented Dec 1, 2021

does the current version still support Ivy? if not, I would like to work again on this
(my guess is that it is not supported anymore, since I saw #23969 , but I did not find an explicit mention for it in the changelog)

@crisbeto
Copy link
Member

crisbeto commented Dec 1, 2021

We don't support ViewEngine as of version 13. Note that this PR has been pushed very far down in the queue so you might be better off opening a new PR.

@ranma42 ranma42 force-pushed the tree-shake branch 2 times, most recently from a0ef7d4 to c6bec19 Compare December 4, 2021 14:40
@ranma42 ranma42 marked this pull request as ready for review December 4, 2021 15:06
@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 28, 2021
@josephperrott josephperrott added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed needs rebase labels Nov 16, 2022
@csisy
Copy link

csisy commented Aug 1, 2023

It looks like this PR has been forgotten, but I think this is important and is a good step in a direction to make cdk/material more tree-shaking friendly. Is there any plan to merge this PR?

ranma42 added 5 commits August 2, 2023 14:56
In order to be tree-shakable, services must be marked as `providedIn`
and they must not be explicitly referenced in the `providers` of an
`NgModule`.
In order to be tree-shakable, services must be marked as `providedIn`
and they must not be explicitly referenced in the `providers` of an
`NgModule`.
In order to be tree-shakable, services must be marked as `providedIn`
and they must not be explicitly referenced in the `providers` of an
`NgModule`.
In order to be tree-shakable, services must be marked as `providedIn`
and they must not be explicitly referenced in the `providers` of an
`NgModule`.
@ranma42
Copy link
Author

ranma42 commented Aug 2, 2023

I rebased the branch on top of the current main branch; I also added a commit to enable the tree-shaking of Overlay

@ranma42
Copy link
Author

ranma42 commented Aug 2, 2023

(note that eventually, as we move away from modules thanks to standalone components, tree-shaking issues such as this one might become irrelevant)

@crisbeto
Copy link
Member

Closing since all of the relevant services are providedIn: 'root' now after the switch to standalone.

@crisbeto crisbeto closed this Feb 28, 2024
@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 Mar 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants