From efdf42facf98cb8252d06d74122aa5b151085013 Mon Sep 17 00:00:00 2001 From: Alan Date: Thu, 20 Dec 2018 11:46:08 +0100 Subject: [PATCH 1/3] test: add tests for elide imports This also adds the option to provide addition files when using `createTypescriptContext` this is paramount for the elide imports tests as without this certain symbols won't have the full details. Which will cause tests to be false positive and re-surface issues like #13212 --- .../test/browser/aot_spec_large.ts | 109 -------- .../webpack/src/transformers/ast_helpers.ts | 9 +- .../src/transformers/elide_imports_spec.ts | 263 ++++++++++++++++++ 3 files changed, 271 insertions(+), 110 deletions(-) create mode 100644 packages/ngtools/webpack/src/transformers/elide_imports_spec.ts diff --git a/packages/angular_devkit/build_angular/test/browser/aot_spec_large.ts b/packages/angular_devkit/build_angular/test/browser/aot_spec_large.ts index 1bbea3c2b586..86259541d359 100644 --- a/packages/angular_devkit/build_angular/test/browser/aot_spec_large.ts +++ b/packages/angular_devkit/build_angular/test/browser/aot_spec_large.ts @@ -30,113 +30,4 @@ describe('Browser Builder AOT', () => { }), ).toPromise().then(done, done.fail); }); - - it('works with aliased imports', (done) => { - const overrides = { aot: true }; - - host.writeMultipleFiles({ - 'src/app/app.component.ts': `import { Component } from '@angular/core'; - import { from as fromPromise } from 'rxjs'; - - @Component({ - selector: 'app-root', - templateUrl: './app.component.html', - styleUrls: ['./app.component.css'] - }) - export class AppComponent { - title = 'app-component'; - - constructor() { - console.log(fromPromise(Promise.resolve('test'))); - } - }`, - }); - - runTargetSpec(host, browserTargetSpec, overrides).pipe( - tap((buildEvent) => expect(buildEvent.success).toBe(true)), - tap(() => { - const fileName = join(outputPath, 'main.js'); - const content = virtualFs.fileBufferToString(host.scopedSync().read(normalize(fileName))); - // if the aliased import was dropped this won't be rewired to a webpack module. - expect(content).toMatch(/rxjs__WEBPACK_IMPORTED_.+[\"from\"]/); - expect(content).not.toContain('fromPromise'); - }), - ).toPromise().then(done, done.fail); - }); - - it('works with aliased imports from an exported object literal', (done) => { - const overrides = { aot: true }; - - host.writeMultipleFiles({ - 'src/foo.ts': ` - import { from as fromPromise } from 'rxjs'; - export { fromPromise }; - `, - 'src/app/app.component.ts': ` - import { Component } from '@angular/core'; - import { fromPromise } from '../foo'; - - @Component({ - selector: 'app-root', - templateUrl: './app.component.html', - styleUrls: ['./app.component.css'] - }) - export class AppComponent { - title = 'app-component'; - - constructor() { - console.log(fromPromise(Promise.resolve('test'))); - } - }`, - }); - - runTargetSpec(host, browserTargetSpec, overrides).pipe( - tap((buildEvent) => expect(buildEvent.success).toBe(true)), - tap(() => { - const fileName = join(outputPath, 'main.js'); - const content = virtualFs.fileBufferToString(host.scopedSync().read(normalize(fileName))); - // if the aliased import was dropped this won't be rewired to a webpack module. - expect(content).toMatch(/rxjs__WEBPACK_IMPORTED_.+[\"from\"]/); - expect(content).toMatch(/rxjs__WEBPACK_IMPORTED_.+[\"fromPromise\"]/); - }), - ).toPromise().then(done, done.fail); - }); - - it('works with aliased imports from an alias export', (done) => { - const overrides = { aot: true }; - - host.writeMultipleFiles({ - 'src/foo.ts': ` - export { from as fromPromise } from 'rxjs'; - `, - 'src/app/app.component.ts': ` - import { Component } from '@angular/core'; - import { fromPromise } from '../foo'; - - @Component({ - selector: 'app-root', - templateUrl: './app.component.html', - styleUrls: ['./app.component.css'] - }) - export class AppComponent { - title = 'app-component'; - - constructor() { - console.log(fromPromise(Promise.resolve('test'))); - } - }`, - }); - - runTargetSpec(host, browserTargetSpec, overrides).pipe( - tap((buildEvent) => expect(buildEvent.success).toBe(true)), - tap(() => { - const fileName = join(outputPath, 'main.js'); - const content = virtualFs.fileBufferToString(host.scopedSync().read(normalize(fileName))); - // if the aliased import was dropped this won't be rewired to a webpack module. - expect(content).toMatch(/rxjs__WEBPACK_IMPORTED_.+[\"from\"]/); - expect(content).toMatch(/rxjs__WEBPACK_IMPORTED_.+[\"fromPromise\"]/); - }), - ).toPromise().then(done, done.fail); - }); - }); diff --git a/packages/ngtools/webpack/src/transformers/ast_helpers.ts b/packages/ngtools/webpack/src/transformers/ast_helpers.ts index 1c894b5bbafa..57cf11e720ad 100644 --- a/packages/ngtools/webpack/src/transformers/ast_helpers.ts +++ b/packages/ngtools/webpack/src/transformers/ast_helpers.ts @@ -45,12 +45,13 @@ export function getLastNode(sourceFile: ts.SourceFile): ts.Node | null { const basePath = '/project/src/'; const fileName = basePath + 'test-file.ts'; -export function createTypescriptContext(content: string) { +export function createTypescriptContext(content: string, additionalFiles?: Record) { // Set compiler options. const compilerOptions: ts.CompilerOptions = { noEmitOnError: false, allowJs: true, newLine: ts.NewLineKind.LineFeed, + moduleResolution: ts.ModuleResolutionKind.NodeJs, target: ts.ScriptTarget.ESNext, skipLibCheck: true, sourceMap: false, @@ -68,6 +69,12 @@ export function createTypescriptContext(content: string) { // Add a dummy file to host content. compilerHost.writeFile(fileName, content, false); + if (additionalFiles) { + for (const key in additionalFiles) { + compilerHost.writeFile(basePath + key, additionalFiles[key], false); + } + } + // Create the TypeScript program. const program = ts.createProgram([fileName], compilerOptions, compilerHost); diff --git a/packages/ngtools/webpack/src/transformers/elide_imports_spec.ts b/packages/ngtools/webpack/src/transformers/elide_imports_spec.ts new file mode 100644 index 000000000000..07ae53b5b173 --- /dev/null +++ b/packages/ngtools/webpack/src/transformers/elide_imports_spec.ts @@ -0,0 +1,263 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ +// tslint:disable:no-big-function +import { tags } from '@angular-devkit/core'; // tslint:disable-line:no-implicit-dependencies +import * as ts from 'typescript'; +import { createTypescriptContext, getLastNode, transformTypescript } from './ast_helpers'; +import { RemoveNodeOperation } from './interfaces'; +import { makeTransform } from './make_transform'; + +describe('@ngtools/webpack transformers', () => { + describe('elide_imports', () => { + + const dummyNode = `const remove = ''`; + + const transformer = (program: ts.Program) => ( + makeTransform( + (sourceFile: ts.SourceFile) => + [new RemoveNodeOperation(sourceFile, getLastNode(sourceFile) as ts.Node)], + () => program.getTypeChecker(), + ) + ); + + const additionalFiles: Record = { + 'const.ts': ` + export const promise = () => null; + export const take = () => null; + export default promise; + `, + }; + + it('should remove unused aliased imports', () => { + const input = tags.stripIndent` + import { promise as fromPromise } from './const'; + const unused = fromPromise; + `; + + const { program, compilerHost } = createTypescriptContext(input, additionalFiles); + const result = transformTypescript(undefined, [transformer(program)], program, compilerHost); + + expect(tags.oneLine`${result}`).toEqual(''); + }); + + it('should retain used aliased imports', () => { + const input = tags.stripIndent` + import { promise as fromPromise } from './const'; + const used = fromPromise; + + ${dummyNode} + `; + + const output = tags.stripIndent` + import { promise as fromPromise } from './const'; + const used = fromPromise; + `; + + const { program, compilerHost } = createTypescriptContext(input, additionalFiles); + const result = transformTypescript(undefined, [transformer(program)], program, compilerHost); + + expect(tags.oneLine`${result}`).toEqual(tags.oneLine`${output}`); + }); + + it('should retain used namespaced imports', () => { + const input = tags.stripIndent` + import * as namespaced from './const'; + const used = namespaced; + + ${dummyNode} + `; + + const output = tags.stripIndent` + import * as namespaced from './const'; + const used = namespaced; + `; + + const { program, compilerHost } = createTypescriptContext(input, additionalFiles); + const result = transformTypescript(undefined, [transformer(program)], program, compilerHost); + + expect(tags.oneLine`${result}`).toEqual(tags.oneLine`${output}`); + }); + + it('should drop unused namespaced imports', () => { + const input = tags.stripIndent` + import * as namespaced from './const'; + const used = namespaced; + `; + + const { program, compilerHost } = createTypescriptContext(input, additionalFiles); + const result = transformTypescript(undefined, [transformer(program)], program, compilerHost); + + expect(tags.oneLine`${result}`).toEqual(''); + }); + + it('should drop unused imports in export specifier', () => { + const input = tags.stripIndent` + import { promise as fromPromise } from './const'; + export { fromPromise }; + `; + + const { program, compilerHost } = createTypescriptContext(input, additionalFiles); + const result = transformTypescript(undefined, [transformer(program)], program, compilerHost); + + expect(tags.oneLine`${result}`).toEqual(''); + }); + + it('should retain used imports in export specifier', () => { + const input = tags.stripIndent` + import { promise as fromPromise } from './const'; + export { fromPromise }; + + ${dummyNode} + `; + + const output = tags.stripIndent` + import { promise as fromPromise } from './const'; + export { fromPromise }; + `; + + const { program, compilerHost } = createTypescriptContext(input, additionalFiles); + const result = transformTypescript(undefined, [transformer(program)], program, compilerHost); + + expect(tags.oneLine`${result}`).toEqual(tags.oneLine`${output}`); + }); + + it('should drop unused in shorthand property assignment', () => { + const input = tags.stripIndent` + import { promise as fromPromise } from './const'; + const unused = { fromPromise }; + `; + + const { program, compilerHost } = createTypescriptContext(input, additionalFiles); + const result = transformTypescript(undefined, [transformer(program)], program, compilerHost); + + expect(tags.oneLine`${result}`).toEqual(''); + }); + + it('should retain used imports in shorthand property assignment', () => { + const input = tags.stripIndent` + import { promise as fromPromise } from './const'; + const used = { fromPromise }; + + ${dummyNode} + `; + + const output = tags.stripIndent` + import { promise as fromPromise } from './const'; + const used = { fromPromise }; + `; + + const { program, compilerHost } = createTypescriptContext(input, additionalFiles); + const result = transformTypescript(undefined, [transformer(program)], program, compilerHost); + + expect(tags.oneLine`${result}`).toEqual(tags.oneLine`${output}`); + }); + + it('should drop unused default import', () => { + const input = tags.stripIndent` + import defaultPromise from './const'; + const unused = defaultPromise; + `; + + const { program, compilerHost } = createTypescriptContext(input, additionalFiles); + const result = transformTypescript(undefined, [transformer(program)], program, compilerHost); + + expect(tags.oneLine`${result}`).toEqual(''); + }); + + it('should retain used default import', () => { + const input = tags.stripIndent` + import defaultPromise from './const'; + const used = defaultPromise; + + ${dummyNode} + `; + + const output = tags.stripIndent` + import defaultPromise from './const'; + const used = defaultPromise; + `; + + const { program, compilerHost } = createTypescriptContext(input, additionalFiles); + const result = transformTypescript(undefined, [transformer(program)], program, compilerHost); + + expect(tags.oneLine`${result}`).toEqual(tags.oneLine`${output}`); + }); + + it('should only drop unused default imports when named and default', () => { + const input = tags.stripIndent` + import promise, { promise as fromPromise } from './const'; + const used = fromPromise; + const unused = promise; + `; + + const output = tags.stripIndent` + import { promise as fromPromise } from './const'; + const used = fromPromise; + `; + + const { program, compilerHost } = createTypescriptContext(input, additionalFiles); + const result = transformTypescript(undefined, [transformer(program)], program, compilerHost); + + expect(tags.oneLine`${result}`).toEqual(tags.oneLine`${output}`); + }); + + it('should only drop unused named imports when named and default', () => { + const input = tags.stripIndent` + import promise, { promise as fromPromise, take } from './const'; + const used = fromPromise; + const unused = promise; + `; + + const output = tags.stripIndent` + import { promise as fromPromise } from './const'; + const used = fromPromise; + `; + + const { program, compilerHost } = createTypescriptContext(input, additionalFiles); + const result = transformTypescript(undefined, [transformer(program)], program, compilerHost); + + expect(tags.oneLine`${result}`).toEqual(tags.oneLine`${output}`); + }); + + it('should only drop default imports when having named and default', () => { + const input = tags.stripIndent` + import promise, { fromPromise } from './const'; + const used = promise; + const unused = fromPromise; + `; + + const output = tags.stripIndent` + import promise from './const'; + const used = promise; + `; + + const { program, compilerHost } = createTypescriptContext(input, additionalFiles); + const result = transformTypescript(undefined, [transformer(program)], program, compilerHost); + + expect(tags.oneLine`${result}`).toEqual(tags.oneLine`${output}`); + }); + + it('should retain import clause', () => { + const input = tags.stripIndent` + import './const'; + + ${dummyNode} + `; + + const output = tags.stripIndent` + import './const'; + `; + + const { program, compilerHost } = createTypescriptContext(input, additionalFiles); + const result = transformTypescript(undefined, [transformer(program)], program, compilerHost); + + expect(tags.oneLine`${result}`).toEqual(tags.oneLine`${output}`); + }); + + }); +}); From f4726d71cef089636c5e66496b74b0fcf24b0071 Mon Sep 17 00:00:00 2001 From: Alan Date: Thu, 20 Dec 2018 12:23:07 +0100 Subject: [PATCH 2/3] fix(@ngtools/webpack): drop only unused default import when used with named imports At the moment when having a default import together with a named import example: ``` import abc, { def } from './foo'; ``` And the default import becomes unused it will drop the entire node which will caused used named imports to be dropped as well. --- .../webpack/src/transformers/elide_imports.ts | 49 +++++++++++++------ 1 file changed, 33 insertions(+), 16 deletions(-) diff --git a/packages/ngtools/webpack/src/transformers/elide_imports.ts b/packages/ngtools/webpack/src/transformers/elide_imports.ts index 89fb55a8f88e..ca4d88a525bc 100644 --- a/packages/ngtools/webpack/src/transformers/elide_imports.ts +++ b/packages/ngtools/webpack/src/transformers/elide_imports.ts @@ -31,7 +31,7 @@ export function elideImports( // Collect all imports and used identifiers const specialCaseNames = new Set(); const usedSymbols = new Set(); - const imports = [] as ts.ImportDeclaration[]; + const imports: ts.ImportDeclaration[] = []; ts.forEachChild(sourceFile, function visit(node) { // Skip removed nodes if (removedNodes.includes(node)) { @@ -84,28 +84,45 @@ export function elideImports( continue; } - if (node.importClause.name) { - // "import XYZ from 'abc';" - if (isUnused(node.importClause.name)) { - ops.push(new RemoveNodeOperation(sourceFile, node)); - } - } else if (node.importClause.namedBindings - && ts.isNamespaceImport(node.importClause.namedBindings)) { + const namedBindings = node.importClause.namedBindings; + + if (namedBindings && ts.isNamespaceImport(namedBindings)) { // "import * as XYZ from 'abc';" - if (isUnused(node.importClause.namedBindings.name)) { + if (isUnused(namedBindings.name)) { ops.push(new RemoveNodeOperation(sourceFile, node)); } - } else if (node.importClause.namedBindings - && ts.isNamedImports(node.importClause.namedBindings)) { - // "import { XYZ, ... } from 'abc';" + } else { const specifierOps = []; - for (const specifier of node.importClause.namedBindings.elements) { - if (isUnused(specifier.name)) { - specifierOps.push(new RemoveNodeOperation(sourceFile, specifier)); + let clausesCount = 0; + + // "import { XYZ, ... } from 'abc';" + if (namedBindings && ts.isNamedImports(namedBindings)) { + let removedClausesCount = 0; + clausesCount += namedBindings.elements.length; + + for (const specifier of namedBindings.elements) { + if (isUnused(specifier.name)) { + removedClausesCount++; + // in case we don't have any more namedImports we should remove the parent ie the {} + const nodeToRemove = clausesCount === removedClausesCount + ? specifier.parent + : specifier; + + specifierOps.push(new RemoveNodeOperation(sourceFile, nodeToRemove)); + } + } + } + + // "import XYZ from 'abc';" + if (node.importClause.name) { + clausesCount++; + + if (isUnused(node.importClause.name)) { + specifierOps.push(new RemoveNodeOperation(sourceFile, node.importClause.name)); } } - if (specifierOps.length === node.importClause.namedBindings.elements.length) { + if (specifierOps.length === clausesCount) { ops.push(new RemoveNodeOperation(sourceFile, node)); } else { ops.push(...specifierOps); From 18e461370c5a5945262b39563b5bef07a67c5dfa Mon Sep 17 00:00:00 2001 From: Alan Date: Thu, 20 Dec 2018 15:45:22 +0100 Subject: [PATCH 3/3] refactor: use `getExportSpecifierLocalTargetSymbol` and `getShorthandAssignmentValueSymbol` to handle symbols lookups Use typechecker methods to handle special cases for `ExportSpecifier` and `ShorthandPropertyAssignment` --- .../webpack/src/transformers/elide_imports.ts | 34 +++++++++---------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/packages/ngtools/webpack/src/transformers/elide_imports.ts b/packages/ngtools/webpack/src/transformers/elide_imports.ts index ca4d88a525bc..c7b2cd10654e 100644 --- a/packages/ngtools/webpack/src/transformers/elide_imports.ts +++ b/packages/ngtools/webpack/src/transformers/elide_imports.ts @@ -29,9 +29,9 @@ export function elideImports( const typeChecker = getTypeChecker(); // Collect all imports and used identifiers - const specialCaseNames = new Set(); const usedSymbols = new Set(); const imports: ts.ImportDeclaration[] = []; + ts.forEachChild(sourceFile, function visit(node) { // Skip removed nodes if (removedNodes.includes(node)) { @@ -45,20 +45,22 @@ export function elideImports( return; } - if (ts.isIdentifier(node)) { - const symbol = typeChecker.getSymbolAtLocation(node); - if (symbol) { - usedSymbols.add(symbol); - } - } else if (ts.isExportSpecifier(node)) { - // Export specifiers return the non-local symbol from the above - // so check the name string instead - specialCaseNames.add((node.propertyName || node.name).text); + let symbol: ts.Symbol | undefined; + + switch (node.kind) { + case ts.SyntaxKind.Identifier: + symbol = typeChecker.getSymbolAtLocation(node); + break; + case ts.SyntaxKind.ExportSpecifier: + symbol = typeChecker.getExportSpecifierLocalTargetSymbol(node as ts.ExportSpecifier); + break; + case ts.SyntaxKind.ShorthandPropertyAssignment: + symbol = typeChecker.getShorthandAssignmentValueSymbol(node); + break; + } - return; - } else if (ts.isShorthandPropertyAssignment(node)) { - // Shorthand property assignments return the object property's symbol not the import's - specialCaseNames.add(node.name.text); + if (symbol) { + usedSymbols.add(symbol); } ts.forEachChild(node, visit); @@ -69,10 +71,6 @@ export function elideImports( } const isUnused = (node: ts.Identifier) => { - if (specialCaseNames.has(node.text)) { - return false; - } - const symbol = typeChecker.getSymbolAtLocation(node); return symbol && !usedSymbols.has(symbol);