Skip to content

Commit 9ad6948

Browse files
filipesilvaBrocco
authored andcommitted
fix(@ngtools/webpack): elide imports for all removed identifiers
1 parent 13b3f76 commit 9ad6948

10 files changed

+187
-124
lines changed

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

+3-5
Original file line numberDiff line numberDiff line change
@@ -638,16 +638,14 @@ export class AngularCompilerPlugin implements Tapable {
638638
const isMainPath = (fileName: string) => fileName === this._mainPath;
639639
const getEntryModule = () => this.entryModule;
640640
const getLazyRoutes = () => this._lazyRoutes;
641+
const getTypeChecker = () => this._getTsProgram().getTypeChecker();
641642

642643
if (this._JitMode) {
643644
// Replace resources in JIT.
644645
this._transformers.push(replaceResources(isAppPath));
645646
} else {
646647
// Remove unneeded angular decorators.
647-
this._transformers.push(removeDecorators(
648-
() => this._getTsProgram().getTypeChecker(),
649-
isAppPath
650-
));
648+
this._transformers.push(removeDecorators(isAppPath, getTypeChecker));
651649
}
652650

653651
if (this._platform === PLATFORM.Browser) {
@@ -661,7 +659,7 @@ export class AngularCompilerPlugin implements Tapable {
661659

662660
if (!this._JitMode) {
663661
// Replace bootstrap in browser AOT.
664-
this._transformers.push(replaceBootstrap(isAppPath, getEntryModule));
662+
this._transformers.push(replaceBootstrap(isAppPath, getEntryModule, getTypeChecker));
665663
}
666664
} else if (this._platform === PLATFORM.Server) {
667665
this._transformers.push(exportLazyModuleMap(isMainPath, getLazyRoutes));
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
// @ignoreDep typescript
2+
import * as ts from 'typescript';
3+
4+
import { collectDeepNodes } from './ast_helpers';
5+
import { RemoveNodeOperation, TransformOperation } from './interfaces';
6+
7+
8+
interface RemovedSymbol {
9+
symbol: ts.Symbol;
10+
importDecl: ts.ImportDeclaration;
11+
importSpec: ts.ImportSpecifier;
12+
singleImport: boolean;
13+
removed: ts.Identifier[];
14+
all: ts.Identifier[];
15+
}
16+
17+
// Remove imports for which all identifiers have been removed.
18+
// Needs type checker, and works even if it's not the first transformer.
19+
// Works by removing imports for symbols whose identifiers have all been removed.
20+
// Doesn't use the `symbol.declarations` because that previous transforms might have removed nodes
21+
// but the type checker doesn't know.
22+
// See https://github.com/Microsoft/TypeScript/issues/17552 for more information.
23+
export function elideImports(
24+
sourceFile: ts.SourceFile,
25+
removedNodes: ts.Node[],
26+
getTypeChecker: () => ts.TypeChecker,
27+
): TransformOperation[] {
28+
const ops: TransformOperation[] = [];
29+
30+
if (removedNodes.length === 0) {
31+
return [];
32+
}
33+
34+
// Get all children identifiers inside the removed nodes.
35+
const removedIdentifiers = removedNodes
36+
.map((node) => collectDeepNodes<ts.Identifier>(node, ts.SyntaxKind.Identifier))
37+
.reduce((prev, curr) => prev.concat(curr), [])
38+
// Also add the top level nodes themselves if they are identifiers.
39+
.concat(removedNodes.filter((node) =>
40+
node.kind === ts.SyntaxKind.Identifier) as ts.Identifier[]);
41+
42+
if (removedIdentifiers.length === 0) {
43+
return [];
44+
}
45+
46+
// Get all imports in the source file.
47+
const allImports = collectDeepNodes<ts.ImportDeclaration>(
48+
sourceFile, ts.SyntaxKind.ImportDeclaration);
49+
50+
if (allImports.length === 0) {
51+
return [];
52+
}
53+
54+
const removedSymbolMap: Map<string, RemovedSymbol> = new Map();
55+
const typeChecker = getTypeChecker();
56+
57+
// Find all imports that use a removed identifier and add them to the map.
58+
allImports
59+
.filter((node: ts.ImportDeclaration) => {
60+
// TODO: try to support removing `import * as X from 'XYZ'`.
61+
// Filter out import statements that are either `import 'XYZ'` or `import * as X from 'XYZ'`.
62+
const clause = node.importClause as ts.ImportClause;
63+
if (!clause || clause.name || !clause.namedBindings) {
64+
return false;
65+
}
66+
return clause.namedBindings.kind == ts.SyntaxKind.NamedImports;
67+
})
68+
.forEach((importDecl: ts.ImportDeclaration) => {
69+
const importClause = importDecl.importClause as ts.ImportClause;
70+
const namedImports = importClause.namedBindings as ts.NamedImports;
71+
72+
namedImports.elements.forEach((importSpec: ts.ImportSpecifier) => {
73+
const importId = importSpec.name;
74+
const symbol = typeChecker.getSymbolAtLocation(importId);
75+
76+
const removedNodesForImportId = removedIdentifiers.filter((id) =>
77+
id.text === importId.text && typeChecker.getSymbolAtLocation(id) === symbol);
78+
79+
if (removedNodesForImportId.length > 0) {
80+
removedSymbolMap.set(importId.text, {
81+
symbol,
82+
importDecl,
83+
importSpec,
84+
singleImport: namedImports.elements.length === 1,
85+
removed: removedNodesForImportId,
86+
all: []
87+
});
88+
}
89+
});
90+
});
91+
92+
93+
if (removedSymbolMap.size === 0) {
94+
return [];
95+
}
96+
97+
// Find all identifiers in the source file that have a removed symbol, and add them to the map.
98+
collectDeepNodes<ts.Identifier>(sourceFile, ts.SyntaxKind.Identifier)
99+
.forEach((id) => {
100+
if (removedSymbolMap.has(id.text)) {
101+
const symbol = removedSymbolMap.get(id.text);
102+
if (typeChecker.getSymbolAtLocation(id) === symbol.symbol) {
103+
symbol.all.push(id);
104+
}
105+
}
106+
});
107+
108+
Array.from(removedSymbolMap.values())
109+
.filter((symbol) => {
110+
// If the number of removed imports plus one (the import specifier) is equal to the total
111+
// number of identifiers for that symbol, it's safe to remove the import.
112+
return symbol.removed.length + 1 === symbol.all.length;
113+
})
114+
.forEach((symbol) => {
115+
// Remove the whole declaration if it's a single import.
116+
const nodeToRemove = symbol.singleImport ? symbol.importSpec : symbol.importDecl;
117+
ops.push(new RemoveNodeOperation(sourceFile, nodeToRemove));
118+
});
119+
120+
return ops;
121+
}

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ export * from './interfaces';
22
export * from './ast_helpers';
33
export * from './make_transform';
44
export * from './insert_import';
5-
export * from './remove_import';
5+
export * from './elide_imports';
66
export * from './replace_bootstrap';
77
export * from './export_ngfactory';
88
export * from './export_lazy_module_map';

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

+13-1
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
AddNodeOperation,
1010
ReplaceNodeOperation,
1111
} from './interfaces';
12+
import { elideImports } from './elide_imports';
1213

1314

1415
// Typescript below 2.5.0 needs a workaround.
@@ -17,7 +18,8 @@ const visitEachChild = satisfies(ts.version, '^2.5.0')
1718
: visitEachChildWorkaround;
1819

1920
export function makeTransform(
20-
standardTransform: StandardTransform
21+
standardTransform: StandardTransform,
22+
getTypeChecker?: () => ts.TypeChecker,
2123
): ts.TransformerFactory<ts.SourceFile> {
2224

2325
return (context: ts.TransformationContext): ts.Transformer<ts.SourceFile> => {
@@ -30,6 +32,16 @@ export function makeTransform(
3032
const replaceOps = ops
3133
.filter((op) => op.kind === OPERATION_KIND.Replace) as ReplaceNodeOperation[];
3234

35+
// If nodes are removed, elide the imports as well.
36+
// Mainly a workaround for https://github.com/Microsoft/TypeScript/issues/17552.
37+
// WARNING: this assumes that replaceOps DO NOT reuse any of the nodes they are replacing.
38+
// This is currently true for transforms that use replaceOps (replace_bootstrap and
39+
// replace_resources), but may not be true for new transforms.
40+
if (getTypeChecker && removeOps.length + replaceOps.length > 0) {
41+
const removedNodes = removeOps.concat(replaceOps).map((op) => op.target);
42+
removeOps.push(...elideImports(sf, removedNodes, getTypeChecker));
43+
}
44+
3345
const visitor: ts.Visitor = (node) => {
3446
let modified = false;
3547
let modifiedNodes = [node];

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

+25-3
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,31 @@
11
import { oneLine, stripIndent } from 'common-tags';
2-
import { transformTypescript } from './ast_helpers';
2+
import { createTypescriptContext, transformTypescript } from './ast_helpers';
33
import { replaceBootstrap } from './replace_bootstrap';
44
import { exportNgFactory } from './export_ngfactory';
55
import { exportLazyModuleMap } from './export_lazy_module_map';
6+
import { removeDecorators } from './remove_decorators';
7+
68

79
describe('@ngtools/webpack transformers', () => {
810
describe('multiple_transformers', () => {
911
it('should apply multiple transformers on the same file', () => {
1012
const input = stripIndent`
1113
import { enableProdMode } from '@angular/core';
1214
import { platformBrowserDynamic } from '@angular/platform-browser-dynamic';
15+
import { Component } from '@angular/core';
1316
1417
import { AppModule } from './app/app.module';
1518
import { environment } from './environments/environment';
1619
20+
@Component({
21+
selector: 'app-root',
22+
templateUrl: './app.component.html',
23+
styleUrls: ['./app.component.css']
24+
})
25+
class AppComponent {
26+
title = 'app';
27+
}
28+
1729
if (environment.production) {
1830
enableProdMode();
1931
}
@@ -31,6 +43,11 @@ describe('@ngtools/webpack transformers', () => {
3143
3244
import * as __NgCli_bootstrap_1 from "./app/app.module.ngfactory";
3345
import * as __NgCli_bootstrap_2 from "@angular/platform-browser";
46+
47+
class AppComponent {
48+
constructor() { this.title = 'app'; }
49+
}
50+
3451
if (environment.production) {
3552
enableProdMode();
3653
}
@@ -40,12 +57,16 @@ describe('@ngtools/webpack transformers', () => {
4057
`;
4158
// tslint:enable:max-line-length
4259

60+
const { program, compilerHost } = createTypescriptContext(input);
61+
4362
const shouldTransform = () => true;
4463
const getEntryModule = () =>
4564
({ path: '/project/src/app/app.module', className: 'AppModule' });
65+
const getTypeChecker = () => program.getTypeChecker();
66+
4667

4768
const transformers = [
48-
replaceBootstrap(shouldTransform, getEntryModule),
69+
replaceBootstrap(shouldTransform, getEntryModule, getTypeChecker),
4970
exportNgFactory(shouldTransform, getEntryModule),
5071
exportLazyModuleMap(shouldTransform,
5172
() => ({
@@ -54,9 +75,10 @@ describe('@ngtools/webpack transformers', () => {
5475
'./lazy2/lazy2.module.ngfactory#LazyModule2NgFactory':
5576
'/project/src/app/lazy2/lazy2.module.ngfactory.ts',
5677
})),
78+
removeDecorators(shouldTransform, getTypeChecker),
5779
];
5880

59-
const result = transformTypescript(input, transformers);
81+
const result = transformTypescript(undefined, transformers, program, compilerHost);
6082

6183
expect(oneLine`${result}`).toEqual(oneLine`${output}`);
6284
});

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

+3-3
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,8 @@ describe('@ngtools/webpack transformers', () => {
2727

2828
const { program, compilerHost } = createTypescriptContext(input);
2929
const transformer = removeDecorators(
30-
() => program.getTypeChecker(),
3130
() => true,
31+
() => program.getTypeChecker(),
3232
);
3333
const result = transformTypescript(undefined, [transformer], program, compilerHost);
3434

@@ -68,8 +68,8 @@ describe('@ngtools/webpack transformers', () => {
6868

6969
const { program, compilerHost } = createTypescriptContext(input);
7070
const transformer = removeDecorators(
71-
() => program.getTypeChecker(),
7271
() => true,
72+
() => program.getTypeChecker(),
7373
);
7474
const result = transformTypescript(undefined, [transformer], program, compilerHost);
7575

@@ -101,8 +101,8 @@ describe('@ngtools/webpack transformers', () => {
101101

102102
const { program, compilerHost } = createTypescriptContext(input);
103103
const transformer = removeDecorators(
104-
() => program.getTypeChecker(),
105104
() => true,
105+
() => program.getTypeChecker(),
106106
);
107107
const result = transformTypescript(undefined, [transformer], program, compilerHost);
108108

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

+2-16
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,13 @@
11
import * as ts from 'typescript';
22

33
import { collectDeepNodes } from './ast_helpers';
4-
import { removeImport } from './remove_import';
54
import { StandardTransform, TransformOperation, RemoveNodeOperation } from './interfaces';
65
import { makeTransform } from './make_transform';
76

87

98
export function removeDecorators(
9+
shouldTransform: (fileName: string) => boolean,
1010
getTypeChecker: () => ts.TypeChecker,
11-
shouldTransform: (fileName: string) => boolean
1211
): ts.TransformerFactory<ts.SourceFile> {
1312

1413
const standardTransform: StandardTransform = function (sourceFile: ts.SourceFile) {
@@ -23,25 +22,12 @@ export function removeDecorators(
2322
.forEach((decorator) => {
2423
// Remove the decorator node.
2524
ops.push(new RemoveNodeOperation(sourceFile, decorator));
26-
// Also remove imports for identifiers used in the decorator.
27-
// TS doesn't automatically elide imports for things removed in transformers so we have
28-
// to do it manually.
29-
collectDeepNodes<ts.Identifier>(decorator, ts.SyntaxKind.Identifier)
30-
.forEach((identifier) => {
31-
// This is finding a lot of things that might not be imports at all, but we can't
32-
// use the type checker since previous transforms might have modified things
33-
// Worst case scenario, some imports will be left over because there was another
34-
// identifier somewhere in the file as a property access or something with the same
35-
// name as the identifer we want to remove.
36-
// TODO: figure out if there's a better way to elide imports.
37-
ops.push(...removeImport(sourceFile, [identifier]));
38-
});
3925
});
4026

4127
return ops;
4228
};
4329

44-
return makeTransform(standardTransform);
30+
return makeTransform(standardTransform, getTypeChecker);
4531
}
4632

4733
function shouldRemove(decorator: ts.Decorator, typeChecker: ts.TypeChecker): boolean {

0 commit comments

Comments
 (0)