From dca4443a8ed65d341356fe62f6df9934e6314dfe Mon Sep 17 00:00:00 2001 From: Doug Parker Date: Fri, 31 Jul 2020 17:42:21 -0700 Subject: [PATCH] fix(compiler-cli): mark eager `NgModuleFactory` construction as not side 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 --- .../src/ngtsc/annotations/src/ng_module.ts | 2 +- .../src/ngtsc/shims/src/factory_generator.ts | 79 ++++++++++++++++++- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 35 ++++++-- 3 files changed, 107 insertions(+), 9 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts index e83ced30ad..115a9e7361 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts @@ -309,7 +309,7 @@ export class NgModuleDecoratorHandler implements if (this.factoryTracker !== null) { this.factoryTracker.track(node.getSourceFile(), { name: analysis.factorySymbolName, - hasId: !!analysis.id, + hasId: analysis.id !== null, }); } 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 f9dcf6de28..4ee78ec513 100644 --- a/packages/compiler-cli/src/ngtsc/shims/src/factory_generator.ts +++ b/packages/compiler-cli/src/ngtsc/shims/src/factory_generator.ts @@ -186,8 +186,24 @@ function transformFactorySourceFile( // Otherwise, check if this export is a factory for a known NgModule, and retain it if so. const match = STRIP_NG_FACTORY.exec(decl.name.text); - if (match !== null && moduleSymbols.has(match[1])) { - transformedStatements.push(stmt); + const module = match ? moduleSymbols.get(match[1]) : null; + if (module) { + // If the module can be tree shaken, then the factory should be wrapped in a + // `noSideEffects()` call which tells Closure to treat the expression as pure, allowing + // it to be removed if the result is not used. + // + // `NgModule`s with an `id` property will be lazy loaded. Google-internal lazy loading + // infra relies on a side effect from the `new NgModuleFactory()` call, which registers + // the module globally. Because of this, we **cannot** tree shake any module which has + // an `id` property. Doing so would cause lazy loaded modules to never be registered. + const moduleIsTreeShakable = !module.hasId; + const newStmt = !moduleIsTreeShakable ? + stmt : + updateInitializers( + stmt, + (init) => init ? wrapInNoSideEffects(init) : undefined, + ); + transformedStatements.push(newStmt); } } else { // Leave the statement alone, as it can't be understood. @@ -266,3 +282,62 @@ function getFileoverviewComment(sourceFile: ts.SourceFile): string|null { return commentText; } + +/** + * Wraps the given expression in a call to `ɵnoSideEffects()`, which tells + * Closure we don't care about the side effects of this expression and it should + * be treated as "pure". Closure is free to tree shake this expression if its + * result is not used. + * + * Example: Takes `1 + 2` and returns `i0.ɵnoSideEffects(() => 1 + 2)`. + */ +function wrapInNoSideEffects(expr: ts.Expression): ts.Expression { + const noSideEffects = ts.createPropertyAccess( + ts.createIdentifier('i0'), + 'ɵnoSideEffects', + ); + + return ts.createCall( + noSideEffects, + /* typeArguments */[], + /* arguments */ + [ + ts.createFunctionExpression( + /* modifiers */[], + /* asteriskToken */ undefined, + /* name */ undefined, + /* typeParameters */[], + /* parameters */[], + /* type */ undefined, + /* body */ ts.createBlock([ + ts.createReturn(expr), + ]), + ), + ], + ); +} + +/** + * Clones and updates the initializers for a given statement to use the new + * expression provided. Does not mutate the input statement. + */ +function updateInitializers( + stmt: ts.VariableStatement, + update: (initializer?: ts.Expression) => ts.Expression | undefined, + ): ts.VariableStatement { + return ts.updateVariableStatement( + stmt, + stmt.modifiers, + ts.updateVariableDeclarationList( + stmt.declarationList, + stmt.declarationList.declarations.map( + (decl) => ts.updateVariableDeclaration( + decl, + decl.name, + decl.type, + update(decl.initializer), + ), + ), + ), + ); +} diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 886e364fbc..5ae96b6e5c 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.\u0275noSideEffects(function () { ' + + 'return new i0.\u0275NgModuleFactory(TestModule); });'); expect(factoryContents).not.toContain(`NotAModuleNgFactory`); expect(factoryContents).not.toContain('\u0275NonEmptyModule'); @@ -3677,11 +3679,32 @@ 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(factoryContents) + .toBe( + 'import * as i0 from "./r3_symbols";\n' + + 'import { TestModule } from \'./test\';\n' + + 'export var TestModuleNgFactory = i0.\u0275noSideEffects(function () {' + + ' return new i0.NgModuleFactory(TestModule); });\n'); + }); + + it('should generate side effectful NgModuleFactory constructor when lazy loaded', () => { + env.tsconfig({'allowEmptyCodegenFiles': true}); + + env.write('test.ts', ` + import {NgModule} from '@angular/core'; + + @NgModule({ + id: 'test', // ID to use for lazy loading. + }) + export class TestModule {} + `); + + env.driveMain(); + + // Should **not** contain noSideEffects(), because the module is lazy loaded. + const factoryContents = env.getContents('test.ngfactory.js'); + expect(factoryContents) + .toContain('export var TestModuleNgFactory = new i0.ɵNgModuleFactory(TestModule);'); }); describe('file-level comments', () => {