From cfe5fadb3aeedef74ab7f3b1b4300e4566fdf2e9 Mon Sep 17 00:00:00 2001 From: Doug Parker Date: Fri, 17 Jul 2020 16:23:21 -0700 Subject: [PATCH] fix(compiler): mark `NgModuleFactory` construction as not side effectful (#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 --- .../compiler-cli/src/ngtsc/imports/src/core.ts | 1 + .../src/ngtsc/shims/src/factory_generator.ts | 3 ++- .../compiler-cli/test/ngtsc/fake_core/index.ts | 2 ++ packages/compiler-cli/test/ngtsc/ngtsc_spec.ts | 17 +++++++++++------ .../core/src/core_render3_private_export.ts | 1 + packages/core/src/r3_symbols.ts | 1 + 6 files changed, 18 insertions(+), 7 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/imports/src/core.ts b/packages/compiler-cli/src/ngtsc/imports/src/core.ts index 122b399829..eae858337b 100644 --- a/packages/compiler-cli/src/ngtsc/imports/src/core.ts +++ b/packages/compiler-cli/src/ngtsc/imports/src/core.ts @@ -65,6 +65,7 @@ const CORE_SUPPORTED_SYMBOLS = new Map([ ['ɵɵInjectorDef', 'ɵɵInjectorDef'], ['ɵɵNgModuleDefWithMeta', 'ɵɵNgModuleDefWithMeta'], ['ɵNgModuleFactory', 'NgModuleFactory'], + ['ɵnoSideEffects', 'ɵnoSideEffects'], ]); const CORE_MODULE = '@angular/core'; diff --git a/packages/compiler-cli/src/ngtsc/shims/src/factory_generator.ts b/packages/compiler-cli/src/ngtsc/shims/src/factory_generator.ts index 8e54e282a5..08f633f056 100644 --- a/packages/compiler-cli/src/ngtsc/shims/src/factory_generator.ts +++ b/packages/compiler-cli/src/ngtsc/shims/src/factory_generator.ts @@ -63,7 +63,8 @@ export class FactoryGenerator implements PerFileShimGenerator, FactoryTracker { // because it won't miss any that do. const varLines = symbolNames.map( name => `export const ${ - name}NgFactory: i0.ɵNgModuleFactory = new i0.ɵNgModuleFactory(${name});`); + name}NgFactory: i0.ɵNgModuleFactory = i0.ɵnoSideEffects(() => new i0.ɵNgModuleFactory(${ + name}));`); sourceText += [ // This might be incorrect if the current package being compiled is Angular core, but it's // okay to leave in at type checking time. TypeScript can handle this reference via its path diff --git a/packages/compiler-cli/test/ngtsc/fake_core/index.ts b/packages/compiler-cli/test/ngtsc/fake_core/index.ts index 7aad1050cf..bcbb9b0261 100644 --- a/packages/compiler-cli/test/ngtsc/fake_core/index.ts +++ b/packages/compiler-cli/test/ngtsc/fake_core/index.ts @@ -103,3 +103,5 @@ export interface QueryList/* implements Iterable */ { export type NgIterable = Array|Iterable; export class NgZone {} + +export declare function ɵnoSideEffects(fn: () => T): T; diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 6e6848a80d..7af46bfd7c 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -3538,7 +3538,9 @@ runInEachFileSystem(os => { expect(factoryContents).toContain(`import * as i0 from '@angular/core';`); expect(factoryContents).toContain(`import { NotAModule, TestModule } from './test';`); expect(factoryContents) - .toContain(`export var TestModuleNgFactory = new i0.\u0275NgModuleFactory(TestModule);`); + .toContain( + 'export var TestModuleNgFactory = ' + + 'i0.ɵnoSideEffects(function () { return new i0.\u0275NgModuleFactory(TestModule); });'); expect(factoryContents).not.toContain(`NotAModuleNgFactory`); expect(factoryContents).not.toContain('\u0275NonEmptyModule'); @@ -3677,11 +3679,14 @@ runInEachFileSystem(os => { env.driveMain(); const factoryContents = env.getContents('test.ngfactory.js'); - expect(normalize(factoryContents)).toBe(normalize(` - import * as i0 from "./r3_symbols"; - import { TestModule } from './test'; - export var TestModuleNgFactory = new i0.NgModuleFactory(TestModule); - `)); + expect(normalize(factoryContents)) + .toBe( + 'import * as i0 from "./r3_symbols"; ' + + 'import { TestModule } from \'./test\'; ' + + 'export var TestModuleNgFactory = ' + + 'i0.\u0275noSideEffects(function () { ' + + 'return new i0.NgModuleFactory(TestModule); ' + + '});'); }); describe('file-level comments', () => { diff --git a/packages/core/src/core_render3_private_export.ts b/packages/core/src/core_render3_private_export.ts index d20a6449ac..01f844c351 100644 --- a/packages/core/src/core_render3_private_export.ts +++ b/packages/core/src/core_render3_private_export.ts @@ -292,5 +292,6 @@ export { ɵɵsanitizeUrl, ɵɵsanitizeUrlOrResourceUrl, } from './sanitization/sanitization'; +export {noSideEffects as ɵnoSideEffects} from './util/closure'; // clang-format on diff --git a/packages/core/src/r3_symbols.ts b/packages/core/src/r3_symbols.ts index 391bb8bf56..1ee3762e83 100644 --- a/packages/core/src/r3_symbols.ts +++ b/packages/core/src/r3_symbols.ts @@ -28,6 +28,7 @@ export {ɵɵdefineNgModule} from './render3/definition'; export {ɵɵFactoryDef} from './render3/interfaces/definition'; export {setClassMetadata} from './render3/metadata'; export {NgModuleFactory} from './render3/ng_module_ref'; +export {noSideEffects as ɵnoSideEffects} from './util/closure';