Skip to content

Commit 3a5d342

Browse files
devversionjelbourn
authored andcommitted
refactor: fix v8 material imports update rule (#15594)
* Fixes that the V8 `updateAngularMaterialImportsRule` does currently not work (due to incorrect resolution of the value declaration source file) * Fixes that aliased imports are transformed into non-alias imports (e.g. `import {a as b} from ..` turns into `import {a} from ...`) * Fixes that inline comments within the module specifiers are omitted. * Adds a test-case that ensures that the rule properly works * Slightly refactors the test-case setup so that we can modify the test-case temporary dirctory before running the fixers.
1 parent beae208 commit 3a5d342

File tree

9 files changed

+169
-81
lines changed

9 files changed

+169
-81
lines changed

src/cdk/schematics/ng-update/test-cases/misc/method-call-checks.spec.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
11
import {migrationCollection} from '../index.spec';
2-
import {runTestCases} from '../../../testing';
2+
import {createTestCaseSetup} from '../../../testing';
33

44
describe('v6 method call checks', () => {
55

66
it('should properly report invalid method calls', async () => {
7-
const {logOutput, removeTempDir} = await runTestCases('migration-v6', migrationCollection,
7+
const {runFixers, removeTempDir} = createTestCaseSetup('migration-v6', migrationCollection,
88
[require.resolve('./method-call-checks_input.ts')]);
99

10+
const {logOutput} = await runFixers();
11+
1012
expect(logOutput)
1113
.toMatch(/:15.*Found call to "FocusMonitor\.monitor".*renderer.*has been removed/);
1214
expect(logOutput)

src/cdk/schematics/testing/test-case-setup.ts

Lines changed: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import * as virtualFs from '@angular-devkit/core/src/virtual-fs/host';
1212
import {SchematicTestRunner} from '@angular-devkit/schematics/testing';
1313
import {mkdirpSync, readFileSync, writeFileSync, removeSync} from 'fs-extra';
1414
import {sync as globSync} from 'glob';
15-
import {dirname, join, basename, relative, sep} from 'path';
15+
import {dirname, join, extname, basename, relative, sep} from 'path';
1616
import {createTestApp, runPostScheduledTasks} from '../testing';
1717

1818
/** Suffix that indicates whether a given file is a test case input. */
@@ -44,7 +44,7 @@ export function createFileSystemTestApp(runner: SchematicTestRunner) {
4444
return {appTree, tempPath, removeTempDir: () => removeSync(tempPath)};
4545
}
4646

47-
export async function runTestCases(migrationName: string, collectionPath: string,
47+
export function createTestCaseSetup(migrationName: string, collectionPath: string,
4848
inputFiles: string[]) {
4949

5050
const runner = new SchematicTestRunner('schematics', collectionPath);
@@ -58,27 +58,31 @@ export async function runTestCases(migrationName: string, collectionPath: string
5858
// Write each test-case input to the file-system. This is necessary because otherwise
5959
// TSLint won't be able to pick up the test cases.
6060
inputFiles.forEach(inputFilePath => {
61-
const inputTestName = basename(inputFilePath);
61+
const inputTestName = basename(inputFilePath, extname(inputFilePath));
6262
const tempInputPath = join(tempPath, `projects/cdk-testing/src/test-cases/${inputTestName}.ts`);
6363

6464
mkdirpSync(dirname(tempInputPath));
6565
writeFileSync(tempInputPath, readFileContent(inputFilePath));
6666
});
6767

68-
runner.runSchematic(migrationName, {}, appTree);
68+
const runFixers = async function() {
69+
runner.runSchematic(migrationName, {}, appTree);
6970

70-
// Switch to the new temporary directory because otherwise TSLint cannot read the files.
71-
process.chdir(tempPath);
71+
// Switch to the new temporary directory because otherwise TSLint cannot read the files.
72+
process.chdir(tempPath);
7273

73-
// Run the scheduled TSLint fix task from the update schematic. This task is responsible for
74-
// identifying outdated code parts and performs the fixes. Since tasks won't run automatically
75-
// within a `SchematicTestRunner`, we manually need to run the scheduled task.
76-
await runPostScheduledTasks(runner, 'tslint-fix').toPromise();
74+
// Run the scheduled TSLint fix task from the update schematic. This task is responsible for
75+
// identifying outdated code parts and performs the fixes. Since tasks won't run automatically
76+
// within a `SchematicTestRunner`, we manually need to run the scheduled task.
77+
await runPostScheduledTasks(runner, 'tslint-fix').toPromise();
7778

78-
// Switch back to the initial working directory.
79-
process.chdir(initialWorkingDir);
79+
// Switch back to the initial working directory.
80+
process.chdir(initialWorkingDir);
8081

81-
return {tempPath, logOutput, removeTempDir};
82+
return {logOutput};
83+
};
84+
85+
return {tempPath, removeTempDir, runFixers};
8286
}
8387

8488
/**
@@ -144,8 +148,10 @@ export function defineJasmineTestCases(versionName: string, collectionFile: stri
144148
let cleanupTestApp: () => void;
145149

146150
beforeAll(async () => {
147-
const {tempPath, removeTempDir} =
148-
await runTestCases(`migration-${versionName}`, collectionFile, inputFiles);
151+
const {tempPath, runFixers, removeTempDir} =
152+
createTestCaseSetup(`migration-${versionName}`, collectionFile, inputFiles);
153+
154+
await runFixers();
149155

150156
testCasesOutputPath = join(tempPath, 'projects/cdk-testing/src/test-cases/');
151157
cleanupTestApp = removeTempDir;
@@ -156,7 +162,7 @@ export function defineJasmineTestCases(versionName: string, collectionFile: stri
156162
// Iterates through every test case directory and generates a jasmine test block that will
157163
// verify that the update schematics properly updated the test input to the expected output.
158164
inputFiles.forEach(inputFile => {
159-
const inputTestName = basename(inputFile);
165+
const inputTestName = basename(inputFile, extname(inputFile));
160166

161167
it(`should apply update schematics to test case: ${inputTestName}`, () => {
162168
expect(readFileContent(join(testCasesOutputPath, `${inputTestName}.ts`)))

src/lib/schematics/BUILD.bazel

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,10 @@ ts_library(
5555
"//src/cdk/schematics/testing",
5656
"@npm//@schematics/angular",
5757
"@npm//@angular-devkit/schematics",
58-
"@npm//@types/jasmine",
58+
"@npm//@types/fs-extra",
5959
"@npm//@types/node",
60+
"@npm//@types/jasmine",
61+
"@npm//fs-extra",
6062
],
6163
tsconfig = ":tsconfig.json",
6264
testonly = True,

src/lib/schematics/ng-update/test-cases/misc/constructor-checks.spec.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
import {migrationCollection} from '../index.spec';
2-
import {runTestCases} from '@angular/cdk/schematics/testing';
2+
import {createTestCaseSetup} from '@angular/cdk/schematics/testing';
33

44
describe('constructor checks', () => {
55

66
it('should properly report invalid constructor expression signatures', async () => {
7-
const {logOutput, removeTempDir} = await runTestCases('migration-v6', migrationCollection,
8-
[require.resolve('./constructor-checks_input.ts')]);
7+
const {removeTempDir, runFixers} = createTestCaseSetup('migration-v6',
8+
migrationCollection, [require.resolve('./constructor-checks_input.ts')]);
9+
10+
const {logOutput} = await runFixers();
911

1012
expect(logOutput).toMatch(/:22.*Found "NativeDateAdapter"/,
1113
'Expected the constructor checks to report if an argument is not assignable.');

src/lib/schematics/ng-update/test-cases/misc/import-checks.spec.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
1-
import {runTestCases} from '@angular/cdk/schematics/testing';
1+
import {createTestCaseSetup} from '@angular/cdk/schematics/testing';
22
import {migrationCollection} from '../index.spec';
33

44
describe('v6 import misc checks', () => {
55

66
it('should report imports for deleted animation constants', async () => {
7-
const {logOutput, removeTempDir} = await runTestCases('migration-v6', migrationCollection,
8-
[require.resolve('./import-checks_input.ts')]);
7+
const {removeTempDir, runFixers} = createTestCaseSetup('migration-v6',
8+
migrationCollection, [require.resolve('./import-checks_input.ts')]);
9+
10+
const {logOutput} = await runFixers();
911

1012
expect(logOutput).toMatch(/Found deprecated symbol "SHOW_ANIMATION"/);
1113
expect(logOutput).toMatch(/Found deprecated symbol "HIDE_ANIMATION"/);
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
import {createTestCaseSetup, readFileContent} from '@angular/cdk/schematics/testing';
2+
import {migrationCollection} from '../index.spec';
3+
import {writeFileSync, mkdirpSync} from 'fs-extra';
4+
import {join} from 'path';
5+
6+
describe('v8 material imports', () => {
7+
8+
function writeSecondaryEntryPoint(materialPath: string, name: string) {
9+
const entryPointPath = join(materialPath, name);
10+
mkdirpSync(entryPointPath);
11+
writeFileSync(join(entryPointPath, 'index.d.ts'), `export const ${name} = '';`);
12+
}
13+
14+
it('should report imports for deleted animation constants', async () => {
15+
const {runFixers, removeTempDir, tempPath} = createTestCaseSetup(
16+
'migration-v8', migrationCollection, [require.resolve('./material-imports_input.ts')]);
17+
const materialPath = join(tempPath, 'node_modules/@angular/material');
18+
19+
mkdirpSync(materialPath);
20+
writeFileSync(join(materialPath, 'index.d.ts'), `
21+
export * from './a';
22+
export * from './b';
23+
export * from './c';
24+
`);
25+
26+
writeSecondaryEntryPoint(materialPath, 'a');
27+
writeSecondaryEntryPoint(materialPath, 'b');
28+
writeSecondaryEntryPoint(materialPath, 'c');
29+
30+
await runFixers();
31+
32+
const outputPath = join(tempPath,
33+
'projects/cdk-testing/src/test-cases/material-imports_input.ts');
34+
35+
expect(readFileContent(outputPath)).toBe(
36+
readFileContent(require.resolve('./material-imports_expected_output.ts')));
37+
38+
removeTempDir();
39+
});
40+
});
41+
42+
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
import { c as c2 } from "@angular/material/c";
2+
import { a } from "@angular/material/a";
3+
import { b } from "@angular/material/b";
4+
import { c } from "@angular/material/c";
5+
import { a as a2 } from "@angular/material/a";
6+
7+
// unsorted
8+
import { a } from "@angular/material/a";
9+
import { b } from "@angular/material/b";
10+
import { c } from "@angular/material/c";
11+
12+
import { /* comment */ a as a3 } from "@angular/material/a";
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
import {c as c2} from '@angular/material';
2+
import {a, b, c} from '@angular/material';
3+
import {a as a2} from '@angular/material';
4+
5+
// unsorted
6+
import {c, b, a} from '@angular/material';
7+
8+
import {/* comment */ a as a3} from '@angular/material';

src/lib/schematics/ng-update/upgrade-rules/package-imports-v8/updateAngularMaterialImportsRule.ts

Lines changed: 68 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import * as ts from 'typescript';
1111
import {materialModuleSpecifier} from '../../../ng-update/typescript/module-specifiers';
1212

1313
/**
14-
* Regex for testing file paths against to determinte if the file is from the
14+
* Regex for testing file paths against to determine if the file is from the
1515
* Angular Material library.
1616
*/
1717
const ANGULAR_MATERIAL_FILEPATH_REGEX = new RegExp(`${materialModuleSpecifier}/(.*?)/`);
@@ -51,17 +51,20 @@ export class Rule extends Lint.Rules.TypedRule {
5151

5252
/**
5353
* A walker to walk a given source file to check for imports from the
54-
* @angular/material module.
54+
* "@angular/material" module.
5555
*/
5656
function walk(ctx: Lint.WalkContext<boolean>, checker: ts.TypeChecker): void {
5757
// The source file to walk.
5858
const sf = ctx.sourceFile;
59+
const printer = ts.createPrinter();
5960
const cb = (declaration: ts.Node) => {
6061
// Only look at import declarations.
61-
if (!ts.isImportDeclaration(declaration)) {
62+
if (!ts.isImportDeclaration(declaration) ||
63+
!ts.isStringLiteralLike(declaration.moduleSpecifier)) {
6264
return;
6365
}
64-
const importLocation = declaration.moduleSpecifier.getText(sf);
66+
67+
const importLocation = declaration.moduleSpecifier.text;
6568
// If the import module is not @angular/material, skip check.
6669
if (importLocation !== materialModuleSpecifier) {
6770
return;
@@ -85,86 +88,95 @@ function walk(ctx: Lint.WalkContext<boolean>, checker: ts.TypeChecker): void {
8588
return ctx.addFailureAtNode(declaration, Rule.NO_IMPORT_NAMED_SYMBOLS_FAILURE_STR);
8689
}
8790

88-
// Map of submodule locations to arrays of imported symbols.
89-
const importMap = new Map<string, Set<string>>();
91+
// Map which consists of secondary entry-points and import specifiers which are used
92+
// within the current import declaration.
93+
const importMap = new Map<string, ts.ImportSpecifier[]>();
9094

9195
// Determine the subpackage each symbol in the namedBinding comes from.
9296
for (const element of declaration.importClause.namedBindings.elements) {
93-
// Confirm the named import is a symbol that can be looked up.
97+
// Ensure the import specifier name is statically analyzable.
9498
if (!ts.isIdentifier(element.name)) {
95-
return ctx.addFailureAtNode(
96-
element, element.getFullText(sf) + Rule.SYMBOL_NOT_FOUND_FAILURE_STR);
99+
return ctx.addFailureAtNode(element, element.getText() + Rule.SYMBOL_NOT_FOUND_FAILURE_STR);
97100
}
98-
// Get the type for the named binding element.
99-
const type = checker.getTypeAtLocation(element.name);
100-
// Get the original symbol where it is declared upstream.
101-
const symbol = type.getSymbol();
101+
102+
// Get the symbol for the named binding element. Note that we cannot determine the
103+
// value declaration based on the type of the element as types are not necessarily
104+
// specific to a given secondary entry-point (e.g. exports with the type of "string")
105+
// would resolve to the module types provided by TypeScript itself.
106+
const symbol = getDeclarationSymbolOfNode(element.name, checker);
102107

103108
// If the symbol can't be found, add failure saying the symbol
104109
// can't be found.
105110
if (!symbol || !symbol.valueDeclaration) {
106-
return ctx.addFailureAtNode(
107-
element, element.getFullText(sf) + Rule.SYMBOL_NOT_FOUND_FAILURE_STR);
108-
}
109-
110-
// If the symbol has no declarations, add failure saying the symbol can't
111-
// be found.
112-
if (!symbol.declarations || !symbol.declarations.length) {
113-
return ctx.addFailureAtNode(
114-
element, element.getFullText(sf) + Rule.SYMBOL_NOT_FOUND_FAILURE_STR);
111+
return ctx.addFailureAtNode(element, element.getText() + Rule.SYMBOL_NOT_FOUND_FAILURE_STR);
115112
}
116113

117114
// The filename for the source file of the node that contains the
118-
// first declaration of the symbol. All symbol declarations must be
115+
// first declaration of the symbol. All symbol declarations must be
119116
// part of a defining node, so parent can be asserted to be defined.
120-
const sourceFile = symbol.valueDeclaration.getSourceFile().fileName;
117+
const sourceFile: string = symbol.valueDeclaration.getSourceFile().fileName;
118+
121119
// File the module the symbol belongs to from a regex match of the
122-
// filename. This will always match since only @angular/material symbols
123-
// are being looked at.
124-
const [, moduleName] = sourceFile.match(ANGULAR_MATERIAL_FILEPATH_REGEX) || [] as undefined[];
125-
if (!moduleName) {
120+
// filename. This will always match since only "@angular/material"
121+
// elements are analyzed.
122+
const matches = sourceFile.match(ANGULAR_MATERIAL_FILEPATH_REGEX);
123+
if (!matches) {
126124
return ctx.addFailureAtNode(
127-
element, element.getFullText(sf) + Rule.SYMBOL_FILE_NOT_FOUND_FAILURE_STR);
125+
element, element.getText() + Rule.SYMBOL_FILE_NOT_FOUND_FAILURE_STR);
128126
}
129-
// The module name where the symbol is defined e.g. card, dialog. The
127+
const [, moduleName] = matches;
128+
129+
// The module name where the symbol is defined e.g. card, dialog. The
130130
// first capture group is contains the module name.
131131
if (importMap.has(moduleName)) {
132-
importMap.get(moduleName)!.add(symbol.getName());
132+
importMap.get(moduleName)!.push(element);
133133
} else {
134-
importMap.set(moduleName, new Set([symbol.getName()]));
134+
importMap.set(moduleName, [element]);
135135
}
136136
}
137-
const fix = buildSecondaryImportStatements(importMap);
138137

139-
// Without a fix to provide, show error message only.
140-
if (!fix) {
138+
// Transforms the import declaration into multiple import declarations that import
139+
// the given symbols from the individual secondary entry-points. For example:
140+
// import {MatCardModule, MatCardTitle} from '@angular/material/card';
141+
// import {MatRadioModule} from '@angular/material/radio';
142+
const newImportStatements =
143+
Array.from(importMap.entries())
144+
.sort()
145+
.map(([name, elements]) => {
146+
const newImport = ts.createImportDeclaration(
147+
undefined, undefined,
148+
ts.createImportClause(undefined, ts.createNamedImports(elements)),
149+
ts.createStringLiteral(`${materialModuleSpecifier}/${name}`));
150+
return printer.printNode(ts.EmitHint.Unspecified, newImport, sf);
151+
})
152+
.join('\n');
153+
154+
// Without any import statements that were generated, we can assume that this was an empty
155+
// import declaration. We still want to add a failure in order to make developers aware that
156+
// importing from "@angular/material" is deprecated.
157+
if (!newImportStatements) {
141158
return ctx.addFailureAtNode(declaration.moduleSpecifier, Rule.ONLY_SUBPACKAGE_FAILURE_STR);
142159
}
143-
// Mark the lint failure at the module specifier, providing a
144-
// recommended fix.
160+
161+
// Mark the lint failure at the module specifier, providing the replacement that switches
162+
// the import to individual secondary entry-point imports.
145163
ctx.addFailureAtNode(
146164
declaration.moduleSpecifier, Rule.ONLY_SUBPACKAGE_FAILURE_STR,
147-
new Lint.Replacement(declaration.getStart(sf), declaration.getWidth(), fix));
165+
new Lint.Replacement(declaration.getStart(), declaration.getWidth(), newImportStatements));
148166
};
167+
149168
sf.statements.forEach(cb);
150169
}
151170

152-
/**
153-
* Builds the recommended fix from a map of the imported symbols found in the
154-
* import declaration. Imports declarations are sorted by module, then by
155-
* symbol within each import declaration.
156-
*
157-
* Example of the format:
158-
*
159-
* import {MatCardModule, MatCardTitle} from '@angular/material/card';
160-
* import {MatRadioModule} from '@angular/material/radio';
161-
*/
162-
function buildSecondaryImportStatements(importMap: Map<string, Set<string>>): string {
163-
return Array.from(importMap.entries())
164-
.sort((a, b) => a[0] > b[0] ? 1 : -1)
165-
.map(entry => {
166-
const imports = Array.from(entry[1]).sort((a, b) => a > b ? 1 : -1).join(', ');
167-
return `import {${imports}} from '@angular/material/${entry[0]}';\n`;
168-
})
169-
.join('');
171+
/** Gets the symbol that contains the value declaration of the given node. */
172+
function getDeclarationSymbolOfNode(node: ts.Node, checker: ts.TypeChecker) {
173+
const symbol = checker.getSymbolAtLocation(node);
174+
175+
// Symbols can be aliases of the declaration symbol. e.g. in named import specifiers.
176+
// We need to resolve the aliased symbol back to the declaration symbol.
177+
// tslint:disable-next-line:no-bitwise
178+
if (symbol && (symbol.flags & ts.SymbolFlags.Alias) !== 0) {
179+
return checker.getAliasedSymbol(symbol);
180+
}
181+
return symbol;
170182
}

0 commit comments

Comments
 (0)