Address feedback

Author: devversion

src/cdk/schematics/ng-update/devkit-file-system.ts

@@ -11,7 +11,11 @@ import {Tree, UpdateRecorder} from '@angular-devkit/schematics';
 import {relative} from 'path';
 import {FileSystem} from '../update-tool/file-system';
 
-/** File system that leverages the virtual tree from the CLI devkit. */
+/**
+ * File system that leverages the virtual tree from the CLI devkit. This file
+ * system is commonly used by `ng update` migrations that run as part of the
+ * Angular CLI.
+ */
 export class DevkitFileSystem implements FileSystem {
   private _updateRecorderCache = new Map<string, UpdateRecorder>();
 

src/cdk/schematics/ng-update/devkit-migration-rule.ts

@@ -11,6 +11,7 @@ import {NodePackageInstallTask} from '@angular-devkit/schematics/tasks';
 import {WorkspaceProject} from '@schematics/angular/utility/workspace-models';
 import {sync as globSync} from 'glob';
 import {join} from 'path';
+import {DevkitMigration, MigrationCtor} from '..';
 
 import {UpdateProject} from '../update-tool';
 import {TargetVersion} from '../update-tool/target-version';
@@ -33,7 +34,7 @@ import {UpgradeData} from './upgrade-data';
 
 
 /** List of migrations which run for the CDK update. */
-export const cdkMigrations: DevkitMigrationCtor<UpgradeData>[] = [
+export const cdkMigrations: MigrationCtor<UpgradeData>[] = [
   AttributeSelectorsMigration,
   ClassInheritanceMigration,
   ClassNamesMigration,
@@ -47,7 +48,7 @@ export const cdkMigrations: DevkitMigrationCtor<UpgradeData>[] = [
   PropertyNamesMigration,
 ];
 
-type NullableMigration = DevkitMigrationCtor<UpgradeData|null>;
+export type NullableDevkitMigration = MigrationCtor<UpgradeData|null, DevkitContext>;
 
 type PostMigrationFn =
     (context: SchematicContext, targetVersion: TargetVersion, hasFailure: boolean) => void;
@@ -57,8 +58,8 @@ type PostMigrationFn =
  * specified target version.
  */
 export function createMigrationSchematicRule(
-    targetVersion: TargetVersion, extraMigrations: NullableMigration[], upgradeData: UpgradeData,
-    onMigrationCompleteFn?: PostMigrationFn): Rule {
+    targetVersion: TargetVersion, extraMigrations: NullableDevkitMigration[],
+    upgradeData: UpgradeData, onMigrationCompleteFn?: PostMigrationFn): Rule {
   return async (tree: Tree, context: SchematicContext) => {
     const logger = context.logger;
     const workspace = getWorkspaceConfigGracefully(tree);
@@ -76,7 +77,7 @@ export function createMigrationSchematicRule(
     const workspaceFsPath = process.cwd();
     const fileSystem = new DevkitFileSystem(tree, workspaceFsPath);
     const projectNames = Object.keys(workspace.projects);
-    const migrations: NullableMigration[] = [...cdkMigrations, ...extraMigrations];
+    const migrations: NullableDevkitMigration[] = [...cdkMigrations, ...extraMigrations];
     let hasFailures = false;
 
     for (const projectName of projectNames) {
@@ -89,18 +90,19 @@ export function createMigrationSchematicRule(
         continue;
       }
       if (buildTsconfigPath !== null) {
-        runMigrations(project, buildTsconfigPath, false);
+        runMigrations(project, projectName, buildTsconfigPath, false);
       }
       if (testTsconfigPath !== null) {
-        runMigrations(project, testTsconfigPath, true);
+        runMigrations(project, projectName, testTsconfigPath, true);
       }
     }
 
     let runPackageManager = false;
-    // Run the global post migration static members for all migrations.
+    // Run the global post migration static members for all
+    // registered devkit migrations.
     migrations.forEach(m => {
-      const actionResult =
-          m.globalPostMigration !== undefined ? m.globalPostMigration(tree, context) : null;
+      const actionResult = isDevkitMigration(m) && m.globalPostMigration !== undefined ?
+          m.globalPostMigration(tree, context) : null;
       if (actionResult) {
         runPackageManager = runPackageManager || actionResult.runPackageManager;
       }
@@ -119,13 +121,15 @@ export function createMigrationSchematicRule(
     }
 
     /** Runs the migrations for the specified workspace project. */
-    function runMigrations(project: WorkspaceProject, tsconfigPath: string, isTestTarget: boolean) {
+    function runMigrations(project: WorkspaceProject, projectName: string,
+                           tsconfigPath: string, isTestTarget: boolean) {
       const projectRootFsPath = join(workspaceFsPath, project.root);
       const tsconfigFsPath = join(workspaceFsPath, tsconfigPath);
       const program = UpdateProject.createProgramFromTsconfig(tsconfigFsPath, fileSystem);
       const updateContext: DevkitContext = {
         workspaceFsPath,
         isTestTarget,
+        projectName,
         project,
         tree,
       };
@@ -159,3 +163,9 @@ export function createMigrationSchematicRule(
     }
   };
 }
+
+/** Whether the given migration type refers to a devkit migration */
+export function isDevkitMigration(value: MigrationCtor<any, any>)
+    : value is DevkitMigrationCtor<any> {
+  return DevkitMigration.isPrototypeOf(value);
+}

src/cdk/schematics/ng-update/devkit-migration.ts

@@ -13,6 +13,8 @@ import {Constructor, Migration, PostMigrationAction} from '../update-tool/migrat
 export type DevkitContext = {
   /** Devkit tree for the current migrations. Can be used to insert/remove files. */
   tree: Tree,
+  /** Name of the project the migrations run against. */
+  projectName: string;
   /** Workspace project the migrations run against. */
   project: WorkspaceProject,
   /** Absolute file system path to the workspace */
@@ -22,6 +24,13 @@ export type DevkitContext = {
 };
 
 export abstract class DevkitMigration<Data> extends Migration<Data, DevkitContext> {
+
+  /** Prints an informative message with context on the current target. */
+  protected printInfo(text: string) {
+    const targetName = this.context.isTestTarget ? 'test' : 'build';
+    this.logger.info(`- ${this.context.projectName}@${targetName}: ${text}`);
+  }
+
   /**
    * Optional static method that will be called once the migration of all project
    * targets has been performed. This method can be used to make changes respecting the

src/cdk/schematics/ng-update/test-cases/misc/global-stylesheets.spec.ts

@@ -1,20 +1,21 @@
+import {resolveBazelPath} from '@angular/cdk/schematics/testing';
 import {readFileSync} from 'fs';
-import {resolveBazel} from '@angular/cdk/schematics/testing';
 import {MIGRATION_PATH} from '../../../index.spec';
 import {createTestCaseSetup} from '../../../testing';
 
 describe('global stylesheets migration', () => {
 
   it('should not check stylesheet twice if referenced in component', async () => {
     const {runFixers, writeFile, removeTempDir, appTree} = await createTestCaseSetup(
-      'migration-v6', MIGRATION_PATH, [resolveBazel(__dirname, './global-stylesheets_input.ts')]);
+      'migration-v6', MIGRATION_PATH,
+      [resolveBazelPath(__dirname, './global-stylesheets_input.ts')]);
 
     const testStylesheetPath = 'projects/cdk-testing/src/test-cases/global-stylesheets-test.scss';
 
     // Copy the test stylesheets file into the test CLI application. That way it will
     // be picked up by the update-tool.
     writeFile(testStylesheetPath,
-        readFileSync(resolveBazel(__dirname, './global-stylesheets-test.scss'), 'utf8'));
+        readFileSync(resolveBazelPath(__dirname, './global-stylesheets-test.scss'), 'utf8'));
     writeFile('/projects/cdk-testing/third_party/materialize.css/bundle.css', '');
 
     await runFixers();

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

@@ -1,11 +1,12 @@
-import {resolveBazel} from '@angular/cdk/schematics/testing';
+import {resolveBazelPath} from '@angular/cdk/schematics/testing';
 import {MIGRATION_PATH} from '../../../index.spec';
 import {createTestCaseSetup} from '../../../testing';
 
 describe('v6 method call checks', () => {
   it('should properly report invalid method calls', async () => {
     const {runFixers, removeTempDir} = await createTestCaseSetup(
-        'migration-v6', MIGRATION_PATH, [resolveBazel(__dirname, './method-call-checks_input.ts')]);
+        'migration-v6', MIGRATION_PATH,
+        [resolveBazelPath(__dirname, './method-call-checks_input.ts')]);
 
     const {logOutput} = await runFixers();
 

src/cdk/schematics/testing/index.ts

@@ -10,4 +10,4 @@ export * from './post-scheduled-tasks';
 export * from './test-app';
 export * from './test-case-setup';
 export * from './file-content';
-export * from './resolve-bazel';
+export * from './resolve-bazel-path';

src/cdk/schematics/testing/resolve-bazel.ts renamed to src/cdk/schematics/testing/resolve-bazel-path.ts

@@ -12,9 +12,9 @@ import * as path from 'path';
  * Resolves a given path through Bazel while respecting the specified parent directory.
  * Usually `require.resolve` could just be used for resolving runfiles relatively, but support
  * for this has been removed and absolute manifest paths or the runfile helpers are now needed
- * on Windows. This helper provides a resolution method that works similar on all platforms.
+ * on Windows. This helper provides a resolution method that works the same on all platforms.
  */
-export function resolveBazel(parent: string, relativePath: string) {
+export function resolveBazelPath(parent: string, relativePath: string) {
   if (process.env['RUNFILES_MANIFEST_ONLY'] !== '1') {
     return path.join(parent, relativePath);
   }

src/cdk/schematics/update-tool/index.ts

@@ -42,27 +42,28 @@ export class UpdateProject<Context> {
    */
   migrate<Data>(migrationTypes: MigrationCtor<Data, Context>[], target: TargetVersion, data: Data,
       additionalStylesheetPaths?: string[]): {hasFailures: boolean} {
-    const migrations: Migration<Data, Context>[] = [];
-
-    // Create instances of all specified migrations.
-    for (const ctor of migrationTypes) {
-      const instance = new ctor(this._program, this._typeChecker, target, this._context,
-          data, this._fileSystem, this._logger);
-      instance.init();
-      if (instance.enabled) {
-        migrations.push(instance);
-      }
-    }
-
+    // Create instances of the specified migrations.
+    const migrations = this._createMigrations(migrationTypes, target, data);
+    // Creates the component resource collector. The collector can visit arbitrary
+    // TypeScript nodes and will find Angular component resources. Resources include
+    // templates and stylesheets. It also captures inline stylesheets and templates.
+    const resourceCollector = new ComponentResourceCollector(this._typeChecker, this._fileSystem);
+    // Collect all of the TypeScript source files we want to migrate. We don't
+    // migrate type definition files, or source files from external libraries.
     const sourceFiles = this._program.getSourceFiles().filter(
       f => !f.isDeclarationFile && !this._program.isSourceFileFromExternalLibrary(f));
-    const resourceCollector = new ComponentResourceCollector(this._typeChecker, this._fileSystem);
+
+    // Helper function that visits a given TypeScript node and collects all referenced
+    // component resources (i.e. stylesheets or templates). Additionally, the helper
+    // visits the node in each instantiated migration.
     const visitNodeAndCollectResources = (node: ts.Node) => {
       migrations.forEach(r => r.visitNode(node));
       ts.forEachChild(node, visitNodeAndCollectResources);
       resourceCollector.visitNode(node);
     };
 
+    // Walk through all source file, if it has not been visited before, and
+    // visit found nodes while collecting potential resources.
     sourceFiles.forEach(sourceFile => {
       const resolvedPath = this._fileSystem.resolve(sourceFile.fileName);
       // Do not visit source files which have been checked as part of a
@@ -73,6 +74,9 @@ export class UpdateProject<Context> {
       }
     });
 
+    // Walk through all resolved templates and visit them in each instantiated
+    // migration. Note that this can only happen after source files have been
+    // visited because we find templates through the TypeScript source files.
     resourceCollector.resolvedTemplates.forEach(template => {
       const resolvedPath = this._fileSystem.resolve(template.filePath);
       // Do not visit the template if it has been checked before. Inline
@@ -83,6 +87,9 @@ export class UpdateProject<Context> {
       }
     });
 
+    // Walk through all resolved stylesheets and visit them in each instantiated
+    // migration. Note that this can only happen after source files have been
+    // visited because we find stylesheets through the TypeScript source files.
     resourceCollector.resolvedStylesheets.forEach(stylesheet => {
       const resolvedPath = this._fileSystem.resolve(stylesheet.filePath);
       // Do not visit the stylesheet if it has been checked before. Inline
@@ -93,8 +100,10 @@ export class UpdateProject<Context> {
       }
     });
 
-    // In some applications, developers will have global stylesheets which are not specified
-    // in any Angular component. Therefore we allow for additional stylesheets being specified.
+    // In some applications, developers will have global stylesheets which are not
+    // specified in any Angular component. Therefore we allow for additional stylesheets
+    // being specified. We visit them in each migration unless they have been already
+    // discovered before as actual component resource.
     additionalStylesheetPaths.forEach(filePath => {
       const stylesheet = resourceCollector.resolveExternalStylesheet(filePath, null);
       const resolvedPath = this._fileSystem.resolve(filePath);
@@ -126,6 +135,24 @@ export class UpdateProject<Context> {
     };
   }
 
+  /**
+   * Creates instances of the given migrations with the specified target
+   * version and data.
+   */
+  private _createMigrations<Data>(types: MigrationCtor<Data, Context>[], target: TargetVersion,
+                                  data: Data): Migration<Data, Context>[] {
+    const result: Migration<Data, Context>[] = [];
+    for (const ctor of types) {
+      const instance = new ctor(this._program, this._typeChecker, target, this._context,
+        data, this._fileSystem, this._logger);
+      instance.init();
+      if (instance.enabled) {
+        result.push(instance);
+      }
+    }
+    return result;
+  }
+
   /**
    * Creates a program form the specified tsconfig and patches the host
    * to read files through the given file system.

src/cdk/schematics/update-tool/migration.ts

@@ -84,10 +84,4 @@ export abstract class Migration<Data, Context = never> {
       message: message,
     });
   }
-
-  /** Prints the given message with "info" loglevel. */
-  printInfo(text: string) {
-    // TODO: FIND REPLACEMENT???
-    this.logger.info(`- ${text}`);
-  }
 }

src/material/schematics/ng-update/index.ts

@@ -9,9 +9,8 @@
 import {Rule, SchematicContext} from '@angular-devkit/schematics';
 import {
   createMigrationSchematicRule,
-  DevkitMigrationCtor,
-  TargetVersion,
-  UpgradeData
+  NullableDevkitMigration,
+  TargetVersion
 } from '@angular/cdk/schematics';
 import {HammerGesturesMigration} from './migrations/hammer-gestures-v9/hammer-gestures-migration';
 import {MiscClassInheritanceMigration} from './migrations/misc-checks/misc-class-inheritance';
@@ -28,7 +27,7 @@ import {
 
 import {materialUpgradeData} from './upgrade-data';
 
-const materialMigrations: DevkitMigrationCtor<UpgradeData|null>[] = [
+const materialMigrations: NullableDevkitMigration[] = [
   MiscClassInheritanceMigration,
   MiscClassNamesMigration,
   MiscImportsMigration,

src/material/schematics/ng-update/test-cases/misc/class-inheritance.spec.ts

@@ -1,11 +1,12 @@
-import {createTestCaseSetup, resolveBazel} from '@angular/cdk/schematics/testing';
+import {createTestCaseSetup, resolveBazelPath} from '@angular/cdk/schematics/testing';
 import {MIGRATION_PATH} from '../../../index.spec';
 
 describe('class inheritance misc checks', () => {
   describe('v6 class which extends MatFormFieldControl', () => {
     it('should report if class does not declare "shouldLabelFloat"', async () => {
       const {removeTempDir, runFixers} = await createTestCaseSetup(
-          'migration-v6', MIGRATION_PATH, [resolveBazel(__dirname, './class-inheritance_input.ts')]);
+          'migration-v6', MIGRATION_PATH,
+          [resolveBazelPath(__dirname, './class-inheritance_input.ts')]);
 
       const {logOutput} = await runFixers();
 

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

@@ -1,10 +1,11 @@
-import {createTestCaseSetup, resolveBazel} from '@angular/cdk/schematics/testing';
+import {createTestCaseSetup, resolveBazelPath} from '@angular/cdk/schematics/testing';
 import {MIGRATION_PATH} from '../../../index.spec';
 
 describe('constructor checks', () => {
   it('should properly report invalid constructor expression signatures', async () => {
     const {removeTempDir, runFixers} = await createTestCaseSetup(
-        'migration-v6', MIGRATION_PATH, [resolveBazel(__dirname, './constructor-checks_input.ts')]);
+        'migration-v6', MIGRATION_PATH,
+        [resolveBazelPath(__dirname, './constructor-checks_input.ts')]);
 
     const {logOutput} = await runFixers();
 

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

@@ -1,10 +1,10 @@
-import {createTestCaseSetup, resolveBazel} from '@angular/cdk/schematics/testing';
+import {createTestCaseSetup, resolveBazelPath} from '@angular/cdk/schematics/testing';
 import {MIGRATION_PATH} from '../../../index.spec';
 
 describe('v6 import misc checks', () => {
   it('should report imports for deleted animation constants', async () => {
     const {removeTempDir, runFixers} = await createTestCaseSetup(
-        'migration-v6', MIGRATION_PATH, [resolveBazel(__dirname, './import-checks_input.ts')]);
+        'migration-v6', MIGRATION_PATH, [resolveBazelPath(__dirname, './import-checks_input.ts')]);
 
     const {logOutput} = await runFixers();
 

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

@@ -1,10 +1,15 @@
-import {createTestCaseSetup, readFileContent, resolveBazel} from '@angular/cdk/schematics/testing';
+import {
+  createTestCaseSetup,
+  readFileContent,
+  resolveBazelPath
+} from '@angular/cdk/schematics/testing';
 import {MIGRATION_PATH} from '../../../../index.spec';
 
 describe('v8 material imports', () => {
   it('should re-map top-level material imports to the proper entry points', async () => {
     const {runFixers, appTree, writeFile, removeTempDir} = await createTestCaseSetup(
-        'migration-v8', MIGRATION_PATH, [resolveBazel(__dirname, './material-imports_input.ts')]);
+        'migration-v8', MIGRATION_PATH,
+        [resolveBazelPath(__dirname, './material-imports_input.ts')]);
     const materialPath = '/node_modules/@angular/material';
 
     writeFile(`${materialPath}/index.d.ts`, `
@@ -28,7 +33,8 @@ describe('v8 material imports', () => {
     await runFixers();
 
     expect(appTree.readContent('/projects/cdk-testing/src/test-cases/material-imports_input.ts'))
-        .toBe(readFileContent(resolveBazel(__dirname, './material-imports_expected_output.ts')));
+        .toBe(readFileContent(
+            resolveBazelPath(__dirname, './material-imports_expected_output.ts')));
 
     removeTempDir();
   });