Skip to content

Add tests for elide imports and fix named imports with default imports #13243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 20, 2018
Merged

Add tests for elide imports and fix named imports with default imports #13243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
efdf42f
test: add tests for elide imports
Dec 20, 2018
f4726d7
fix(@ngtools/webpack): drop only unused default import when used with…
Dec 20, 2018
18e4613
refactor: use `getExportSpecifierLocalTargetSymbol` and `getShorthand…
Dec 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 0 additions & 109 deletions packages/angular_devkit/build_angular/test/browser/aot_spec_large.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

});
9 changes: 8 additions & 1 deletion packages/ngtools/webpack/src/transformers/ast_helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, string>) {
// 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,
Expand All @@ -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);

Expand Down
83 changes: 49 additions & 34 deletions packages/ngtools/webpack/src/transformers/elide_imports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ export function elideImports(
const typeChecker = getTypeChecker();

// Collect all imports and used identifiers
const specialCaseNames = new Set<string>();
const usedSymbols = new Set<ts.Symbol>();
const imports = [] as ts.ImportDeclaration[];
const imports: ts.ImportDeclaration[] = [];

ts.forEachChild(sourceFile, function visit(node) {
// Skip removed nodes
if (removedNodes.includes(node)) {
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -84,28 +82,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);
Expand Down
Loading