components icon indicating copy to clipboard operation
components copied to clipboard

Enable tree-shaking of more services

Open ranma42 opened this issue 4 years ago • 7 comments

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 https://github.com/angular/components/issues/11581 and https://github.com/angular/components/issues/18944

ranma42 avatar Mar 31 '20 23:03 ranma42

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)

PierreDuc avatar Apr 01 '20 09:04 PierreDuc

@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).

ranma42 avatar Apr 01 '20 17:04 ranma42

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

crisbeto avatar Apr 05 '20 13:04 crisbeto

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?

ranma42 avatar Apr 05 '20 13:04 ranma42

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

crisbeto avatar Apr 05 '20 15:04 crisbeto

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 https://github.com/angular/components/pull/23969 , but I did not find an explicit mention for it in the changelog)

ranma42 avatar Dec 01 '21 08:12 ranma42

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.

crisbeto avatar Dec 01 '21 09:12 crisbeto

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?

csisy avatar Aug 01 '23 10:08 csisy

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

ranma42 avatar Aug 02 '23 13:08 ranma42

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

ranma42 avatar Aug 02 '23 13:08 ranma42

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

crisbeto avatar Feb 28 '24 08:02 crisbeto

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.