Skip to content

Commit

Permalink
fix(compiler): lower variables with a closure by exporting the variable.
Browse files Browse the repository at this point in the history 
This e.g. leaves comments at the right place, which is important for closure.
  • Loading branch information
@tbosch @matsko
tbosch authored and matsko committed Sep 7, 2017
1 parent b6833d1 commit 5ef6e63
Show file tree
Hide file tree
Showing 3 changed files with 134 additions and 101 deletions.
84 changes: 60 additions & 24 deletions packages/compiler-cli/src/transformers/lower_expressions.ts
View file
Expand Up @@ -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 {
Expand All @@ -18,14 +18,17 @@ export interface LoweringRequest {

export type RequestLocationMap = Map<number, LoweringRequest>;

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<T, K>(items: T[], select: (item: T) => K): Map<K, T> {
Expand Down Expand Up @@ -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);
}
Expand All @@ -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(
Expand All @@ -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.
Expand All @@ -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
Expand Down
146 changes: 69 additions & 77 deletions packages/compiler-cli/test/ngc_spec.ts
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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', () => {
Expand All @@ -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<string>('t1');
export const T2 = new InjectionToken<string>('t2');
export const T3 = new InjectionToken<number>('t3');
export const T4 = new InjectionToken<Info[]>('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', () => {
Expand Down Expand Up @@ -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<string>('t1');
export const T2 = new InjectionToken<string>('t2');
export const T3 = new InjectionToken<number>('t3');
export const T4 = new InjectionToken<Info[]>('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');
});
});
});
5 changes: 5 additions & 0 deletions packages/compiler-cli/test/transformers/lower_expressions_spec.ts
View file
Expand Up @@ -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', () => {
Expand Down

0 comments on commit 5ef6e63

Please sign in to comment.