fix(compiler): mark NgModuleFactory construction as not side effectful
#38147
Conversation
b90a95f
to
fce32b0
mhevery
approved these changes
Jul 24, 2020
5c53f7b
to
4609e6c
|
Status checks wanted me to rerun g3 presubmit, so here's the new results: Just a number of flakes/preexisting failures. I don't see anything that seems related to this PR. |
alxhub
requested changes
Jul 27, 2020
This LGTM, save for the formatting of ngtsc_spec.
d63985d
to
cc3631a
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.
alxhub
approved these changes
Jul 29, 2020
alxhub
added a commit
that referenced
this issue
Jul 29, 2020
…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
alxhub
added a commit
to alxhub/angular
that referenced
this issue
Jul 30, 2020
…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.
dgp1130
added a commit
to dgp1130/angular
that referenced
this issue
Aug 1, 2020
…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.
dgp1130
added a commit
to dgp1130/angular
that referenced
this issue
Aug 1, 2020
…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.
dgp1130
added a commit
to dgp1130/angular
that referenced
this issue
Aug 1, 2020
…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.
5 tasks
dgp1130
added a commit
to dgp1130/angular
that referenced
this issue
Aug 5, 2020
…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.
dgp1130
added a commit
to dgp1130/angular
that referenced
this issue
Aug 6, 2020
…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.
AndrewKushnir
added a commit
that referenced
this issue
Aug 6, 2020
…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
AndrewKushnir
added a commit
that referenced
this issue
Aug 6, 2020
…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
Splaktar
added a commit
to angular-hispano/angular
that referenced
this issue
Aug 8, 2020
…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
Splaktar
added a commit
to angular-hispano/angular
that referenced
this issue
Aug 8, 2020
…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
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
profanis
added a commit
to profanis/angular
that referenced
this issue
Sep 5, 2020
…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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.


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:NgModuleFactoryhas a side-effecting constructor, so this statement cannot be tree shaken, even ifFooNgFactoryis never imported. TheNgModuleFactorycontinues to reference its associatedNgModuleand 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
NgModuleFactoryconstructor withnoSideEffects(() => /* ... */), which tricks the Closure compiler into assuming that the invoked function has no side effects. This allows it to tree-shake unusedNgModuleFactory()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:
PR Type
What kind of change does this PR introduce?
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?
Other information
The text was updated successfully, but these errors were encountered: