Skip to content

Commit

Permalink
fix(ivy): retain JIT metadata unless JIT mode is explicitly disabled (#…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
alxhub committed Nov 20, 2019
1 parent eb6975a commit b54ed98
Show file tree
Hide file tree
Showing 9 changed files with 63 additions and 22 deletions.
23 changes: 16 additions & 7 deletions packages/compiler-cli/src/ngtsc/translator/src/translator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
5 changes: 5 additions & 0 deletions packages/compiler-cli/src/tooling.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
2 changes: 1 addition & 1 deletion packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: ' +
Expand Down
4 changes: 2 additions & 2 deletions packages/compiler-cli/test/ngtsc/scope_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
1 change: 0 additions & 1 deletion packages/compiler/src/compiler_facade_interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,6 @@ export interface R3NgModuleMetadataFacade {
declarations: Function[];
imports: Function[];
exports: Function[];
emitInline: boolean;
schemas: {name: string}[]|null;
id: string|null;
}
Expand Down
24 changes: 18 additions & 6 deletions packages/compiler/src/render3/r3_module_compiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down
24 changes: 21 additions & 3 deletions packages/compiler/src/render3/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -79,4 +87,14 @@ export function getSyntheticPropertyName(name: string) {

export function prepareSyntheticListenerFunctionName(name: string, phase: string) {
return `animation_${name}_${phase}`;
}
}

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);
}
1 change: 0 additions & 1 deletion packages/core/src/compiler/compiler_facade_interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,6 @@ export interface R3NgModuleMetadataFacade {
declarations: Function[];
imports: Function[];
exports: Function[];
emitInline: boolean;
schemas: {name: string}[]|null;
id: string|null;
}
Expand Down
1 change: 0 additions & 1 deletion packages/core/src/render3/jit/module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
Expand Down

0 comments on commit b54ed98

Please sign in to comment.