fix(compiler): mark `NgModuleFactory` construction as not side effectful #38147

dgp1130 · 2020-07-20T19:49:11Z

This allows Closure compiler to tree shake unused constructor calls to NgModuleFactory, which is otherwise considered side-effectful. The Angular compiler generates factory objects which are exported but typically not used, as they are only needed for compatibility with View Engine. This results in top-level constructor calls, such as:

export const FooNgFactory = new NgModuleFactory(Foo);

NgModuleFactory has a side-effecting constructor, so this statement cannot be tree shaken, even if FooNgFactory is never imported. The NgModuleFactory continues to reference its associated NgModule and prevents the module and all its unused dependencies from being tree shaken. This effectively prevents all components from being tree shaken, making Closure builds significantly larger than they should be.

The fix here is to wrap NgModuleFactory constructor with noSideEffects(() => /* ... */), which tricks the Closure compiler into assuming that the invoked function has no side effects. This allows it to tree-shake unused NgModuleFactory() constructors when they aren't imported. Since the factory can be removed, the module can also be removed (if nothing else references it), thus tree shaking unused components as expected.

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: http://b/161548609

Unused components are not tree shaken in Closure compiler.

What is the new behavior?

Unused components are now tree shaken in Closure compiler.

Does this PR introduce a breaking change?

Yes
No

Other information

dgp1130 · 2020-07-21T22:45:01Z

http://test/OCL:322443590:BASE:322443701:1595367128330:be18d491

dgp1130 · 2020-07-24T17:18:45Z

@mhevery, @alxhub, can you please take a look at this PR?

dgp1130 · 2020-07-27T20:27:29Z

Status checks wanted me to rerun g3 presubmit, so here's the new results:
http://test/OCL:322443590:BASE:323406497:1595875409766:177c87aa

Just a number of flakes/preexisting failures. I don't see anything that seems related to this PR.

alxhub

This LGTM, save for the formatting of ngtsc_spec.

packages/compiler-cli/test/ngtsc/fake_core/index.ts

packages/compiler-cli/test/ngtsc/ngtsc_spec.ts

This allows Closure compiler to tree shake unused constructor calls to `NgModuleFactory`, which is otherwise considered side-effectful. The Angular compiler generates factory objects which are exported but typically not used, as they are only needed for compatibility with View Engine. This results in top-level constructor calls, such as: ```typescript export const FooNgFactory = new NgModuleFactory(Foo); ``` `NgModuleFactory` has a side-effecting constructor, so this statement cannot be tree shaken, even if `FooNgFactory` is never imported. The `NgModuleFactory` continues to reference its associated `NgModule` and prevents the module and all its unused dependencies from being tree shaken. This effectively prevents all components from being tree shaken, making Closure builds significantly larger than they should be. The fix here is to wrap `NgModuleFactory` constructor with `noSideEffects(() => /* ... */)`, which tricks the Closure compiler into assuming that the invoked function has no side effects. This allows it to tree-shake unused `NgModuleFactory()` constructors when they aren't imported. Since the factory can be removed, the module can also be removed (if nothing else references it), thus tree shaking unused components as expected.

