fix(core): undecorated-classes-with-decorated-fields migration does not decorate derived classes (#35339)

The `undecorated-classes-with-decorated-fields` migration has been
introduced with 904a2018e0, but misses
logic for decorating derived classes of undecorated classes which use
Angular features. Example scenario:

```ts
export abstract class MyBaseClass {
  @Input() someInput = true;
}

export abstract class BaseClassTwo extends MyBaseClass {}

@Component(...)
export class MyButton extends BaseClassTwo {}
```

Both abstract classes would need to be migrated. Previously, the migration
only added `@Directive()` to `MyBaseClass`, but with this change, it
also decorates `BaseClassTwo`.

This is necessary because the Angular Compiler requires `BaseClassTwo` to
have a directive definition when it flattens the directive metadata for
`MyButton` in order to perform type checking. Technically, not decorating
`BaseClassTwo` does not break at runtime.

We basically want to enforce consistent use of `@Directive` to simplify the
mental model. [See the migration guide](https://angular.io/guide/migration-undecorated-classes#migrating-classes-that-use-field-decorators).

Fixes #34376.

PR Close #35339
This commit is contained in:
Paul Gschwendtner 2020-02-11 16:33:20 +01:00 committed by Kara Erickson
parent 2366480250
commit 32eafef6a7
11 changed files with 358 additions and 138 deletions

View File

@ -13,6 +13,7 @@ ts_library(
"//packages/core/schematics/migrations/static-queries",
"//packages/core/schematics/migrations/template-var-assignment",
"//packages/core/schematics/migrations/undecorated-classes-with-decorated-fields",
"//packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/google3",
"//packages/core/schematics/utils",
"//packages/core/schematics/utils/tslint",
"@npm//tslint",

View File

@ -6,12 +6,10 @@
* found in the LICENSE file at https://angular.io/license
*/
import {Replacement, RuleFailure, Rules} from 'tslint';
import {RuleFailure, Rules} from 'tslint';
import * as ts from 'typescript';
import {FALLBACK_DECORATOR, addImport, getNamedImports, getUndecoratedClassesWithDecoratedFields, hasNamedImport} from '../undecorated-classes-with-decorated-fields/utils';
import {TslintUpdateRecorder} from '../undecorated-classes-with-decorated-fields/google3/tslint_update_recorder';
import {UndecoratedClassesWithDecoratedFieldsTransform} from '../undecorated-classes-with-decorated-fields/transform';
/**
* TSLint rule that adds an Angular decorator to classes that have Angular field decorators.
@ -20,37 +18,32 @@ import {FALLBACK_DECORATOR, addImport, getNamedImports, getUndecoratedClassesWit
export class Rule extends Rules.TypedRule {
applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): RuleFailure[] {
const typeChecker = program.getTypeChecker();
const printer = ts.createPrinter();
const classes = getUndecoratedClassesWithDecoratedFields(sourceFile, typeChecker);
const ruleName = this.ruleName;
const sourceFiles = program.getSourceFiles().filter(
s => !s.isDeclarationFile && !program.isSourceFileFromExternalLibrary(s));
const updateRecorders = new Map<ts.SourceFile, TslintUpdateRecorder>();
const transform =
new UndecoratedClassesWithDecoratedFieldsTransform(typeChecker, getUpdateRecorder);
return classes.map((current, index) => {
const {classDeclaration: declaration, importDeclaration} = current;
const name = declaration.name;
// Migrate all source files in the project.
transform.migrate(sourceFiles);
// Set the class identifier node (if available) as the failing node so IDEs don't highlight
// the entire class with red. This is similar to how errors are shown for classes in other
// cases like an interface not being implemented correctly.
const start = (name || declaration).getStart();
const end = (name || declaration).getEnd();
const fixes = [Replacement.appendText(declaration.getStart(), `@${FALLBACK_DECORATOR}()\n`)];
// Record the changes collected in the import manager.
transform.recordChanges();
// If it's the first class that we're processing in this file, add `Directive` to the imports.
if (index === 0 && !hasNamedImport(importDeclaration, FALLBACK_DECORATOR)) {
const namedImports = getNamedImports(importDeclaration);
if (updateRecorders.has(sourceFile)) {
return updateRecorders.get(sourceFile) !.failures;
}
return [];
if (namedImports) {
fixes.push(new Replacement(
namedImports.getStart(), namedImports.getWidth(),
printer.printNode(
ts.EmitHint.Unspecified, addImport(namedImports, FALLBACK_DECORATOR),
sourceFile)));
}
/** Gets the update recorder for the specified source file. */
function getUpdateRecorder(sourceFile: ts.SourceFile): TslintUpdateRecorder {
if (updateRecorders.has(sourceFile)) {
return updateRecorders.get(sourceFile) !;
}
return new RuleFailure(
sourceFile, start, end,
'Classes with decorated fields must have an Angular decorator as well.',
'undecorated-classes-with-decorated-fields', fixes);
});
const recorder = new TslintUpdateRecorder(ruleName, sourceFile);
updateRecorders.set(sourceFile, recorder);
return recorder;
}
}
}

