Skip to content

Commit 060123f

Browse files
devversionmmalerba
authored andcommitted
refactor(schematics): properly handle inherited constructors for signature checks (#13180)
* Currently if someone extends an Angular Material class of which the signature has changed and the constructor is inherited, the constructor signature checks rule won't report the breaking change because the rule does not properly handle the inherited constructor.
1 parent 19ce1a1 commit 060123f

File tree

3 files changed

+46
-4
lines changed

3 files changed

+46
-4
lines changed

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

Lines changed: 33 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ const signatureErrorDiagnostics = [
2626
];
2727

2828
/** List of classes of which the constructor signature has changed. */
29-
const signatureChangedClasses = getAllChanges(constructorChecks);
29+
const signatureChangeData = getAllChanges(constructorChecks);
3030

3131
/**
3232
* Rule that visits every TypeScript new expression or super call and checks if the parameter
@@ -64,9 +64,19 @@ function visitSourceFile(context: WalkContext<null>, program: ts.Program) {
6464
const className = classType.symbol && classType.symbol.name;
6565
const isNewExpression = ts.isNewExpression(node);
6666

67-
// TODO(devversion): Consider handling pass-through classes better.
68-
// TODO(devversion): e.g. `export class CustomCalendar extends MatCalendar {}`
69-
if (!signatureChangedClasses.includes(className)) {
67+
// Determine the class names of the actual construct signatures because we cannot assume
68+
// that the diagnostic refers to a constructor of the actual expression. In case the constructor
69+
// is inherited, we need to detect that the owner-class of the constructor is added to the
70+
// constructor checks upgrade data. e.g. `class CustomCalendar extends MatCalendar {}`.
71+
const signatureClassNames = classType.getConstructSignatures()
72+
.map(signature => getClassDeclarationOfSignature(signature))
73+
.map(declaration => declaration && declaration.name ? declaration.name.text : null)
74+
.filter(Boolean);
75+
76+
// Besides checking the signature class names, we need to check the actual class name because
77+
// there can be classes without an explicit constructor.
78+
if (!signatureChangeData.includes(className) &&
79+
!signatureClassNames.some(name => signatureChangeData.includes(name!))) {
7080
continue;
7181
}
7282

@@ -120,3 +130,22 @@ function findConstructorNode(diagnostic: ts.Diagnostic, sourceFile: ts.SourceFil
120130

121131
return resolvedNode;
122132
}
133+
134+
/** Determines the class declaration of the specified construct signature. */
135+
function getClassDeclarationOfSignature(signature: ts.Signature): ts.ClassDeclaration | null {
136+
let node: ts.Node = signature.getDeclaration();
137+
138+
// Handle signatures which don't have an actual declaration. This happens if a class
139+
// does not have an explicitly written constructor.
140+
if (!node) {
141+
return null;
142+
}
143+
144+
while (!ts.isSourceFile(node = node.parent)) {
145+
if (ts.isClassDeclaration(node)) {
146+
return node;
147+
}
148+
}
149+
150+
return null;
151+
}

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,9 @@ describe('constructor checks', () => {
4343

4444
expect(output).toMatch(/Found "MatSidenavContent".*super.*: super\((any, ){4}any\)/);
4545
expect(output).toMatch(/Found "MatSidenavContent".*: new \w+\((any, ){4}any\)/);
46+
47+
expect(output).toMatch(/Found "ExtendedDateAdapter".*super.*: super\(string, Platform\)/);
48+
expect(output).toMatch(/Found "ExtendedDateAdapter".*: new \w+\(string, Platform\)/);
4649
});
4750
});
4851

src/lib/schematics/update/test-cases/misc/constructor-checks_input.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,8 @@ class MatSidenavContent {
6464
_ngZone: any) {}
6565
}
6666

67+
class ExtendedDateAdapter extends NativeDateAdapter {}
68+
6769
/* Actual test case using the previously defined definitions. */
6870

6971
class A extends NativeDateAdapter {
@@ -127,3 +129,11 @@ class G extends MatSidenavContent {
127129
}
128130

129131
const _G = new MatSidenavContent({}, 'container');
132+
133+
class H extends ExtendedDateAdapter {
134+
constructor() {
135+
super('myLocale');
136+
}
137+
}
138+
139+
const _H = new ExtendedDateAdapter('myLocale');

0 commit comments

Comments
 (0)