Skip to content

Commit a5437cd

Browse files
devversionjelbourn
authored andcommitted
refactor(ng-update): handle namespaced angular decorators in update-tool (#17737)
Previously Angular did not support namespaced Angular decorators. e.g. ``` @core.Component({...}) @core.Directive({...}) ``` Since this is now supported in ngtsc, we want to handle such cases where stylesheets, templates are referenced in namespaced decorators. This can be achieved by moving the more advanced import resolution logic made for the Hammer gesture migration to the `cdk/update-tool`. Exposing these for other CDK and Material migrations can be helpful anyway. This also unveiled that we did not run non-misc test cases automatically. This will be fixed as part of this commit.
1 parent 36ab4d2 commit a5437cd

16 files changed

+139
-162
lines changed

src/cdk/schematics/ng-update/test-cases/v6/attribute-selectors_expected_output.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import {Component} from '@angular/core';
2+
import * as core from '@angular/core';
23
import {By} from '@angular/platform-browser';
34

45
const a = By.css('[cdkPortalOutlet]');
@@ -9,7 +10,7 @@ const b = By.css('[cdkPortalOutlet]');
910
const c = 'cdkPortalHost';
1011
const d = 'portalHost';
1112

12-
@Component({
13+
@core.Component({
1314
template: `
1415
<div cdkPortalOutlet="E"></div>
1516
<div [cdkPortalOutlet]="myPortal"></div>
@@ -31,4 +32,4 @@ class E {}
3132
'div[cdkPortalOutlet] {color: blue}'
3233
]
3334
})
34-
class F {}
35+
class F {}

src/cdk/schematics/ng-update/test-cases/v6/attribute-selectors_input.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import {Component} from '@angular/core';
2+
import * as core from '@angular/core';
23
import {By} from '@angular/platform-browser';
34

45
const a = By.css('[cdkPortalHost]');
@@ -9,7 +10,7 @@ const b = By.css('[portalHost]');
910
const c = 'cdkPortalHost';
1011
const d = 'portalHost';
1112

12-
@Component({
13+
@core.Component({
1314
template: `
1415
<div cdkPortalHost="E"></div>
1516
<div [cdkPortalHost]="myPortal"></div>
@@ -31,4 +32,4 @@ class E {}
3132
'div[portalHost] {color: blue}'
3233
]
3334
})
34-
class F {}
35+
class F {}

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

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@ const TEST_CASE_INPUT_SUFFIX = '_input.ts';
2323
/** Suffix that indicates whether a given file is an expected output of a test case. */
2424
const TEST_CASE_OUTPUT_SUFFIX = '_expected_output.ts';
2525

26+
/** Name of the folder that can contain test case files which should not run automatically. */
27+
const MISC_FOLDER_NAME = 'misc';
28+
2629
/** Reads the UTF8 content of the specified file. Normalizes the path and ensures that */
2730
export function readFileContent(filePath: string): string {
2831
return readFileSync(filePath, 'utf8');
@@ -135,7 +138,8 @@ export function findBazelVersionTestCases(basePath: string) {
135138
// test case files by using "glob" and store them in our result map.
136139
if (!manifestPath) {
137140
const runfilesBaseDir = join(runfilesDir!, basePath);
138-
const inputFiles = globSync(`**/*${TEST_CASE_INPUT_SUFFIX}`, {cwd: runfilesBaseDir});
141+
const inputFiles = globSync(`**/!(${MISC_FOLDER_NAME})/*${TEST_CASE_INPUT_SUFFIX}`,
142+
{cwd: runfilesBaseDir});
139143

140144
inputFiles.forEach(inputFile => {
141145
// The target version of an input file will be determined from the first
@@ -158,9 +162,13 @@ export function findBazelVersionTestCases(basePath: string) {
158162
// In case the mapped runfile starts with the specified base path and ends with "_input.ts",
159163
// we store it in our result map because we assume that this is a test case.
160164
if (runfilePath.startsWith(basePath) && runfilePath.endsWith(TEST_CASE_INPUT_SUFFIX)) {
165+
const pathSegments = relative(basePath, runfilePath).split(sep);
166+
if (pathSegments.includes(MISC_FOLDER_NAME)) {
167+
return;
168+
}
161169
// The target version of an input file will be determined from the first
162170
// path segment. (e.g. "v6/my_rule_input.ts" will be for "v6")
163-
const targetVersion = relative(basePath, runfilePath).split(sep)[0];
171+
const targetVersion = pathSegments[0];
164172
testCasesMap.set(targetVersion, (testCasesMap.get(targetVersion) || []).concat(realPath));
165173
}
166174
});

src/cdk/schematics/update-tool/public-api.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,3 +11,5 @@ export * from './target-version';
1111
export * from './version-changes';
1212
export * from './migration-rule';
1313
export * from './component-resource-collector';
14+
export * from './utils/imports';
15+
export * from './utils/decorators';

src/cdk/schematics/update-tool/target-version.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,5 +21,5 @@ export enum TargetVersion {
2121
*/
2222
export function getAllVersionNames(): string[] {
2323
return Object.keys(TargetVersion)
24-
.filter(enumValue => typeof TargetVersion[enumValue] === 'number');
24+
.filter(enumValue => typeof TargetVersion[enumValue] === 'string');
2525
}

src/cdk/schematics/update-tool/utils/decorators.ts

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ export type CallExpressionDecorator = ts.Decorator&{
1717
export interface NgDecorator {
1818
name: string;
1919
node: CallExpressionDecorator;
20-
importNode: ts.ImportDeclaration;
2120
}
2221

2322
/**
@@ -27,23 +26,23 @@ export interface NgDecorator {
2726
export function getAngularDecorators(
2827
typeChecker: ts.TypeChecker, decorators: ReadonlyArray<ts.Decorator>): NgDecorator[] {
2928
return decorators.map(node => ({node, importData: getCallDecoratorImport(typeChecker, node)}))
30-
.filter(({importData}) => importData && importData.importModule.startsWith('@angular/'))
31-
.map(({node, importData}) => ({
32-
node: node as CallExpressionDecorator,
33-
name: importData!.name,
34-
importNode: importData!.node
35-
}));
29+
.filter(({importData}) => importData && importData.moduleName.startsWith('@angular/'))
30+
.map(
31+
({node, importData}) =>
32+
({node: node as CallExpressionDecorator, name: importData!.symbolName}));
3633
}
3734

3835
export function getCallDecoratorImport(
3936
typeChecker: ts.TypeChecker, decorator: ts.Decorator): Import|null {
40-
// Note that this does not cover the edge case where decorators are called from
41-
// a namespace import: e.g. "@core.Component()". This is not handled by Ngtsc either.
42-
if (!ts.isCallExpression(decorator.expression) ||
43-
!ts.isIdentifier(decorator.expression.expression)) {
37+
if (!ts.isCallExpression(decorator.expression)) {
4438
return null;
4539
}
46-
47-
const identifier = decorator.expression.expression;
48-
return getImportOfIdentifier(typeChecker, identifier);
40+
const valueExpr = decorator.expression.expression;
41+
let identifier: ts.Identifier|null = null;
42+
if (ts.isIdentifier(valueExpr)) {
43+
identifier = valueExpr;
44+
} else if (ts.isPropertyAccessExpression(valueExpr) && ts.isIdentifier(valueExpr.name)) {
45+
identifier = valueExpr.name;
46+
}
47+
return identifier ? getImportOfIdentifier(identifier, typeChecker) : null;
4948
}

src/cdk/schematics/update-tool/utils/imports.ts

Lines changed: 100 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -8,39 +8,117 @@
88

99
import * as ts from 'typescript';
1010

11-
export type Import = {
12-
name: string,
13-
importModule: string,
14-
node: ts.ImportDeclaration
15-
};
16-
17-
/** Gets import information about the specified identifier by using the type checker. */
18-
export function getImportOfIdentifier(typeChecker: ts.TypeChecker, node: ts.Identifier): Import|
11+
/** Interface describing a resolved import. */
12+
export interface Import {
13+
/** Name of the imported symbol. */
14+
symbolName: string;
15+
/** Module name from which the symbol has been imported. */
16+
moduleName: string;
17+
}
18+
19+
20+
/** Resolves the import of the specified identifier. */
21+
export function getImportOfIdentifier(node: ts.Identifier, typeChecker: ts.TypeChecker): Import|
1922
null {
20-
const symbol = typeChecker.getSymbolAtLocation(node);
23+
// Free standing identifiers which resolve to an import will be handled
24+
// as direct imports. e.g. "@Component()" where "Component" is an identifier
25+
// referring to an import specifier.
26+
const directImport = getSpecificImportOfIdentifier(node, typeChecker);
27+
if (directImport !== null) {
28+
return directImport;
29+
} else if (ts.isQualifiedName(node.parent) && node.parent.right === node) {
30+
// Determines the import of a qualified name. e.g. "let t: core.Component". In that
31+
// case, the import of the most left identifier will be determined ("core").
32+
const qualifierRoot = getQualifiedNameRoot(node.parent);
33+
if (qualifierRoot) {
34+
const moduleName = getImportOfNamespacedIdentifier(qualifierRoot, typeChecker);
35+
if (moduleName) {
36+
return {moduleName, symbolName: node.text};
37+
}
38+
}
39+
} else if (ts.isPropertyAccessExpression(node.parent) && node.parent.name === node) {
40+
// Determines the import of a property expression. e.g. "@core.Component". In that
41+
// case, the import of the most left identifier will be determined ("core").
42+
const rootIdentifier = getPropertyAccessRoot(node.parent);
43+
if (rootIdentifier) {
44+
const moduleName = getImportOfNamespacedIdentifier(rootIdentifier, typeChecker);
45+
if (moduleName) {
46+
return {moduleName, symbolName: node.text};
47+
}
48+
}
49+
}
50+
return null;
51+
}
2152

53+
/**
54+
* Resolves the import of the specified identifier. Expects the identifier to resolve
55+
* to a fine-grained import declaration with import specifiers.
56+
*/
57+
function getSpecificImportOfIdentifier(node: ts.Identifier, typeChecker: ts.TypeChecker): Import|
58+
null {
59+
const symbol = typeChecker.getSymbolAtLocation(node);
2260
if (!symbol || !symbol.declarations || !symbol.declarations.length) {
2361
return null;
2462
}
25-
26-
const decl = symbol.declarations[0];
27-
28-
if (!ts.isImportSpecifier(decl)) {
63+
const declaration = symbol.declarations[0];
64+
if (!ts.isImportSpecifier(declaration)) {
2965
return null;
3066
}
31-
32-
// Since "decl" is an import specifier, we can walk up three times to get a reference
67+
// Since the declaration is an import specifier, we can walk up three times to get a reference
3368
// to the import declaration node (NamedImports -> ImportClause -> ImportDeclaration).
34-
const importDecl = decl.parent.parent.parent;
35-
69+
const importDecl = declaration.parent.parent.parent;
3670
if (!ts.isStringLiteral(importDecl.moduleSpecifier)) {
3771
return null;
3872
}
39-
4073
return {
41-
// Handles aliased imports: e.g. "import {Component as myComp} from ...";
42-
name: decl.propertyName ? decl.propertyName.text : decl.name.text,
43-
importModule: importDecl.moduleSpecifier.text,
44-
node: importDecl
74+
moduleName: importDecl.moduleSpecifier.text,
75+
symbolName: declaration.propertyName ? declaration.propertyName.text : declaration.name.text
4576
};
4677
}
78+
79+
/**
80+
* Resolves the import of the specified identifier. Expects the identifier to
81+
* resolve to a namespaced import declaration. e.g. "import * as core from ...".
82+
*/
83+
function getImportOfNamespacedIdentifier(node: ts.Identifier, typeChecker: ts.TypeChecker): string|
84+
null {
85+
const symbol = typeChecker.getSymbolAtLocation(node);
86+
if (!symbol || !symbol.declarations || !symbol.declarations.length) {
87+
return null;
88+
}
89+
const declaration = symbol.declarations[0];
90+
if (!ts.isNamespaceImport(declaration)) {
91+
return null;
92+
}
93+
// Since the declaration is a namespace import, we can walk up three times to get a reference
94+
// to the import declaration node (NamespaceImport -> ImportClause -> ImportDeclaration).
95+
const importDecl = declaration.parent.parent;
96+
if (!ts.isStringLiteral(importDecl.moduleSpecifier)) {
97+
return null;
98+
}
99+
100+
return importDecl.moduleSpecifier.text;
101+
}
102+
103+
104+
/**
105+
* Gets the root identifier of a qualified type chain. For example: "core.GestureConfig"
106+
* will return the "core" identifier. Allowing us to find the import of "core".
107+
*/
108+
function getQualifiedNameRoot(name: ts.QualifiedName): ts.Identifier|null {
109+
while (ts.isQualifiedName(name.left)) {
110+
name = name.left;
111+
}
112+
return ts.isIdentifier(name.left) ? name.left : null;
113+
}
114+
115+
/**
116+
* Gets the root identifier of a property access chain. For example: "core.GestureConfig"
117+
* will return the "core" identifier. Allowing us to find the import of "core".
118+
*/
119+
function getPropertyAccessRoot(node: ts.PropertyAccessExpression): ts.Identifier|null {
120+
while (ts.isPropertyAccessExpression(node.expression)) {
121+
node = node.expression;
122+
}
123+
return ts.isIdentifier(node.expression) ? node.expression : null;
124+
}

src/material/schematics/ng-update/test-cases/v8/material-imports.spec.ts renamed to src/material/schematics/ng-update/test-cases/v8/misc/material-imports.spec.ts

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

44
describe('v8 material imports', () => {
55
it('should re-map top-level material imports to the proper entry points', async () => {

src/material/schematics/ng-update/test-cases/v9/hammer-migration-v9.spec.ts renamed to src/material/schematics/ng-update/test-cases/v9/misc/hammer-migration-v9.spec.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,11 @@ import {addPackageToPackageJson} from '@angular/cdk/schematics/ng-add/package-co
44
import {createTestCaseSetup} from '@angular/cdk/schematics/testing';
55
import {readFileSync} from 'fs';
66

7-
import {migrationCollection} from '../index.spec';
7+
import {migrationCollection} from '../../index.spec';
88

99
describe('v9 HammerJS removal', () => {
1010
const GESTURE_CONFIG_TEMPLATE_PATH =
11-
require.resolve('../../upgrade-rules/hammer-gestures-v9/gesture-config.template');
11+
require.resolve('../../../upgrade-rules/hammer-gestures-v9/gesture-config.template');
1212

1313
let runner: SchematicTestRunner;
1414
let tree: UnitTestTree;

src/material/schematics/ng-update/test-cases/v9/material-imports.spec.ts renamed to src/material/schematics/ng-update/test-cases/v9/misc/material-imports.spec.ts

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

44
describe('v9 material imports', () => {
55
it('should re-map top-level material imports to the proper entry points when top-level ' +

src/material/schematics/ng-update/upgrade-rules/hammer-gestures-v9/hammer-gestures-rule.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,9 @@ import {
1919
MigrationRule,
2020
PostMigrationAction,
2121
ResolvedResource,
22-
TargetVersion
22+
TargetVersion,
23+
Import,
24+
getImportOfIdentifier,
2325
} from '@angular/cdk/schematics';
2426
import {
2527
addSymbolToNgModuleMetadata,
@@ -38,7 +40,6 @@ import {getProjectFromProgram} from './cli-workspace';
3840
import {findHammerScriptImportElements} from './find-hammer-script-tags';
3941
import {findMainModuleExpression} from './find-main-module';
4042
import {isHammerJsUsedInTemplate} from './hammer-template-check';
41-
import {getImportOfIdentifier, Import} from './identifier-imports';
4243
import {ImportManager} from './import-manager';
4344
import {removeElementFromArrayExpression} from './remove-array-element';
4445
import {removeElementFromHtml} from './remove-element-from-html';

0 commit comments

Comments
 (0)