…ful (#38147) This allows Closure compiler to tree shake unused constructor calls to `NgModuleFactory`, which is otherwise considered side-effectful. The Angular compiler generates factory objects which are exported but typically not used, as they are only needed for compatibility with View Engine. This results in top-level constructor calls, such as: ```typescript export const FooNgFactory = new NgModuleFactory(Foo); ``` `NgModuleFactory` has a side-effecting constructor, so this statement cannot be tree shaken, even if `FooNgFactory` is never imported. The `NgModuleFactory` continues to reference its associated `NgModule` and prevents the module and all its unused dependencies from being tree shaken. This effectively prevents all components from being tree shaken, making Closure builds significantly larger than they should be. The fix here is to wrap `NgModuleFactory` constructor with `noSideEffects(() => /* ... */)`, which tricks the Closure compiler into assuming that the invoked function has no side effects. This allows it to tree-shake unused `NgModuleFactory()` constructors when they aren't imported. Since the factory can be removed, the module can also be removed (if nothing else references it), thus tree shaking unused components as expected. PR Close #38147

…e effectful (angular#38147)" This reverts commit 7f8c222. This commit caused test failures internally, which were traced back to the optimizer removing NgModuleFactory constructor calls when those calls caused side-effectful registration of NgModules by their ids.

…e effectful (#38147)" (#38303) This reverts commit 7f8c222. This commit caused test failures internally, which were traced back to the optimizer removing NgModuleFactory constructor calls when those calls caused side-effectful registration of NgModules by their ids. PR Close #38303

…ide effectful Roll forward of angular#38147. This allows Closure compiler to tree shake unused constructor calls to `NgModuleFactory`, which is otherwise considered side-effectful. The Angular compiler generates factory objects which are exported but typically not used, as they are only needed for compatibility with View Engine. This results in top-level constructor calls, such as: ```typescript export const FooNgFactory = new NgModuleFactory(Foo); ``` `NgModuleFactory` has a side-effecting constructor, so this statement cannot be tree shaken, even if `FooNgFactory` is never imported. The `NgModuleFactory` continues to reference its associated `NgModule` and prevents the module and all its unused dependencies from being tree shaken, making Closure builds significantly larger than necessary. The fix here is to wrap `NgModuleFactory` constructor with `noSideEffects(() => /* ... */)`, which tricks the Closure compiler into assuming that the invoked function has no side effects. This allows it to tree-shake unused `NgModuleFactory()` constructors when they aren't imported. Since the factory can be removed, the module can also be removed (if nothing else references it), thus tree shaking unused dependencies as expected. The one notable edge case is for lazy loaded modules. Internally, lazy loading is done as a side effect when the lazy script is evaluated. For Angular, this side effect is registering the `NgModule`. In Ivy this is done by the `NgModuleFactory` constructor, so lazy loaded modules **cannot** have their top-level `NgModuleFactory` constructor call tree shaken. We handle this case by looking for the `id` field on `@NgModule` annotations. All lazy loaded modules include an `id`. When this `id` is found, the `NgModuleFactory` is generated **without** with `noSideEffects()` call, so Closure will not tree shake it and the module will lazy-load correctly.

…ide effectful (#38320) Roll forward of #38147. This allows Closure compiler to tree shake unused constructor calls to `NgModuleFactory`, which is otherwise considered side-effectful. The Angular compiler generates factory objects which are exported but typically not used, as they are only needed for compatibility with View Engine. This results in top-level constructor calls, such as: ```typescript export const FooNgFactory = new NgModuleFactory(Foo); ``` `NgModuleFactory` has a side-effecting constructor, so this statement cannot be tree shaken, even if `FooNgFactory` is never imported. The `NgModuleFactory` continues to reference its associated `NgModule` and prevents the module and all its unused dependencies from being tree shaken, making Closure builds significantly larger than necessary. The fix here is to wrap `NgModuleFactory` constructor with `noSideEffects(() => /* ... */)`, which tricks the Closure compiler into assuming that the invoked function has no side effects. This allows it to tree-shake unused `NgModuleFactory()` constructors when they aren't imported. Since the factory can be removed, the module can also be removed (if nothing else references it), thus tree shaking unused dependencies as expected. The one notable edge case is for lazy loaded modules. Internally, lazy loading is done as a side effect when the lazy script is evaluated. For Angular, this side effect is registering the `NgModule`. In Ivy this is done by the `NgModuleFactory` constructor, so lazy loaded modules **cannot** have their top-level `NgModuleFactory` constructor call tree shaken. We handle this case by looking for the `id` field on `@NgModule` annotations. All lazy loaded modules include an `id`. When this `id` is found, the `NgModuleFactory` is generated **without** with `noSideEffects()` call, so Closure will not tree shake it and the module will lazy-load correctly. PR Close #38320

…ful (angular#38147) This allows Closure compiler to tree shake unused constructor calls to `NgModuleFactory`, which is otherwise considered side-effectful. The Angular compiler generates factory objects which are exported but typically not used, as they are only needed for compatibility with View Engine. This results in top-level constructor calls, such as: ```typescript export const FooNgFactory = new NgModuleFactory(Foo); ``` `NgModuleFactory` has a side-effecting constructor, so this statement cannot be tree shaken, even if `FooNgFactory` is never imported. The `NgModuleFactory` continues to reference its associated `NgModule` and prevents the module and all its unused dependencies from being tree shaken. This effectively prevents all components from being tree shaken, making Closure builds significantly larger than they should be. The fix here is to wrap `NgModuleFactory` constructor with `noSideEffects(() => /* ... */)`, which tricks the Closure compiler into assuming that the invoked function has no side effects. This allows it to tree-shake unused `NgModuleFactory()` constructors when they aren't imported. Since the factory can be removed, the module can also be removed (if nothing else references it), thus tree shaking unused components as expected. PR Close angular#38147

…e effectful (angular#38147)" (angular#38303) This reverts commit 7f8c222. This commit caused test failures internally, which were traced back to the optimizer removing NgModuleFactory constructor calls when those calls caused side-effectful registration of NgModules by their ids. PR Close angular#38303

angular-automatic-lock-bot · 2020-08-30T16:07:40Z

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.}

…ful (angular#38147) This allows Closure compiler to tree shake unused constructor calls to `NgModuleFactory`, which is otherwise considered side-effectful. The Angular compiler generates factory objects which are exported but typically not used, as they are only needed for compatibility with View Engine. This results in top-level constructor calls, such as: ```typescript export const FooNgFactory = new NgModuleFactory(Foo); ``` `NgModuleFactory` has a side-effecting constructor, so this statement cannot be tree shaken, even if `FooNgFactory` is never imported. The `NgModuleFactory` continues to reference its associated `NgModule` and prevents the module and all its unused dependencies from being tree shaken. This effectively prevents all components from being tree shaken, making Closure builds significantly larger than they should be. The fix here is to wrap `NgModuleFactory` constructor with `noSideEffects(() => /* ... */)`, which tricks the Closure compiler into assuming that the invoked function has no side effects. This allows it to tree-shake unused `NgModuleFactory()` constructors when they aren't imported. Since the factory can be removed, the module can also be removed (if nothing else references it), thus tree shaking unused components as expected. PR Close angular#38147

googlebot added the cla: yes label Jul 20, 2020

dgp1130 force-pushed the ng-module-factory branch 2 times, most recently from b90a95f to fce32b0 Compare July 20, 2020 23:14

dgp1130 requested review from alxhub and mhevery July 21, 2020 02:01

dgp1130 added area: compiler Issues related to `ngc`, Angular's template compiler target: patch This PR is targeted for the next patch release labels Jul 21, 2020

ngbot bot added this to the needsTriage milestone Jul 21, 2020

dgp1130 marked this pull request as ready for review July 21, 2020 02:02

dgp1130 force-pushed the ng-module-factory branch from fce32b0 to 724f7c0 Compare July 21, 2020 02:12

mhevery approved these changes Jul 24, 2020

View reviewed changes

dgp1130 force-pushed the ng-module-factory branch 3 times, most recently from 5c53f7b to 4609e6c Compare July 27, 2020 19:06

alxhub requested changes Jul 27, 2020

View reviewed changes

packages/compiler-cli/test/ngtsc/fake_core/index.ts Outdated Show resolved Hide resolved

packages/compiler-cli/test/ngtsc/ngtsc_spec.ts Outdated Show resolved Hide resolved

dgp1130 force-pushed the ng-module-factory branch 4 times, most recently from d63985d to cc3631a Compare July 27, 2020 21:12

dgp1130 force-pushed the ng-module-factory branch from cc3631a to 1ca48d8 Compare July 27, 2020 21:24

dgp1130 requested a review from alxhub July 27, 2020 23:16

alxhub approved these changes Jul 29, 2020

View reviewed changes

dgp1130 added the action: merge The PR is ready for merge by the caretaker label Jul 29, 2020

alxhub closed this in 7f8c222 Jul 29, 2020

dgp1130 deleted the ng-module-factory branch July 30, 2020 23:47

dgp1130 mentioned this pull request Aug 1, 2020

Mark eager NgModuleFactory construction as not side effectful #38320

Closed

5 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(compiler): mark `NgModuleFactory` construction as not side effectful #38147

fix(compiler): mark `NgModuleFactory` construction as not side effectful #38147

Uh oh!

dgp1130 commented Jul 20, 2020 •

edited

Loading

Uh oh!

dgp1130 commented Jul 21, 2020

Uh oh!

dgp1130 commented Jul 24, 2020

Uh oh!

dgp1130 commented Jul 27, 2020

Uh oh!

alxhub left a comment

Uh oh!

Uh oh!

Uh oh!

angular-automatic-lock-bot bot commented Aug 30, 2020

Uh oh!

Uh oh!

fix(compiler): mark NgModuleFactory construction as not side effectful #38147

fix(compiler): mark NgModuleFactory construction as not side effectful #38147

Uh oh!

Conversation

dgp1130 commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

dgp1130 commented Jul 21, 2020

Uh oh!

dgp1130 commented Jul 24, 2020

Uh oh!

dgp1130 commented Jul 27, 2020

Uh oh!

alxhub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

angular-automatic-lock-bot bot commented Aug 30, 2020

Uh oh!

Uh oh!

fix(compiler): mark `NgModuleFactory` construction as not side effectful #38147

fix(compiler): mark `NgModuleFactory` construction as not side effectful #38147

dgp1130 commented Jul 20, 2020 •

edited

Loading