From b54ed980edb48bbb7f48d0dc3e7611335071c733 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Thu, 7 Nov 2019 14:18:44 -0800 Subject: [PATCH] fix(ivy): retain JIT metadata unless JIT mode is explicitly disabled (#33671) NgModules in Ivy have a definition which contains various different bits of metadata about the module. In particular, this metadata falls into two categories: * metadata required to use the module at runtime (for bootstrapping, etc) in AOT-only applications. * metadata required to depend on the module from a JIT-compiled app. The latter metadata consists of the module's declarations, imports, and exports. To support JIT usage, this metadata must be included in the generated code, especially if that code is shipped to NPM. However, because this metadata preserves the entire NgModule graph (references to all directives and components in the app), it needs to be removed during optimization for AOT-only builds. Previously, this was done with a clever design: 1. The extra metadata was added by a function called `setNgModuleScope`. A call to this function was generated after each NgModule. 2. This function call was marked as "pure" with a comment and used `noSideEffects` internally, which causes optimizers to remove it. The effect was that in dev mode or test mode (which use JIT), no optimizer runs and the full NgModule metadata was available at runtime. But in production (presumably AOT) builds, the optimizer runs and removes the JIT- specific metadata. However, there are cases where apps that want to use JIT in production, and still make an optimized build. In this case, the JIT-specific metadata would be erroneously removed. This commit solves that problem by adding an `ngJitMode` global variable which guards all `setNgModuleScope` calls. An optimizer can be configured to statically define this global to be `false` for AOT-only builds, causing the extra metadata to be stripped. A configuration for Terser used by the CLI is provided in `tooling.ts` which sets `ngJitMode` to `false` when building AOT apps. PR Close #33671 --- .../src/ngtsc/translator/src/translator.ts | 23 ++++++++++++------ packages/compiler-cli/src/tooling.ts | 5 ++++ .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 2 +- .../compiler-cli/test/ngtsc/scope_spec.ts | 4 ++-- .../compiler/src/compiler_facade_interface.ts | 1 - .../src/render3/r3_module_compiler.ts | 24 ++++++++++++++----- packages/compiler/src/render3/util.ts | 24 ++++++++++++++++--- .../src/compiler/compiler_facade_interface.ts | 1 - packages/core/src/render3/jit/module.ts | 1 - 9 files changed, 63 insertions(+), 22 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/translator/src/translator.ts b/packages/compiler-cli/src/ngtsc/translator/src/translator.ts index 49c791255f..1cdeb0e46f 100644 --- a/packages/compiler-cli/src/ngtsc/translator/src/translator.ts +++ b/packages/compiler-cli/src/ngtsc/translator/src/translator.ts @@ -273,16 +273,25 @@ class ExpressionTranslatorVisitor implements ExpressionVisitor, StatementVisitor visitExternalExpr(ast: ExternalExpr, context: Context): ts.PropertyAccessExpression |ts.Identifier { - if (ast.value.moduleName === null || ast.value.name === null) { + if (ast.value.name === null) { throw new Error(`Import unknown module or symbol ${ast.value}`); } - const {moduleImport, symbol} = - this.imports.generateNamedImport(ast.value.moduleName, ast.value.name); - if (moduleImport === null) { - return ts.createIdentifier(symbol); + // If a moduleName is specified, this is a normal import. If there's no module name, it's a + // reference to a global/ambient symbol. + if (ast.value.moduleName !== null) { + // This is a normal import. Find the imported module. + const {moduleImport, symbol} = + this.imports.generateNamedImport(ast.value.moduleName, ast.value.name); + if (moduleImport === null) { + // The symbol was ambient after all. + return ts.createIdentifier(symbol); + } else { + return ts.createPropertyAccess( + ts.createIdentifier(moduleImport), ts.createIdentifier(symbol)); + } } else { - return ts.createPropertyAccess( - ts.createIdentifier(moduleImport), ts.createIdentifier(symbol)); + // The symbol is ambient, so just reference it. + return ts.createIdentifier(ast.value.name); } } diff --git a/packages/compiler-cli/src/tooling.ts b/packages/compiler-cli/src/tooling.ts index 0bad1cab1a..6fa11270a6 100644 --- a/packages/compiler-cli/src/tooling.ts +++ b/packages/compiler-cli/src/tooling.ts @@ -20,3 +20,8 @@ export const GLOBAL_DEFS_FOR_TERSER = { ngDevMode: false, ngI18nClosureMode: false, }; + +export const GLOBAL_DEFS_FOR_TERSER_WITH_AOT = { + ...GLOBAL_DEFS_FOR_TERSER, + ngJitMode: false, +}; diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index b835975d41..1036b31538 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -548,7 +548,7 @@ runInEachFileSystem(os => { .toContain('i0.ɵɵdefineNgModule({ type: TestModule, bootstrap: [TestCmp] });'); expect(jsContents) .toContain( - '/*@__PURE__*/ i0.ɵɵsetNgModuleScope(TestModule, { declarations: [TestCmp] });'); + 'function () { (typeof ngJitMode === "undefined" || ngJitMode) && i0.ɵɵsetNgModuleScope(TestModule, { declarations: [TestCmp] }); })();'); expect(jsContents) .toContain( 'i0.ɵɵdefineInjector({ factory: ' + diff --git a/packages/compiler-cli/test/ngtsc/scope_spec.ts b/packages/compiler-cli/test/ngtsc/scope_spec.ts index 2804e5b5cd..965eca21a1 100644 --- a/packages/compiler-cli/test/ngtsc/scope_spec.ts +++ b/packages/compiler-cli/test/ngtsc/scope_spec.ts @@ -46,7 +46,7 @@ runInEachFileSystem(() => { expect(jsContents).toContain('i0.ɵɵdefineNgModule({ type: TestModule });'); expect(jsContents) .toContain( - '/*@__PURE__*/ i0.ɵɵsetNgModuleScope(TestModule, { imports: [OtherModule] });'); + 'function () { (typeof ngJitMode === "undefined" || ngJitMode) && i0.ɵɵsetNgModuleScope(TestModule, { imports: [OtherModule] }); })();'); const dtsContents = env.getContents('test.d.ts'); expect(dtsContents) @@ -107,7 +107,7 @@ runInEachFileSystem(() => { expect(jsContents).toContain('i0.ɵɵdefineNgModule({ type: TestModule });'); expect(jsContents) .toContain( - '/*@__PURE__*/ i0.ɵɵsetNgModuleScope(TestModule, { exports: [OtherModule] });'); + '(function () { (typeof ngJitMode === "undefined" || ngJitMode) && i0.ɵɵsetNgModuleScope(TestModule, { exports: [OtherModule] }); })();'); const dtsContents = env.getContents('test.d.ts'); expect(dtsContents) diff --git a/packages/compiler/src/compiler_facade_interface.ts b/packages/compiler/src/compiler_facade_interface.ts index 75c7202529..05da363184 100644 --- a/packages/compiler/src/compiler_facade_interface.ts +++ b/packages/compiler/src/compiler_facade_interface.ts @@ -113,7 +113,6 @@ export interface R3NgModuleMetadataFacade { declarations: Function[]; imports: Function[]; exports: Function[]; - emitInline: boolean; schemas: {name: string}[]|null; id: string|null; } diff --git a/packages/compiler/src/render3/r3_module_compiler.ts b/packages/compiler/src/render3/r3_module_compiler.ts index c571ca89e3..37bb1b5add 100644 --- a/packages/compiler/src/render3/r3_module_compiler.ts +++ b/packages/compiler/src/render3/r3_module_compiler.ts @@ -14,7 +14,7 @@ import {OutputContext} from '../util'; import {R3DependencyMetadata, R3FactoryTarget, compileFactoryFunction} from './r3_factory'; import {Identifiers as R3} from './r3_identifiers'; -import {R3Reference, convertMetaToOutput, mapToMapExpression} from './util'; +import {R3Reference, convertMetaToOutput, jitOnlyGuardedExpression, mapToMapExpression} from './util'; export interface R3NgModuleDef { expression: o.Expression; @@ -199,13 +199,25 @@ function generateSetNgModuleScopeCall(meta: R3NgModuleMetadata): o.Statement|nul return null; } + // setNgModuleScope(...) const fnCall = new o.InvokeFunctionExpr( /* fn */ o.importExpr(R3.setNgModuleScope), - /* args */[moduleType, mapToMapExpression(scopeMap)], - /* type */ undefined, - /* sourceSpan */ undefined, - /* pure */ true); - return fnCall.toStmt(); + /* args */[moduleType, mapToMapExpression(scopeMap)]); + + // (ngJitMode guard) && setNgModuleScope(...) + const guardedCall = jitOnlyGuardedExpression(fnCall); + + // function() { (ngJitMode guard) && setNgModuleScope(...); } + const iife = new o.FunctionExpr( + /* params */[], + /* statements */[guardedCall.toStmt()]); + + // (function() { (ngJitMode guard) && setNgModuleScope(...); })() + const iifeCall = new o.InvokeFunctionExpr( + /* fn */ iife, + /* args */[]); + + return iifeCall.toStmt(); } export interface R3InjectorDef { diff --git a/packages/compiler/src/render3/util.ts b/packages/compiler/src/render3/util.ts index 493ed0e90e..1bfecc586f 100644 --- a/packages/compiler/src/render3/util.ts +++ b/packages/compiler/src/render3/util.ts @@ -13,8 +13,16 @@ import {OutputContext} from '../util'; /** * Convert an object map with `Expression` values into a `LiteralMapExpr`. */ -export function mapToMapExpression(map: {[key: string]: o.Expression}): o.LiteralMapExpr { - const result = Object.keys(map).map(key => ({key, value: map[key], quoted: false})); +export function mapToMapExpression(map: {[key: string]: o.Expression | undefined}): + o.LiteralMapExpr { + const result = Object.keys(map).map( + key => ({ + key, + // The assertion here is because really TypeScript doesn't allow us to express that if the + // key is present, it will have a value, but this is true in reality. + value: map[key] !, + quoted: false, + })); return o.literalMap(result); } @@ -79,4 +87,14 @@ export function getSyntheticPropertyName(name: string) { export function prepareSyntheticListenerFunctionName(name: string, phase: string) { return `animation_${name}_${phase}`; -} \ No newline at end of file +} + +export function jitOnlyGuardedExpression(expr: o.Expression): o.Expression { + const ngJitMode = new o.ExternalExpr({name: 'ngJitMode', moduleName: null}); + const jitFlagNotDefined = new o.BinaryOperatorExpr( + o.BinaryOperator.Identical, new o.TypeofExpr(ngJitMode), o.literal('undefined')); + const jitFlagUndefinedOrTrue = new o.BinaryOperatorExpr( + o.BinaryOperator.Or, jitFlagNotDefined, ngJitMode, /* type */ undefined, + /* sourceSpan */ undefined, true); + return new o.BinaryOperatorExpr(o.BinaryOperator.And, jitFlagUndefinedOrTrue, expr); +} diff --git a/packages/core/src/compiler/compiler_facade_interface.ts b/packages/core/src/compiler/compiler_facade_interface.ts index 75c7202529..05da363184 100644 --- a/packages/core/src/compiler/compiler_facade_interface.ts +++ b/packages/core/src/compiler/compiler_facade_interface.ts @@ -113,7 +113,6 @@ export interface R3NgModuleMetadataFacade { declarations: Function[]; imports: Function[]; exports: Function[]; - emitInline: boolean; schemas: {name: string}[]|null; id: string|null; } diff --git a/packages/core/src/render3/jit/module.ts b/packages/core/src/render3/jit/module.ts index f0daa049bf..a101597797 100644 --- a/packages/core/src/render3/jit/module.ts +++ b/packages/core/src/render3/jit/module.ts @@ -125,7 +125,6 @@ export function compileNgModuleDefs( exports: flatten(ngModule.exports || EMPTY_ARRAY) .map(resolveForwardRef) .map(expandModuleWithProviders), - emitInline: true, schemas: ngModule.schemas ? flatten(ngModule.schemas) : null, id: ngModule.id || null, });