View File

@ -7,6 +7,7 @@ ts_library(
visibility = [
"//packages/core/schematics:__pkg__",
"//packages/core/schematics/migrations/google3:__pkg__",
"//packages/core/schematics/migrations/undecorated-classes-with-decorated-fields/google3:__pkg__",
"//packages/core/schematics/test:__pkg__",
],
deps = [

View File

@ -0,0 +1,13 @@
load("//tools:defaults.bzl", "ts_library")
ts_library(
name = "google3",
srcs = glob(["**/*.ts"]),
tsconfig = "//packages/core/schematics:tsconfig.json",
visibility = ["//packages/core/schematics/migrations/google3:__pkg__"],
deps = [
"//packages/core/schematics/migrations/undecorated-classes-with-decorated-fields",
"@npm//tslint",
"@npm//typescript",
],
)

View File

@ -0,0 +1,49 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {Replacement, RuleFailure} from 'tslint';
import * as ts from 'typescript';
import {UpdateRecorder} from '../update_recorder';
export class TslintUpdateRecorder implements UpdateRecorder {
failures: RuleFailure[] = [];
constructor(private ruleName: string, private sourceFile: ts.SourceFile) {}
/** Adds the specified decorator to the given class declaration. */
addClassDecorator(node: ts.ClassDeclaration, decoratorText: string) {
// Adding a decorator should be the last replacement. Replacements/rule failures
// are handled in reverse and in case a decorator and import are inserted at
// the start of the file, the class decorator should come after the import.
this.failures.unshift(new RuleFailure(
this.sourceFile, node.getStart(), 0, `Class needs to be decorated with ` +
`"${decoratorText}" because it uses Angular features.`,
this.ruleName, Replacement.appendText(node.getStart(), `${decoratorText}\n`)));
}
/** Adds the specified import to the source file at the given position */
addNewImport(start: number, importText: string) {
this.failures.push(new RuleFailure(
this.sourceFile, start, 0, `Source file needs to have import: "${importText}"`,
this.ruleName, Replacement.appendText(start, importText)));
}
/** Updates existing named imports to the given new named imports. */
updateExistingImport(namedBindings: ts.NamedImports, newNamedBindings: string): void {
const fix = [
Replacement.deleteText(namedBindings.getStart(), namedBindings.getWidth()),
Replacement.appendText(namedBindings.getStart(), newNamedBindings),
];
this.failures.push(new RuleFailure(
this.sourceFile, namedBindings.getStart(), namedBindings.getEnd(),
`Import needs to be updated to import symbols: "${newNamedBindings}"`, this.ruleName, fix));
}
commitUpdate() {}
}

View File

@ -6,21 +6,21 @@
* found in the LICENSE file at https://angular.io/license
*/
import {Rule, SchematicContext, SchematicsException, Tree} from '@angular-devkit/schematics';
import {Rule, SchematicsException, Tree,} from '@angular-devkit/schematics';
import {dirname, relative} from 'path';
import * as ts from 'typescript';
import {getProjectTsConfigPaths} from '../../utils/project_tsconfig_paths';
import {createMigrationCompilerHost} from '../../utils/typescript/compiler_host';
import {parseTsconfigFile} from '../../utils/typescript/parse_tsconfig';
import {FALLBACK_DECORATOR, addImport, getNamedImports, getUndecoratedClassesWithDecoratedFields, hasNamedImport} from './utils';
import {UpdateRecorder} from './update_recorder';
import {UndecoratedClassesWithDecoratedFieldsTransform} from './transform';
/**
* Migration that adds an Angular decorator to classes that have Angular field decorators.
* https://hackmd.io/vuQfavzfRG6KUCtU7oK_EA
*/
export default function(): Rule {
return (tree: Tree, context: SchematicContext) => {
return (tree: Tree) => {
const {buildPaths, testPaths} = getProjectTsConfigPaths(tree);
const basePath = process.cwd();
const allPaths = [...buildPaths, ...testPaths];
@ -41,39 +41,49 @@ function runUndecoratedClassesMigration(tree: Tree, tsconfigPath: string, basePa
const host = createMigrationCompilerHost(tree, parsed.options, basePath);
const program = ts.createProgram(parsed.fileNames, parsed.options, host);
const typeChecker = program.getTypeChecker();
const printer = ts.createPrinter();
const sourceFiles = program.getSourceFiles().filter(
file => !file.isDeclarationFile && !program.isSourceFileFromExternalLibrary(file));
const updateRecorders = new Map<ts.SourceFile, UpdateRecorder>();
const transform =
new UndecoratedClassesWithDecoratedFieldsTransform(typeChecker, getUpdateRecorder);
sourceFiles.forEach(sourceFile => {
const classes = getUndecoratedClassesWithDecoratedFields(sourceFile, typeChecker);
// Migrate all source files in the project.
transform.migrate(sourceFiles);
if (classes.length === 0) {
return;
// Record the changes collected in the import manager.
transform.recordChanges();
// Walk through each update recorder and commit the update. We need to commit the
// updates in batches per source file as there can be only one recorder per source
// file in order to avoid shifted character offsets.
updateRecorders.forEach(recorder => recorder.commitUpdate());
/** Gets the update recorder for the specified source file. */
function getUpdateRecorder(sourceFile: ts.SourceFile): UpdateRecorder {
if (updateRecorders.has(sourceFile)) {
return updateRecorders.get(sourceFile) !;
}
const update = tree.beginUpdate(relative(basePath, sourceFile.fileName));
classes.forEach((current, index) => {
// If it's the first class that we're processing in this file, add `Directive` to the imports.
if (index === 0 && !hasNamedImport(current.importDeclaration, FALLBACK_DECORATOR)) {
const namedImports = getNamedImports(current.importDeclaration);
if (namedImports) {
update.remove(namedImports.getStart(), namedImports.getWidth());
update.insertRight(
namedImports.getStart(),
printer.printNode(
ts.EmitHint.Unspecified, addImport(namedImports, FALLBACK_DECORATOR),
sourceFile));
}
}
// We don't need to go through the AST to insert the decorator, because the change
// is pretty basic. Also this has a better chance of preserving the user's formatting.
update.insertLeft(current.classDeclaration.getStart(), `@${FALLBACK_DECORATOR}()\n`);
});
tree.commitUpdate(update);
});
const treeRecorder = tree.beginUpdate(relative(basePath, sourceFile.fileName));
const recorder: UpdateRecorder = {
addClassDecorator(node: ts.ClassDeclaration, text: string) {
// New imports should be inserted at the left while decorators should be inserted
// at the right in order to ensure that imports are inserted before the decorator
// if the start position of import and decorator is the source file start.
treeRecorder.insertRight(node.getStart(), `${text}\n`);
},
addNewImport(start: number, importText: string) {
// New imports should be inserted at the left while decorators should be inserted
// at the right in order to ensure that imports are inserted before the decorator
// if the start position of import and decorator is the source file start.
treeRecorder.insertLeft(start, importText);
},
updateExistingImport(namedBindings: ts.NamedImports, newNamedBindings: string) {
treeRecorder.remove(namedBindings.getStart(), namedBindings.getWidth());
treeRecorder.insertRight(namedBindings.getStart(), newNamedBindings);
},
commitUpdate() { tree.commitUpdate(treeRecorder); }
};
updateRecorders.set(sourceFile, recorder);
return recorder;
}
}

View File

@ -0,0 +1,100 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import * as ts from 'typescript';
import {ImportManager} from '../../utils/import_manager';
import {getAngularDecorators} from '../../utils/ng_decorators';
import {findBaseClassDeclarations} from '../../utils/typescript/find_base_classes';
import {UpdateRecorder} from './update_recorder';
export class UndecoratedClassesWithDecoratedFieldsTransform {
private printer = ts.createPrinter();
private importManager = new ImportManager(this.getUpdateRecorder, this.printer);
constructor(
private typeChecker: ts.TypeChecker,
private getUpdateRecorder: (sf: ts.SourceFile) => UpdateRecorder) {}
/**
* Migrates the specified source files. The transform adds the abstract `@Directive`
* decorator to classes that have Angular field decorators but are not decorated.
* https://hackmd.io/vuQfavzfRG6KUCtU7oK_EA
*/
migrate(sourceFiles: ts.SourceFile[]) {
this._findUndecoratedDirectives(sourceFiles).forEach(node => {
const sourceFile = node.getSourceFile();
const recorder = this.getUpdateRecorder(sourceFile);
const directiveExpr =
this.importManager.addImportToSourceFile(sourceFile, 'Directive', '@angular/core');
const decoratorExpr = ts.createDecorator(ts.createCall(directiveExpr, undefined, undefined));
recorder.addClassDecorator(
node, this.printer.printNode(ts.EmitHint.Unspecified, decoratorExpr, sourceFile));
});
}
/** Records all changes that were made in the import manager. */
recordChanges() { this.importManager.recordChanges(); }
/** Finds undecorated directives in the specified source files. */
private _findUndecoratedDirectives(sourceFiles: ts.SourceFile[]) {
const typeChecker = this.typeChecker;
const undecoratedDirectives = new Set<ts.ClassDeclaration>();
const undecoratedClasses = new Set<ts.ClassDeclaration>();
const decoratedDirectives = new WeakSet<ts.ClassDeclaration>();
const visitNode = (node: ts.Node) => {
node.forEachChild(visitNode);
if (!ts.isClassDeclaration(node)) {
return;
}
const ngDecorators = node.decorators && getAngularDecorators(typeChecker, node.decorators);
const isDirectiveOrComponent = ngDecorators !== undefined &&
ngDecorators.some(({name}) => name === 'Directive' || name === 'Component');
if (isDirectiveOrComponent) {
decoratedDirectives.add(node);
} else {
if (this._hasAngularDecoratedClassMember(node)) {
undecoratedDirectives.add(node);
} else {
undecoratedClasses.add(node);
}
}
};
sourceFiles.forEach(sourceFile => sourceFile.forEachChild(visitNode));
// We collected all class declarations that use Angular features but are not decorated. For
// such undecorated directives, the derived classes also need to be migrated. To achieve this,
// we walk through all undecorated classes and mark those which extend from an undecorated
// directive as undecorated directive too.
undecoratedClasses.forEach(node => {
for (const {node: baseClass} of findBaseClassDeclarations(node, this.typeChecker)) {
// If the undecorated class inherits from a decorated directive, skip the current class.
// We do this because undecorated classes which inherit from directives/components are
// handled as part of the the `undecorated-classes-with-di` migration which copies
// inherited metadata.
if (decoratedDirectives.has(baseClass)) {
break;
} else if (undecoratedDirectives.has(baseClass)) {
undecoratedDirectives.add(node);
undecoratedClasses.delete(node);
break;
}
}
});
return undecoratedDirectives;
}
private _hasAngularDecoratedClassMember(node: ts.ClassDeclaration): boolean {
return node.members.some(
m => m.decorators && getAngularDecorators(this.typeChecker, m.decorators).length !== 0);
}
}

View File

@ -0,0 +1,19 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import * as ts from 'typescript';
import {ImportManagerUpdateRecorder} from '../../utils/import_manager';
/**
* Update recorder interface that is used to transform source files
* in a non-colliding way.
*/
export interface UpdateRecorder extends ImportManagerUpdateRecorder {
addClassDecorator(node: ts.ClassDeclaration, text: string): void;
commitUpdate(): void;
}

View File

@ -1,71 +0,0 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import * as ts from 'typescript';
import {getAngularDecorators} from '../../utils/ng_decorators';
/** Name of the decorator that should be added to undecorated classes. */
export const FALLBACK_DECORATOR = 'Directive';
/** Finds all of the undecorated classes that have decorated fields within a file. */
export function getUndecoratedClassesWithDecoratedFields(
sourceFile: ts.SourceFile, typeChecker: ts.TypeChecker) {
const classes: UndecoratedClassWithDecoratedFields[] = [];
sourceFile.forEachChild(function walk(node: ts.Node) {
if (ts.isClassDeclaration(node) &&
(!node.decorators || !getAngularDecorators(typeChecker, node.decorators).length)) {
for (const member of node.members) {
const angularDecorators =
member.decorators && getAngularDecorators(typeChecker, member.decorators);
if (angularDecorators && angularDecorators.length) {
classes.push(
{classDeclaration: node, importDeclaration: angularDecorators[0].importNode});
return;
}
}
}
node.forEachChild(walk);
});
return classes;
}
/** Checks whether an import declaration has an import with a certain name. */
export function hasNamedImport(declaration: ts.ImportDeclaration, symbolName: string): boolean {
const namedImports = getNamedImports(declaration);
if (namedImports) {
return namedImports.elements.some(element => {
const {name, propertyName} = element;
return propertyName ? propertyName.text === symbolName : name.text === symbolName;
});
}
return false;
}
/** Extracts the NamedImports node from an import declaration. */
export function getNamedImports(declaration: ts.ImportDeclaration): ts.NamedImports|null {
const namedBindings = declaration.importClause && declaration.importClause.namedBindings;
return (namedBindings && ts.isNamedImports(namedBindings)) ? namedBindings : null;
}
/** Adds a new import to a NamedImports node. */
export function addImport(declaration: ts.NamedImports, symbolName: string) {
return ts.updateNamedImports(declaration, [
...declaration.elements, ts.createImportSpecifier(undefined, ts.createIdentifier(symbolName))
]);
}
interface UndecoratedClassWithDecoratedFields {
classDeclaration: ts.ClassDeclaration;
importDeclaration: ts.ImportDeclaration;
}

View File

@ -64,7 +64,7 @@ describe('Google3 undecorated classes with decorated fields TSLint rule', () =>
expect(failures.length).toBe(1);
expect(failures[0])
.toBe('Classes with decorated fields must have an Angular decorator as well.');
.toBe('Class needs to be decorated with "@Directive()" because it uses Angular features.');
});
it(`should add an import for Directive if there isn't one already`, () => {
@ -97,6 +97,27 @@ describe('Google3 undecorated classes with decorated fields TSLint rule', () =>
expect(getFile('/index.ts')).toContain(`import { Directive, Input } from '@angular/core';`);
});
it('should not generate conflicting imports there is a different `Directive` symbol', async() => {
writeFile('/index.ts', `
import { HostBinding } from '@angular/core';
export class Directive {
// Simulates a scenario where a library defines a class named "Directive".
// We don't want to generate a conflicting import.
}
export class MyLibrarySharedBaseClass {
@HostBinding('class.active') isActive: boolean;
}
`);
runTSLint(true);
const fileContent = getFile('/index.ts');
expect(fileContent)
.toContain(`import { HostBinding, Directive as Directive_1 } from '@angular/core';`);
expect(fileContent).toMatch(/@Directive_1\(\)\s+export class MyLibrarySharedBaseClass/);
});
it('should add @Directive to undecorated classes that have @Input', () => {
writeFile('/index.ts', `
import { Input } from '@angular/core';
@ -229,4 +250,35 @@ describe('Google3 undecorated classes with decorated fields TSLint rule', () =>
expect(getFile('/index.ts')).toContain(`@Directive()\nexport class Base {`);
});
it('should add @Directive to undecorated derived classes of a migrated class', async() => {
writeFile('/index.ts', `
import { Input, Directive, NgModule } from '@angular/core';
export class Base {
@Input() isActive: boolean;
}
export class DerivedA extends Base {}
export class DerivedB extends DerivedA {}
export class DerivedC extends DerivedB {}
@Directive({selector: 'my-comp'})
export class MyComp extends DerivedC {}
export class MyCompWrapped extends MyComp {}
@NgModule({declarations: [MyComp, MyCompWrapped]})
export class AppModule {}
`);
runTSLint(true);
const fileContent = getFile('/index.ts');
expect(fileContent).toContain(`import { Input, Directive, NgModule } from '@angular/core';`);
expect(fileContent).toMatch(/@Directive\(\)\s+export class Base/);
expect(fileContent).toMatch(/@Directive\(\)\s+export class DerivedA/);
expect(fileContent).toMatch(/@Directive\(\)\s+export class DerivedB/);
expect(fileContent).toMatch(/@Directive\(\)\s+export class DerivedC/);
expect(fileContent).toMatch(/}\s+@Directive\(\{selector: 'my-comp'}\)\s+export class MyComp/);
expect(fileContent).toMatch(/}\s+export class MyCompWrapped/);
});
});

View File

@ -74,6 +74,27 @@ describe('Undecorated classes with decorated fields migration', () => {
.toContain(`import { Directive, Input } from '@angular/core';`);
});
it('should not generate conflicting imports there is a different `Directive` symbol', async() => {
writeFile('/index.ts', `
import { HostBinding } from '@angular/core';
export class Directive {
// Simulates a scenario where a library defines a class named "Directive".
// We don't want to generate a conflicting import.
}
export class MyLibrarySharedBaseClass {
@HostBinding('class.active') isActive: boolean;
}
`);
await runMigration();
const fileContent = tree.readContent('/index.ts');
expect(fileContent)
.toContain(`import { HostBinding, Directive as Directive_1 } from '@angular/core';`);
expect(fileContent).toMatch(/@Directive_1\(\)\s+export class MyLibrarySharedBaseClass/);
});
it('should add @Directive to undecorated classes that have @Input', async() => {
writeFile('/index.ts', `
import { Input } from '@angular/core';
@ -206,6 +227,38 @@ describe('Undecorated classes with decorated fields migration', () => {
expect(tree.readContent('/index.ts')).toContain(`@Directive()\nexport class Base {`);
});
it('should add @Directive to undecorated derived classes of a migrated class', async() => {
writeFile('/index.ts', `
import { Input, Directive, NgModule } from '@angular/core';
export class Base {
@Input() isActive: boolean;
}
export class DerivedA extends Base {}
export class DerivedB extends DerivedA {}
export class DerivedC extends DerivedB {}
@Directive({selector: 'my-comp'})
export class MyComp extends DerivedC {}
export class MyCompWrapped extends MyComp {}
@NgModule({declarations: [MyComp, MyCompWrapped]})
export class AppModule {}
`);
await runMigration();
const fileContent = tree.readContent('/index.ts');
expect(fileContent).toContain(`import { Input, Directive, NgModule } from '@angular/core';`);
expect(fileContent).toMatch(/@Directive\(\)\s+export class Base/);
expect(fileContent).toMatch(/@Directive\(\)\s+export class DerivedA/);
expect(fileContent).toMatch(/@Directive\(\)\s+export class DerivedB/);
expect(fileContent).toMatch(/@Directive\(\)\s+export class DerivedC/);
expect(fileContent).toMatch(/}\s+@Directive\(\{selector: 'my-comp'}\)\s+export class MyComp/);
expect(fileContent).toMatch(/}\s+export class MyCompWrapped/);
});
function writeFile(filePath: string, contents: string) {
host.sync.write(normalize(filePath), virtualFs.stringToFileBuffer(contents));
}