Skip to content

Commit 487dca1

Browse files
devversiondevversion
committed
refactor(ng-update): better check for updated constructor signatures
Instead of just checking the length of the constructor arguments, we now check the types of the constructor or super call. This means that we can *way* better report invalid signatures for constructor changes like for the `MatCalendar` (#9775). Just relying on the length of arguments means that *order* is being ignored. This also makes maintaining the constructor signature changes easier (there are a lot of instances for V7).
1 parent 193c2d0 commit 487dca1

File tree

9 files changed

+337
-200
lines changed

9 files changed

+337
-200
lines changed

‎src/lib/schematics/update/material/data/constructor-checks.ts‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import {transformChanges} from '../transform-change-data';
10+
11+
export const constructorChecks = transformChanges<string>([
12+
{
13+
pr: 'https://github.com/angular/material2/pull/9190',
14+
changes: ['NativeDateAdapter']
15+
},
16+
17+
{
18+
pr: 'https://github.com/angular/material2/pull/10319',
19+
changes: ['MatAutocomplete']
20+
},
21+
22+
{
23+
pr: 'https://github.com/angular/material2/pull/10344',
24+
changes: ['MatTooltip']
25+
},
26+
27+
{
28+
pr: 'https://github.com/angular/material2/pull/10389',
29+
changes: ['MatIconRegistry']
30+
},
31+
32+
{
33+
pr: 'https://github.com/angular/material2/pull/9775',
34+
changes: ['MatCalendar']
35+
}
36+
]);

‎src/lib/schematics/update/material/data/method-call-checks.ts‎

Lines changed: 0 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -18,38 +18,6 @@ export interface MaterialMethodCallData {
1818
}
1919

2020
export const methodCallChecks = transformChanges<MaterialMethodCallData>([
21-
{
22-
pr: 'https://github.com/angular/material2/pull/9190',
23-
changes: [
24-
{
25-
className: 'NativeDateAdapter',
26-
method: 'constructor',
27-
invalidArgCounts: [
28-
{
29-
count: 1,
30-
message: '"g{{platform}}" is now required as a second argument'
31-
}
32-
]
33-
}
34-
]
35-
},
36-
37-
{
38-
pr: 'https://github.com/angular/material2/pull/10319',
39-
changes: [
40-
{
41-
className: 'MatAutocomplete',
42-
method: 'constructor',
43-
invalidArgCounts: [
44-
{
45-
count: 2,
46-
message: '"g{{defaults}}" is now required as a third argument'
47-
}
48-
]
49-
}
50-
]
51-
},
52-
5321
{
5422
pr: 'https://github.com/angular/material2/pull/10325',
5523
changes: [
@@ -65,57 +33,4 @@ export const methodCallChecks = transformChanges<MaterialMethodCallData>([
6533
}
6634
]
6735
},
68-
69-
{
70-
pr: 'https://github.com/angular/material2/pull/10344',
71-
changes: [
72-
{
73-
className: 'MatTooltip',
74-
method: 'constructor',
75-
invalidArgCounts: [
76-
{
77-
count: 11,
78-
message: '"g{{_defaultOptions}}" is now required as a twelfth argument'
79-
}
80-
]
81-
}
82-
]
83-
},
84-
85-
{
86-
pr: 'https://github.com/angular/material2/pull/10389',
87-
changes: [
88-
{
89-
className: 'MatIconRegistry',
90-
method: 'constructor',
91-
invalidArgCounts: [
92-
{
93-
count: 2,
94-
message: '"g{{document}}" is now required as a third argument'
95-
}
96-
]
97-
}
98-
]
99-
},
100-
101-
{
102-
pr: 'https://github.com/angular/material2/pull/9775',
103-
changes: [
104-
{
105-
className: 'MatCalendar',
106-
method: 'constructor',
107-
invalidArgCounts: [
108-
{
109-
count: 6,
110-
message: '"r{{_elementRef}}" and "r{{_ngZone}}" arguments have been removed'
111-
},
112-
{
113-
count: 7,
114-
message: '"r{{_elementRef}}", "r{{_ngZone}}", and "r{{_dir}}" arguments have been ' +
115-
'removed'
116-
}
117-
]
118-
}
119-
]
120-
}
12136
]);

‎src/lib/schematics/update/rules/signature-check/constructorSignatureCheckRule.ts‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import {bold, green} from 'chalk';
10+
import {ProgramAwareRuleWalker, RuleFailure, Rules} from 'tslint';
11+
import * as ts from 'typescript';
12+
import {constructorChecks} from '../../material/data/constructor-checks';
13+
14+
/**
15+
* Rule that visits every TypeScript new expression or super call and checks if the parameter
16+
* type signature is invalid and needs to be updated manually.
17+
*/
18+
export class Rule extends Rules.TypedRule {
19+
applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): RuleFailure[] {
20+
return this.applyWithWalker(new Walker(sourceFile, this.getOptions(), program));
21+
}
22+
}
23+
24+
export class Walker extends ProgramAwareRuleWalker {
25+
26+
visitNewExpression(node: ts.NewExpression) {
27+
this.checkExpressionSignature(node);
28+
super.visitNewExpression(node);
29+
}
30+
31+
visitCallExpression(node: ts.CallExpression) {
32+
if (node.expression.kind === ts.SyntaxKind.SuperKeyword) {
33+
this.checkExpressionSignature(node);
34+
}
35+
36+
return super.visitCallExpression(node);
37+
}
38+
39+
private getParameterTypesFromSignature(signature: ts.Signature): ts.Type[] {
40+
return signature.getParameters()
41+
.map(param => param.declarations[0] as ts.ParameterDeclaration)
42+
.map(node => node.type)
43+
.map(node => this.getTypeChecker().getTypeFromTypeNode(node));
44+
}
45+
46+
private checkExpressionSignature(node: ts.CallExpression | ts.NewExpression) {
47+
const classType = this.getTypeChecker().getTypeAtLocation(node.expression);
48+
const className = classType.symbol && classType.symbol.name;
49+
const isNewExpression = ts.isNewExpression(node);
50+
51+
// TODO(devversion): Consider handling pass-through classes better.
52+
// TODO(devversion): e.g. `export class CustomCalendar extends MatCalendar {}`
53+
if (!classType || !constructorChecks.includes(className)) {
54+
return;
55+
}
56+
57+
const callExpressionSignature = node.arguments
58+
.map(argument => this.getTypeChecker().getTypeAtLocation(argument));
59+
const classSignatures = classType.getConstructSignatures()
60+
.map(signature => this.getParameterTypesFromSignature(signature));
61+
62+
// TODO(devversion): we should check if the type is assignable to the signature
63+
// TODO(devversion): blocked on https://github.com/Microsoft/TypeScript/issues/9879
64+
const doesMatchSignature = classSignatures.some(signature => {
65+
return signature.every((type, index) => callExpressionSignature[index] === type) &&
66+
signature.length === callExpressionSignature.length;
67+
});
68+
69+
if (!doesMatchSignature) {
70+
const expressionName = isNewExpression ? `new ${className}` : 'super';
71+
const signatures = classSignatures
72+
.map(signature => signature.map(t => this.getTypeChecker().typeToString(t)))
73+
.map(signature => `${expressionName}(${signature.join(', ')})`)
74+
.join(' or ');
75+
76+
this.addFailureAtNode(node, `Found "${bold(className)}" constructed with ` +
77+
`an invalid signature. Please manually update the ${bold(expressionName)} expression to ` +
78+
`match the new signature${classSignatures.length > 1 ? 's' : ''}: ${green(signatures)}`);
79+
}
80+
}
81+
}

src/lib/schematics/update/rules/method-calls/methodCallsCheckRule.ts renamed to src/lib/schematics/update/rules/signature-check/methodCallsCheckRule.ts

Lines changed: 2 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@ import {color} from '../../material/color';
1313
import {methodCallChecks} from '../../material/data/method-call-checks';
1414

1515
/**
16-
* Rule that visits every TypeScript call expression or TypeScript new expression and checks
17-
* if the argument count is invalid and needs to be *manually* updated.
16+
* Rule that visits every TypeScript method call expression and checks if the argument count
17+
* is invalid and needs to be *manually* updated.
1818
*/
1919
export class Rule extends Rules.TypedRule {
2020
applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): RuleFailure[] {
@@ -24,24 +24,7 @@ export class Rule extends Rules.TypedRule {
2424

2525
export class Walker extends ProgramAwareRuleWalker {
2626

27-
visitNewExpression(expression: ts.NewExpression) {
28-
const classType = this.getTypeChecker().getTypeAtLocation(expression);
29-
30-
if (classType && classType.symbol) {
31-
this.checkConstructor(expression, classType.symbol.name);
32-
}
33-
}
34-
3527
visitCallExpression(node: ts.CallExpression) {
36-
if (node.expression.kind === ts.SyntaxKind.SuperKeyword) {
37-
const superClassType = this.getTypeChecker().getTypeAtLocation(node.expression);
38-
const superClassName = superClassType.symbol && superClassType.symbol.name;
39-
40-
if (superClassName) {
41-
this.checkConstructor(node, superClassName);
42-
}
43-
}
44-
4528
if (ts.isPropertyAccessExpression(node.expression)) {
4629
this._checkPropertyAccessMethodCall(node);
4730
}
@@ -75,18 +58,4 @@ export class Walker extends ProgramAwareRuleWalker {
7558
this.addFailureAtNode(node, `Found call to "${bold(hostTypeName + '.' + methodName)}" ` +
7659
`with ${bold(`${failure.count}`)} arguments. Message: ${color(failure.message)}`);
7760
}
78-
79-
private checkConstructor(node: ts.NewExpression | ts.CallExpression, className: string) {
80-
const argumentsLength = node.arguments ? node.arguments.length : 0;
81-
const failure = methodCallChecks
82-
.filter(data => data.method === 'constructor' && data.className === className)
83-
.map(data => data.invalidArgCounts.find(f => f.count === argumentsLength))[0];
84-
85-
if (!failure) {
86-
return;
87-
}
88-
89-
this.addFailureAtNode(node, `Found "${bold(className)}" constructed with ` +
90-
`${bold(`${failure.count}`)} arguments. Message: ${color(failure.message)}`);
91-
}
9261
}

‎src/lib/schematics/update/test-cases/index.spec.ts‎

Lines changed: 42 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -1,83 +1,48 @@
1-
import {getSystemPath, normalize, virtualFs} from '@angular-devkit/core';
1+
import {getSystemPath, normalize} from '@angular-devkit/core';
22
import {TempScopedNodeJsSyncHost} from '@angular-devkit/core/node/testing';
3-
import {SchematicTestRunner} from '@angular-devkit/schematics/testing';
3+
import * as virtualFs from '@angular-devkit/core/src/virtual-fs/host';
4+
import {createTestApp} from '../../test-setup/test-app';
45
import {readFileSync} from 'fs';
5-
import {runPostScheduledTasks} from '../../test-setup/post-scheduled-tasks';
6-
import {createTestApp, migrationCollection} from '../../test-setup/test-app';
76

8-
describe('test cases', () => {
9-
10-
// Module name suffix for data files of the `jasmine_node_test` Bazel rule.
11-
const bazelModuleSuffix = 'angular_material/src/lib/schematics/update/test-cases';
12-
13-
/**
14-
* Name of test cases that will be used to verify that update schematics properly update
15-
* a developers application.
16-
*/
17-
const testCases = [
18-
'v5/attribute-selectors',
19-
'v5/class-names',
20-
'v5/css-names',
21-
'v5/element-selectors',
22-
'v5/input-names',
23-
'v5/output-names',
24-
'v5/property-names',
25-
];
26-
27-
// Iterates through every test case directory and generates a jasmine test block that will
28-
// verify that the update schematics properly update the test input to the expected output.
29-
testCases.forEach(testCaseName => {
30-
31-
// Adding the test case files to the data of the `jasmine_node_test` Bazel rule does not mean
32-
// that the files are being copied over to the Bazel bin output. Bazel just patches the NodeJS
33-
// resolve function and maps the module paths to the original file location. Since we
34-
// need to load the content of those test cases, we need to resolve the original file path.
35-
const inputPath = require.resolve(`${bazelModuleSuffix}/${testCaseName}_input.ts`);
36-
const expectedOutputPath = require
37-
.resolve(`${bazelModuleSuffix}/${testCaseName}_expected_output.ts`);
38-
39-
it(`should apply update schematics to test case: ${testCaseName}`, () => {
40-
const runner = new SchematicTestRunner('schematics', migrationCollection);
41-
42-
runner.runSchematic('migration-01', {}, createTestAppWithTestCase(inputPath));
43-
44-
// Run the scheduled TSLint fix task from the update schematic. This task is responsible for
45-
// identifying outdated code parts and performs the fixes. Since tasks won't run automatically
46-
// within a `SchematicTestRunner`, we manually need to run the scheduled task.
47-
return runPostScheduledTasks(runner, 'tslint-fix').toPromise().then(() => {
48-
expect(readFileContent('projects/material/src/main.ts'))
49-
.toBe(readFileContent(expectedOutputPath));
50-
});
51-
});
7+
/** Module name suffix for data files of the `jasmine_node_test` Bazel rule. */
8+
const bazelModuleSuffix = 'angular_material/src/lib/schematics/update/test-cases';
9+
10+
/** Reads the UTF8 content of the specified file. Normalizes the path and ensures that */
11+
export function readFileContent(filePath: string): string {
12+
return readFileSync(filePath, 'utf8');
13+
}
14+
15+
/**
16+
* Resolves the original file path of the specified file that has been to the `data` of the
17+
* jasmine_node_test Bazel rule.
18+
*
19+
* Adding the test case files to the data of the `jasmine_node_test` Bazel rule does not mean
20+
* that the files are being copied over to the Bazel bin output. Bazel just patches the NodeJS
21+
* resolve function and maps the module paths to the original file location.
22+
*/
23+
export function resolveBazelDataFile(filePath: string) {
24+
return require.resolve(`${bazelModuleSuffix}/${filePath}`);
25+
}
26+
27+
/**
28+
* Creates a test app schematic tree that includes the specified test case as TypeScript
29+
* entry point file. Also writes the app tree to a real file system location in order to be
30+
* able to test the tslint fix rules.
31+
*/
32+
export function createTestAppWithTestCase(testCaseInputPath: string) {
33+
const tempFileSystemHost = new TempScopedNodeJsSyncHost();
34+
const appTree = createTestApp();
35+
36+
appTree.overwrite('/projects/material/src/main.ts', readFileContent(testCaseInputPath));
37+
38+
// Since the TSLint fix task expects all files to be present on the real file system, we
39+
// map every file in the app tree to a temporary location on the file system.
40+
appTree.files.map(f => normalize(f)).forEach(f => {
41+
tempFileSystemHost.sync.write(f, virtualFs.stringToFileBuffer(appTree.readContent(f)));
5242
});
5343

54-
/** Reads the UTF8 content of the specified file. Normalizes the path and ensures that */
55-
function readFileContent(filePath: string): string {
56-
return readFileSync(filePath, 'utf8');
57-
}
58-
59-
/**
60-
* Creates a test app schematic tree that includes the specified test case as TypeScript
61-
* entry point file. Also writes the app tree to a real file system location in order to be
62-
* able to test the tslint fix rules.
63-
*/
64-
function createTestAppWithTestCase(testCaseInputPath: string) {
65-
const tempFileSystemHost = new TempScopedNodeJsSyncHost();
66-
const appTree = createTestApp();
67-
68-
appTree.overwrite('/projects/material/src/main.ts', readFileContent(testCaseInputPath));
69-
70-
// Since the TSLint fix task expects all files to be present on the real file system, we
71-
// map every file in the app tree to a temporary location on the file system.
72-
appTree.files.map(f => normalize(f)).forEach(f => {
73-
tempFileSystemHost.sync.write(f, virtualFs.stringToFileBuffer(appTree.readContent(f)));
74-
});
75-
76-
// Switch to the new temporary directory because otherwise TSLint cannot read the files.
77-
process.chdir(getSystemPath(tempFileSystemHost.root));
78-
79-
return appTree;
80-
}
81-
});
82-
44+
// Switch to the new temporary directory because otherwise TSLint cannot read the files.
45+
process.chdir(getSystemPath(tempFileSystemHost.root));
8346

47+
return appTree;
48+
}

0 commit comments

Comments
 (0)