fix(@ngtools/webpack): elide imports for all removed identifiers

packages/@ngtools/webpack/src/angular_compiler_plugin.ts

@@ -638,16 +638,14 @@ export class AngularCompilerPlugin implements Tapable {
     const isMainPath = (fileName: string) => fileName === this._mainPath;
     const getEntryModule = () => this.entryModule;
     const getLazyRoutes = () => this._lazyRoutes;
+    const getTypeChecker = () => this._getTsProgram().getTypeChecker();
 
     if (this._JitMode) {
       // Replace resources in JIT.
       this._transformers.push(replaceResources(isAppPath));
     } else {
       // Remove unneeded angular decorators.
-      this._transformers.push(removeDecorators(
-        () => this._getTsProgram().getTypeChecker(),
-        isAppPath
-      ));
+      this._transformers.push(removeDecorators(isAppPath, getTypeChecker));
     }
 
     if (this._platform === PLATFORM.Browser) {
@@ -661,7 +659,7 @@ export class AngularCompilerPlugin implements Tapable {
 
       if (!this._JitMode) {
         // Replace bootstrap in browser AOT.
-        this._transformers.push(replaceBootstrap(isAppPath, getEntryModule));
+        this._transformers.push(replaceBootstrap(isAppPath, getEntryModule, getTypeChecker));
       }
     } else if (this._platform === PLATFORM.Server) {
       this._transformers.push(exportLazyModuleMap(isMainPath, getLazyRoutes));

packages/@ngtools/webpack/src/transformers/elide_imports.ts

@@ -0,0 +1,121 @@
+// @ignoreDep typescript
+import * as ts from 'typescript';
+
+import { collectDeepNodes } from './ast_helpers';
+import { RemoveNodeOperation, TransformOperation } from './interfaces';
+
+
+interface RemovedSymbol {
+  symbol: ts.Symbol;
+  importDecl: ts.ImportDeclaration;
+  importSpec: ts.ImportSpecifier;
+  singleImport: boolean;
+  removed: ts.Identifier[];
+  all: ts.Identifier[];
+}
+
+// Remove imports for which all identifiers have been removed.
+// Needs type checker, and works even if it's not the first transformer.
+// Works by removing imports for symbols whose identifiers have all been removed.
+// Doesn't use the `symbol.declarations` because that previous transforms might have removed nodes
+// but the type checker doesn't know.
+// See https://github.com/Microsoft/TypeScript/issues/17552 for more information.
+export function elideImports(
+  sourceFile: ts.SourceFile,
+  removedNodes: ts.Node[],
+  getTypeChecker: () => ts.TypeChecker,
+): TransformOperation[] {
+  const ops: TransformOperation[] = [];
+
+  if (removedNodes.length === 0) {
+    return [];
+  }
+
+  // Get all children identifiers inside the removed nodes.
+  const removedIdentifiers = removedNodes
+    .map((node) => collectDeepNodes<ts.Identifier>(node, ts.SyntaxKind.Identifier))
+    .reduce((prev, curr) => prev.concat(curr), [])
+    // Also add the top level nodes themselves if they are identifiers.
+    .concat(removedNodes.filter((node) =>
+      node.kind === ts.SyntaxKind.Identifier) as ts.Identifier[]);
+
+  if (removedIdentifiers.length === 0) {
+    return [];
+  }
+
+  // Get all imports in the source file.
+  const allImports = collectDeepNodes<ts.ImportDeclaration>(
+    sourceFile, ts.SyntaxKind.ImportDeclaration);
+
+  if (allImports.length === 0) {
+    return [];
+  }
+
+  const removedSymbolMap: Map<string, RemovedSymbol> = new Map();
+  const typeChecker = getTypeChecker();
+
+  // Find all imports that use a removed identifier and add them to the map.
+  allImports
+    .filter((node: ts.ImportDeclaration) => {
+      // TODO: try to support removing `import * as X from 'XYZ'`.
+      // Filter out import statements that are either `import 'XYZ'` or `import * as X from 'XYZ'`.
+      const clause = node.importClause as ts.ImportClause;
+      if (!clause || clause.name || !clause.namedBindings) {
+        return false;
+      }
+      return clause.namedBindings.kind == ts.SyntaxKind.NamedImports;
+    })
+    .forEach((importDecl: ts.ImportDeclaration) => {
+      const importClause = importDecl.importClause as ts.ImportClause;
+      const namedImports = importClause.namedBindings as ts.NamedImports;
+
+      namedImports.elements.forEach((importSpec: ts.ImportSpecifier) => {
+        const importId = importSpec.name;
+        const symbol = typeChecker.getSymbolAtLocation(importId);
+
+        const removedNodesForImportId = removedIdentifiers.filter((id) =>
+          id.text === importId.text && typeChecker.getSymbolAtLocation(id) === symbol);
+
+        if (removedNodesForImportId.length > 0) {
+          removedSymbolMap.set(importId.text, {
+            symbol,
+            importDecl,
+            importSpec,
+            singleImport: namedImports.elements.length === 1,
+            removed: removedNodesForImportId,
+            all: []
+          });
+        }
+      });
+    });
+
+
+  if (removedSymbolMap.size === 0) {
+    return [];
+  }
+
+  // Find all identifiers in the source file that have a removed symbol, and add them to the map.
+  collectDeepNodes<ts.Identifier>(sourceFile, ts.SyntaxKind.Identifier)
+    .forEach((id) => {
+      if (removedSymbolMap.has(id.text)) {
+        const symbol = removedSymbolMap.get(id.text);
+        if (typeChecker.getSymbolAtLocation(id) === symbol.symbol) {
+          symbol.all.push(id);
+        }
+      }
+    });
+
+  Array.from(removedSymbolMap.values())
+    .filter((symbol) => {
+      // If the number of removed imports plus one (the import specifier) is equal to the total
+      // number of identifiers for that symbol, it's safe to remove the import.
+      return symbol.removed.length + 1 === symbol.all.length;
+    })
+    .forEach((symbol) => {
+      // Remove the whole declaration if it's a single import.
+      const nodeToRemove = symbol.singleImport ? symbol.importSpec : symbol.importDecl;
+      ops.push(new RemoveNodeOperation(sourceFile, nodeToRemove));
+    });
+
+  return ops;
+}

packages/@ngtools/webpack/src/transformers/index.ts

@@ -2,7 +2,7 @@ export * from './interfaces';
 export * from './ast_helpers';
 export * from './make_transform';
 export * from './insert_import';
-export * from './remove_import';
+export * from './elide_imports';
 export * from './replace_bootstrap';
 export * from './export_ngfactory';
 export * from './export_lazy_module_map';

packages/@ngtools/webpack/src/transformers/make_transform.ts

@@ -9,6 +9,7 @@ import {
   AddNodeOperation,
   ReplaceNodeOperation,
 } from './interfaces';
+import { elideImports } from './elide_imports';
 
 
 // Typescript below 2.5.0 needs a workaround.
@@ -17,7 +18,8 @@ const visitEachChild = satisfies(ts.version, '^2.5.0')
   : visitEachChildWorkaround;
 
 export function makeTransform(
-  standardTransform: StandardTransform
+  standardTransform: StandardTransform,
+  getTypeChecker?: () => ts.TypeChecker,
 ): ts.TransformerFactory<ts.SourceFile> {
 
   return (context: ts.TransformationContext): ts.Transformer<ts.SourceFile> => {
@@ -30,6 +32,16 @@ export function makeTransform(
       const replaceOps = ops
         .filter((op) => op.kind === OPERATION_KIND.Replace) as ReplaceNodeOperation[];
 
+      // If nodes are removed, elide the imports as well.
+      // Mainly a workaround for https://github.com/Microsoft/TypeScript/issues/17552.
+      // WARNING: this assumes that replaceOps DO NOT reuse any of the nodes they are replacing.
+      // This is currently true for transforms that use replaceOps (replace_bootstrap and
+      // replace_resources), but may not be true for new transforms.
+      if (getTypeChecker && removeOps.length + replaceOps.length > 0) {
+        const removedNodes = removeOps.concat(replaceOps).map((op) => op.target);
+        removeOps.push(...elideImports(sf, removedNodes, getTypeChecker));
+      }
+
       const visitor: ts.Visitor = (node) => {
         let modified = false;
         let modifiedNodes = [node];

packages/@ngtools/webpack/src/transformers/multiple_transformers.spec.ts

@@ -1,19 +1,31 @@
 import { oneLine, stripIndent } from 'common-tags';
-import { transformTypescript } from './ast_helpers';
+import { createTypescriptContext, transformTypescript } from './ast_helpers';
 import { replaceBootstrap } from './replace_bootstrap';
 import { exportNgFactory } from './export_ngfactory';
 import { exportLazyModuleMap } from './export_lazy_module_map';
+import { removeDecorators } from './remove_decorators';
+
 
 describe('@ngtools/webpack transformers', () => {
   describe('multiple_transformers', () => {
     it('should apply multiple transformers on the same file', () => {
       const input = stripIndent`
         import { enableProdMode } from '@angular/core';
         import { platformBrowserDynamic } from '@angular/platform-browser-dynamic';
+        import { Component } from '@angular/core';
 
         import { AppModule } from './app/app.module';
         import { environment } from './environments/environment';
 
+        @Component({
+          selector: 'app-root',
+          templateUrl: './app.component.html',
+          styleUrls: ['./app.component.css']
+        })
+        class AppComponent {
+          title = 'app';
+        }
+
         if (environment.production) {
           enableProdMode();
         }
@@ -31,6 +43,11 @@ describe('@ngtools/webpack transformers', () => {
 
         import * as __NgCli_bootstrap_1 from "./app/app.module.ngfactory";
         import * as __NgCli_bootstrap_2 from "@angular/platform-browser";
+
+        class AppComponent {
+          constructor() { this.title = 'app'; }
+        }
+
         if (environment.production) {
           enableProdMode();
         }
@@ -40,12 +57,16 @@ describe('@ngtools/webpack transformers', () => {
       `;
       // tslint:enable:max-line-length
 
+      const { program, compilerHost } = createTypescriptContext(input);
+
       const shouldTransform = () => true;
       const getEntryModule = () =>
         ({ path: '/project/src/app/app.module', className: 'AppModule' });
+      const getTypeChecker = () => program.getTypeChecker();
+
 
       const transformers = [
-        replaceBootstrap(shouldTransform, getEntryModule),
+        replaceBootstrap(shouldTransform, getEntryModule, getTypeChecker),
         exportNgFactory(shouldTransform, getEntryModule),
         exportLazyModuleMap(shouldTransform,
           () => ({
@@ -54,9 +75,10 @@ describe('@ngtools/webpack transformers', () => {
             './lazy2/lazy2.module.ngfactory#LazyModule2NgFactory':
             '/project/src/app/lazy2/lazy2.module.ngfactory.ts',
           })),
+        removeDecorators(shouldTransform, getTypeChecker),
       ];
 
-      const result = transformTypescript(input, transformers);
+      const result = transformTypescript(undefined, transformers, program, compilerHost);
 
       expect(oneLine`${result}`).toEqual(oneLine`${output}`);
     });

packages/@ngtools/webpack/src/transformers/remove_decorators.spec.ts

@@ -27,8 +27,8 @@ describe('@ngtools/webpack transformers', () => {
 
       const { program, compilerHost } = createTypescriptContext(input);
       const transformer = removeDecorators(
-        () => program.getTypeChecker(),
         () => true,
+        () => program.getTypeChecker(),
       );
       const result = transformTypescript(undefined, [transformer], program, compilerHost);
 
@@ -68,8 +68,8 @@ describe('@ngtools/webpack transformers', () => {
 
       const { program, compilerHost } = createTypescriptContext(input);
       const transformer = removeDecorators(
-        () => program.getTypeChecker(),
         () => true,
+        () => program.getTypeChecker(),
       );
       const result = transformTypescript(undefined, [transformer], program, compilerHost);
 
@@ -101,8 +101,8 @@ describe('@ngtools/webpack transformers', () => {
 
       const { program, compilerHost } = createTypescriptContext(input);
       const transformer = removeDecorators(
-        () => program.getTypeChecker(),
         () => true,
+        () => program.getTypeChecker(),
       );
       const result = transformTypescript(undefined, [transformer], program, compilerHost);
 

packages/@ngtools/webpack/src/transformers/remove_decorators.ts

@@ -1,14 +1,13 @@
 import * as ts from 'typescript';
 
 import { collectDeepNodes } from './ast_helpers';
-import { removeImport } from './remove_import';
 import { StandardTransform, TransformOperation, RemoveNodeOperation } from './interfaces';
 import { makeTransform } from './make_transform';
 
 
 export function removeDecorators(
+  shouldTransform: (fileName: string) => boolean,
   getTypeChecker: () => ts.TypeChecker,
-  shouldTransform: (fileName: string) => boolean
 ): ts.TransformerFactory<ts.SourceFile> {
 
   const standardTransform: StandardTransform = function (sourceFile: ts.SourceFile) {
@@ -23,25 +22,12 @@ export function removeDecorators(
       .forEach((decorator) => {
         // Remove the decorator node.
         ops.push(new RemoveNodeOperation(sourceFile, decorator));
-        // Also remove imports for identifiers used in the decorator.
-        // TS doesn't automatically elide imports for things removed in transformers so we have
-        // to do it manually.
-        collectDeepNodes<ts.Identifier>(decorator, ts.SyntaxKind.Identifier)
-          .forEach((identifier) => {
-            // This is finding a lot of things that might not be imports at all, but we can't
-            // use the type checker since previous transforms might have modified things
-            // Worst case scenario, some imports will be left over because there was another
-            // identifier somewhere in the file as a property access or something with the same
-            // name as the identifer we want to remove.
-            // TODO: figure out if there's a better way to elide imports.
-            ops.push(...removeImport(sourceFile, [identifier]));
-          });
       });
 
     return ops;
   };
 
-  return makeTransform(standardTransform);
+  return makeTransform(standardTransform, getTypeChecker);
 }
 
 function shouldRemove(decorator: ts.Decorator, typeChecker: ts.TypeChecker): boolean {