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.

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<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> {
@@ -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;
+          const exportName = nodeRequest.name;
-          declarations.push({name, node});
+          declarations.push({name: exportName, node, order: DeclarationOrder.BeforeStmt});
-          return ts.createIdentifier(name);
+          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.
+      // Insert the declarations relative to the rewritten statement that references them.
-      const insertMap = toMap(inserts, i => i.priorTo);
+      const insertMap = toMap(inserts, i => i.relativeTo);
-      for (let i = newStatements.length; i >= 0; i--) {
+      const tmpStatements: ts.Statement[] = [];
-        const statement = newStatements[i];
+      newStatements.forEach(statement => {
         const insert = insertMap.get(statement);
         if (insert) {
-          const declarations = insert.declarations.map(
+          const before = insert.declarations.filter(d => d.order === DeclarationOrder.BeforeStmt);
-              i => ts.createVariableDeclaration(
+          if (before.length) {
-                  i.name, /* type */ undefined, i.node as ts.Expression));
+            tmpStatements.push(createVariableStatementForDeclarations(before));
-          const statement = ts.createVariableStatement(
+          }
-              /* modifiers */ undefined,
+          tmpStatements.push(statement);
-              ts.createVariableDeclarationList(declarations, ts.NodeFlags.Const));
+          const after = insert.declarations.filter(d => d.order === DeclarationOrder.AfterStmt);
-          newStatements.splice(i, 0, statement);
+          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

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('var factory = function () { return new Foo(); }');
-        expect(mymoduleSource).toContain('export { ɵ0');
+        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<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', () => {
@@ -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');
-    });
-  });
 });

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', () => {