From 5ef6e6366f09107925d17e844e818ef071edffe2 Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Fri, 1 Sep 2017 16:27:35 -0700 Subject: [PATCH] fix(compiler): lower variables with a closure by exporting the variable. This e.g. leaves comments at the right place, which is important for closure. --- .../src/transformers/lower_expressions.ts | 84 +++++++--- packages/compiler-cli/test/ngc_spec.ts | 146 +++++++++--------- .../transformers/lower_expressions_spec.ts | 5 + 3 files changed, 134 insertions(+), 101 deletions(-) diff --git a/packages/compiler-cli/src/transformers/lower_expressions.ts b/packages/compiler-cli/src/transformers/lower_expressions.ts index 11666c96dc..e6a2476b3d 100644 --- a/packages/compiler-cli/src/transformers/lower_expressions.ts +++ b/packages/compiler-cli/src/transformers/lower_expressions.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {CollectorOptions, MetadataCollector, MetadataValue, ModuleMetadata, isMetadataGlobalReferenceExpression} from '@angular/tsc-wrapped'; +import {CollectorOptions, MetadataCollector, MetadataError, MetadataValue, ModuleMetadata, isMetadataGlobalReferenceExpression} from '@angular/tsc-wrapped'; import * as ts from 'typescript'; export interface LoweringRequest { @@ -18,14 +18,17 @@ export interface LoweringRequest { export type RequestLocationMap = Map; +const enum DeclarationOrder { BeforeStmt, AfterStmt } + interface Declaration { name: string; node: ts.Node; + order: DeclarationOrder; } interface DeclarationInsert { declarations: Declaration[]; - priorTo: ts.Node; + relativeTo: ts.Node; } function toMap(items: T[], select: (item: T) => K): Map { @@ -72,17 +75,32 @@ function transformSourceFile( function visitNode(node: ts.Node): ts.Node { // Get the original node before tsickle - const {pos, end, kind} = ts.getOriginalNode(node); + const {pos, end, kind, parent: originalParent} = ts.getOriginalNode(node); const nodeRequest = requests.get(pos); if (nodeRequest && nodeRequest.kind == kind && nodeRequest.end == end) { // This node is requested to be rewritten as a reference to the exported name. + if (originalParent && originalParent.kind === ts.SyntaxKind.VariableDeclaration) { + // As the value represents the whole initializer of a variable declaration, + // just refer to that variable. This e.g. helps to preserve closure comments + // at the right place. + const varParent = originalParent as ts.VariableDeclaration; + if (varParent.name.kind === ts.SyntaxKind.Identifier) { + const varName = varParent.name.text; + const exportName = nodeRequest.name; + declarations.push({ + name: exportName, + node: ts.createIdentifier(varName), + order: DeclarationOrder.AfterStmt + }); + return node; + } + } // Record that the node needs to be moved to an exported variable with the given name - const name = nodeRequest.name; - declarations.push({name, node}); - return ts.createIdentifier(name); + const exportName = nodeRequest.name; + declarations.push({name: exportName, node, order: DeclarationOrder.BeforeStmt}); + return ts.createIdentifier(exportName); } let result = node; - if (shouldVisit(pos, end) && !isLexicalScope(node)) { result = ts.visitEachChild(node, visitNode, context); } @@ -91,35 +109,44 @@ function transformSourceFile( // Get the original node before tsickle const {pos, end} = ts.getOriginalNode(node); - const result = shouldVisit(pos, end) ? ts.visitEachChild(node, visitNode, context) : node; + let resultStmt: ts.Statement; + if (shouldVisit(pos, end)) { + resultStmt = ts.visitEachChild(node, visitNode, context); + } else { + resultStmt = node; + } if (declarations.length) { - inserts.push({priorTo: result, declarations}); + inserts.push({relativeTo: resultStmt, declarations}); } - return result; + return resultStmt; } - const newStatements = sourceFile.statements.map(topLevelStatement); + let newStatements = sourceFile.statements.map(topLevelStatement); if (inserts.length) { - // Insert the declarations before the rewritten statement that references them. - const insertMap = toMap(inserts, i => i.priorTo); - for (let i = newStatements.length; i >= 0; i--) { - const statement = newStatements[i]; + // Insert the declarations relative to the rewritten statement that references them. + const insertMap = toMap(inserts, i => i.relativeTo); + const tmpStatements: ts.Statement[] = []; + newStatements.forEach(statement => { const insert = insertMap.get(statement); if (insert) { - const declarations = insert.declarations.map( - i => ts.createVariableDeclaration( - i.name, /* type */ undefined, i.node as ts.Expression)); - const statement = ts.createVariableStatement( - /* modifiers */ undefined, - ts.createVariableDeclarationList(declarations, ts.NodeFlags.Const)); - newStatements.splice(i, 0, statement); + const before = insert.declarations.filter(d => d.order === DeclarationOrder.BeforeStmt); + if (before.length) { + tmpStatements.push(createVariableStatementForDeclarations(before)); + } + tmpStatements.push(statement); + const after = insert.declarations.filter(d => d.order === DeclarationOrder.AfterStmt); + if (after.length) { + tmpStatements.push(createVariableStatementForDeclarations(after)); + } + } else { + tmpStatements.push(statement); } - } + }); // Insert an exports clause to export the declarations - newStatements.push(ts.createExportDeclaration( + tmpStatements.push(ts.createExportDeclaration( /* decorators */ undefined, /* modifiers */ undefined, ts.createNamedExports( @@ -130,6 +157,8 @@ function transformSourceFile( .map( declaration => ts.createExportSpecifier( /* propertyName */ undefined, declaration.name))))); + + newStatements = tmpStatements; } // Note: We cannot use ts.updateSourcefile here as // it does not work well with decorators. @@ -145,6 +174,13 @@ function transformSourceFile( return visitSourceFile(sourceFile); } +function createVariableStatementForDeclarations(declarations: Declaration[]): ts.VariableStatement { + const varDecls = declarations.map( + i => ts.createVariableDeclaration(i.name, /* type */ undefined, i.node as ts.Expression)); + return ts.createVariableStatement( + /* modifiers */ undefined, ts.createVariableDeclarationList(varDecls, ts.NodeFlags.Const)); +} + export function getExpressionLoweringTransformFactory(requestsMap: RequestsMap): (context: ts.TransformationContext) => (sourceFile: ts.SourceFile) => ts.SourceFile { // Return the factory diff --git a/packages/compiler-cli/test/ngc_spec.ts b/packages/compiler-cli/test/ngc_spec.ts index 13b616c291..78d052e82e 100644 --- a/packages/compiler-cli/test/ngc_spec.ts +++ b/packages/compiler-cli/test/ngc_spec.ts @@ -519,7 +519,6 @@ describe('ngc transformer command-line', () => { }); function compile(): number { - errorSpy.calls.reset(); const result = mainSync(['-p', path.join(basePath, 'tsconfig.json')], errorSpy); expect(errorSpy).not.toHaveBeenCalled(); return result; @@ -622,8 +621,9 @@ describe('ngc transformer command-line', () => { const mymodulejs = path.resolve(outDir, 'mymodule.js'); const mymoduleSource = fs.readFileSync(mymodulejs, 'utf8'); - expect(mymoduleSource).toContain('var ɵ0 = function () { return new Foo(); }'); - expect(mymoduleSource).toContain('export { ɵ0'); + expect(mymoduleSource).toContain('var factory = function () { return new Foo(); }'); + expect(mymoduleSource).toContain('var ɵ0 = factory;'); + expect(mymoduleSource).toContain('export { ɵ0 };'); }); it('should not lower a lambda that is already exported', () => { @@ -647,6 +647,72 @@ describe('ngc transformer command-line', () => { const mymoduleSource = fs.readFileSync(mymodulejs, 'utf8'); expect(mymoduleSource).not.toContain('ɵ0'); }); + + it('should be able to lower supported expressions', () => { + writeConfig(`{ + "extends": "./tsconfig-base.json", + "files": ["module.ts"] + }`); + write('module.ts', ` + import {NgModule, InjectionToken} from '@angular/core'; + import {AppComponent} from './app'; + + export interface Info { + route: string; + data: string; + } + + export const T1 = new InjectionToken('t1'); + export const T2 = new InjectionToken('t2'); + export const T3 = new InjectionToken('t3'); + export const T4 = new InjectionToken('t4'); + + enum SomeEnum { + OK, + Cancel + } + + function calculateString() { + return 'someValue'; + } + + const routeLikeData = [{ + route: '/home', + data: calculateString() + }]; + + @NgModule({ + declarations: [AppComponent], + providers: [ + { provide: T1, useValue: calculateString() }, + { provide: T2, useFactory: () => 'someValue' }, + { provide: T3, useValue: SomeEnum.OK }, + { provide: T4, useValue: routeLikeData } + ] + }) + export class MyModule {} + `); + write('app.ts', ` + import {Component, Inject} from '@angular/core'; + import * as m from './module'; + + @Component({ + selector: 'my-app', + template: '' + }) + export class AppComponent { + constructor( + @Inject(m.T1) private t1: string, + @Inject(m.T2) private t2: string, + @Inject(m.T3) private t3: number, + @Inject(m.T4) private t4: m.Info[], + ) {} + } + `); + + expect(mainSync(['-p', basePath], errorSpy)).toBe(0); + shouldExist('module.js'); + }); }); it('should be able to generate a flat module library', () => { @@ -848,78 +914,4 @@ describe('ngc transformer command-line', () => { shouldExist('app/main.js'); }); }); - - describe('expression lowering', () => { - const shouldExist = (fileName: string) => { - if (!fs.existsSync(path.resolve(basePath, fileName))) { - throw new Error(`Expected ${fileName} to be emitted (basePath: ${basePath})`); - } - }; - - it('should be able to lower supported expressions', () => { - writeConfig(`{ - "extends": "./tsconfig-base.json", - "files": ["module.ts"] - }`); - write('module.ts', ` - import {NgModule, InjectionToken} from '@angular/core'; - import {AppComponent} from './app'; - - export interface Info { - route: string; - data: string; - } - - export const T1 = new InjectionToken('t1'); - export const T2 = new InjectionToken('t2'); - export const T3 = new InjectionToken('t3'); - export const T4 = new InjectionToken('t4'); - - enum SomeEnum { - OK, - Cancel - } - - function calculateString() { - return 'someValue'; - } - - const routeLikeData = [{ - route: '/home', - data: calculateString() - }]; - - @NgModule({ - declarations: [AppComponent], - providers: [ - { provide: T1, useValue: calculateString() }, - { provide: T2, useFactory: () => 'someValue' }, - { provide: T3, useValue: SomeEnum.OK }, - { provide: T4, useValue: routeLikeData } - ] - }) - export class MyModule {} - `); - write('app.ts', ` - import {Component, Inject} from '@angular/core'; - import * as m from './module'; - - @Component({ - selector: 'my-app', - template: '' - }) - export class AppComponent { - constructor( - @Inject(m.T1) private t1: string, - @Inject(m.T2) private t2: string, - @Inject(m.T3) private t3: number, - @Inject(m.T4) private t4: m.Info[], - ) {} - } - `); - - expect(mainSync(['-p', basePath], s => {})).toBe(0); - shouldExist('built/module.js'); - }); - }); }); diff --git a/packages/compiler-cli/test/transformers/lower_expressions_spec.ts b/packages/compiler-cli/test/transformers/lower_expressions_spec.ts index fdb5c92c18..584576f800 100644 --- a/packages/compiler-cli/test/transformers/lower_expressions_spec.ts +++ b/packages/compiler-cli/test/transformers/lower_expressions_spec.ts @@ -28,6 +28,11 @@ describe('Expression lowering', () => { class MyClass {} `)).toContain('const l = () => null; exports.l = l;'); }); + + it('should be able to export a variable if the whole value is lowered', () => { + expect(convert('/*a*/ const a =◊b: () => null◊;')) + .toBe('/*a*/ const a = () => null; const b = a; export { b };'); + }); }); describe('collector', () => {