fix(ng-update): properly handle update from different working directory

In some situations, developers will run `ng update` from a sub directory
of the project. This currently results in a noop migration as file
system paths were computed incorrectly. This is because ng-update
always assumed that the CWD is the workspace root in the real file
system. This is not the case and therefore threw off path resolution.

We fix this by fully relying on the virtual file system now. Previously
we mixed virtual and real file system as TypeScript relied on the
real file system for parsing configuration, matching and reading files.

This commit adds a new compiler host for config parsing and program
creation that works fully with a virtual file system. This is something
we had planned a long time ago as schematics should never deal with
the real file system. We can re-use this logic in other repositories
too (such as Angular framework, potentially the CLI, and universal
schematics). Ideally we will share this as part of better tooling for
migrations.

Fixes #19779.

Author: devversion

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

@@ -6,37 +6,26 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {normalize, Path, relative} from '@angular-devkit/core';
+import {normalize, Path, PathIsDirectoryException} from '@angular-devkit/core';
 import {Tree, UpdateRecorder} from '@angular-devkit/schematics';
+import {DirectoryEntry, FileSystem} from '../update-tool/file-system';
 import * as path from 'path';
-import {FileSystem} from '../update-tool/file-system';
 
 /**
  * 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 extends FileSystem<Path> {
+export class DevkitFileSystem extends FileSystem {
   private _updateRecorderCache = new Map<string, UpdateRecorder>();
-  private _workspaceFsPath: Path;
 
-  constructor(private _tree: Tree, workspaceFsPath: string) {
+  constructor(private _tree: Tree) {
     super();
-    this._workspaceFsPath = normalize(workspaceFsPath);
   }
 
   resolve(...segments: string[]): Path {
     // Note: We use `posix.resolve` as the devkit paths are using posix separators.
-    const resolvedPath = normalize(path.posix.resolve(...segments.map(normalize)));
-    // If the resolved path points to the workspace root, then this is an absolute disk
-    // path and we need to compute a devkit tree relative path.
-    if (resolvedPath.startsWith(this._workspaceFsPath)) {
-      return relative(this._workspaceFsPath, resolvedPath);
-    }
-    // Otherwise we know that the path is absolute (due to the resolve), and that it
-    // refers to an absolute devkit tree path (like `/angular.json`). We keep those
-    // unmodified as they are already resolved workspace paths.
-    return resolvedPath;
+    return normalize(path.posix.resolve('/', ...segments.map(normalize)));
   }
 
   edit(filePath: Path) {
@@ -53,8 +42,18 @@ export class DevkitFileSystem extends FileSystem<Path> {
     this._updateRecorderCache.clear();
   }
 
-  exists(filePath: Path) {
-    return this._tree.exists(filePath);
+  exists(fileOrDirPath: Path) {
+    // The devkit tree does not expose an API for checking whether a given
+    // directory exists. It throws a specific error though if a directory
+    // is being read as a file. We use that to check if a directory exists.
+    try {
+      return this._tree.get(fileOrDirPath) !== null;
+    } catch (e) {
+      if (e instanceof PathIsDirectoryException) {
+        return true;
+      }
+    }
+    return false;
   }
 
   overwrite(filePath: Path, content: string) {
@@ -73,4 +72,9 @@ export class DevkitFileSystem extends FileSystem<Path> {
     const buffer = this._tree.read(filePath);
     return buffer !== null ? buffer.toString() : null;
   }
+
+  readDirectory(dirPath: Path): DirectoryEntry {
+    const {subdirs: directories, subfiles: files} = this._tree.getDir(dirPath);
+    return {directories, files};
+  }
 }

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

@@ -9,17 +9,16 @@
 import {Rule, SchematicContext, Tree} from '@angular-devkit/schematics';
 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 {UpdateProject} from '../update-tool';
+import {WorkspacePath} from '../update-tool/file-system';
 import {MigrationCtor} from '../update-tool/migration';
 import {TargetVersion} from '../update-tool/target-version';
-import {WorkspacePath} from '../update-tool/file-system';
 import {getTargetTsconfigPath, getWorkspaceConfigGracefully} from '../utils/project-tsconfig-paths';
 
 import {DevkitFileSystem} from './devkit-file-system';
 import {DevkitContext, DevkitMigration, DevkitMigrationCtor} from './devkit-migration';
+import {findStylesheetFiles} from './find-stylesheets';
 import {AttributeSelectorsMigration} from './migrations/attribute-selectors';
 import {ClassInheritanceMigration} from './migrations/class-inheritance';
 import {ClassNamesMigration} from './migrations/class-names';
@@ -74,9 +73,7 @@ export function createMigrationSchematicRule(
     // necessary because multiple TypeScript projects can contain the same source file and
     // we don't want to check these again, as this would result in duplicated failure messages.
     const analyzedFiles = new Set<WorkspacePath>();
-    // The CLI uses the working directory as the base directory for the virtual file system tree.
-    const workspaceFsPath = process.cwd();
-    const fileSystem = new DevkitFileSystem(tree, workspaceFsPath);
+    const fileSystem = new DevkitFileSystem(tree);
     const projectNames = Object.keys(workspace.projects);
     const migrations: NullableDevkitMigration[] = [...cdkMigrations, ...extraMigrations];
     let hasFailures = false;
@@ -123,12 +120,9 @@ export function createMigrationSchematicRule(
 
     /** Runs the migrations for the specified workspace project. */
     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);
+                           tsconfigPath: WorkspacePath, isTestTarget: boolean) {
+      const program = UpdateProject.createProgramFromTsconfig(tsconfigPath, fileSystem);
       const updateContext: DevkitContext = {
-        workspaceFsPath,
         isTestTarget,
         projectName,
         project,
@@ -145,13 +139,9 @@ export function createMigrationSchematicRule(
 
       // In some applications, developers will have global stylesheets which are not
       // specified in any Angular component. Therefore we glob up all CSS and SCSS files
-      // outside of node_modules and dist. The files will be read by the individual
-      // stylesheet rules and checked.
+      // in the project and migrate them if needed.
       // TODO: rework this to collect global stylesheets from the workspace config. COMP-280.
-      const additionalStylesheets = globSync(
-        '!(node_modules|dist)/**/*.+(css|scss)',
-        {absolute: true, cwd: projectRootFsPath, nodir: true});
-
+      const additionalStylesheets = findStylesheetFiles(tree, project.root);
       const result =
         updateProject.migrate(migrations, targetVersion, upgradeData, additionalStylesheets);
 

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

@@ -17,8 +17,6 @@ export type DevkitContext = {
   projectName: string;
   /** Workspace project the migrations run against. */
   project: WorkspaceProject,
-  /** Absolute file system path to the workspace */
-  workspaceFsPath: string,
   /** Whether the migrations run for a test target. */
   isTestTarget: boolean,
 };

src/cdk/schematics/ng-update/find-stylesheets.ts

@@ -0,0 +1,30 @@
+/**
+ * @license
+ * Copyright Google LLC 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 {join} from '@angular-devkit/core';
+import {Tree} from '@angular-devkit/schematics';
+
+/** Regular expression that matches stylesheet paths */
+const STYLESHEET_REGEX = /.*\.(css|scss)/;
+
+/** Finds stylesheets in the given directory from within the specified tree. */
+export function findStylesheetFiles(tree: Tree, baseDir: string): string[] {
+  const result: string[] = [];
+  const visitDir = dirPath => {
+    const {subfiles, subdirs} = tree.getDir(dirPath);
+    result.push(...subfiles.filter(f => STYLESHEET_REGEX.test(f)));
+    subdirs.forEach(fragment => {
+      // Do not visit directories or files inside node modules or `dist/` folders.
+      if (fragment !== 'node_modules' && fragment !== 'dist') {
+        visitDir(join(dirPath, fragment));
+      }
+    });
+  };
+  visitDir(baseDir);
+  return result;
+}

src/cdk/schematics/ng-update/migrations/attribute-selectors.ts

@@ -59,13 +59,12 @@ export class AttributeSelectorsMigration extends Migration<UpgradeData> {
     }
 
     const literalText = literal.getText();
-    const filePath = literal.getSourceFile().fileName;
+    const filePath = this.fileSystem.resolve(literal.getSourceFile().fileName);
 
     this.data.forEach(selector => {
       findAllSubstringIndices(literalText, selector.replace)
           .map(offset => literal.getStart() + offset)
-          .forEach(start => this._replaceSelector(
-              this.fileSystem.resolve(filePath), start, selector));
+          .forEach(start => this._replaceSelector(filePath, start, selector));
     });
   }
 

src/cdk/schematics/ng-update/migrations/css-selectors.ts

@@ -61,7 +61,7 @@ export class CssSelectorsMigration extends Migration<UpgradeData> {
     }
 
     const textContent = node.getText();
-    const filePath = node.getSourceFile().fileName;
+    const filePath = this.fileSystem.resolve(node.getSourceFile().fileName);
 
     this.data.forEach(data => {
       if (data.whitelist && !data.whitelist.strings) {
@@ -70,7 +70,7 @@ export class CssSelectorsMigration extends Migration<UpgradeData> {
 
       findAllSubstringIndices(textContent, data.replace)
           .map(offset => node.getStart() + offset)
-          .forEach(start => this._replaceSelector(this.fileSystem.resolve(filePath), start, data));
+          .forEach(start => this._replaceSelector(filePath, start, data));
     });
   }
 

src/cdk/schematics/ng-update/migrations/element-selectors.ts

@@ -53,13 +53,12 @@ export class ElementSelectorsMigration extends Migration<UpgradeData> {
     }
 
     const textContent = node.getText();
-    const filePath = node.getSourceFile().fileName;
+    const filePath = this.fileSystem.resolve(node.getSourceFile().fileName);
 
     this.data.forEach(selector => {
       findAllSubstringIndices(textContent, selector.replace)
           .map(offset => node.getStart() + offset)
-          .forEach(start => this._replaceSelector(
-              this.fileSystem.resolve(filePath), start, selector));
+          .forEach(start => this._replaceSelector(filePath, start, selector));
     });
   }
 

src/cdk/schematics/ng-update/test-cases/misc/external-resource-resolution.spec.ts

@@ -5,7 +5,7 @@ import {createTestCaseSetup} from '../../../testing';
 describe('ng-update external resource resolution', () => {
 
   it('should properly resolve referenced resources in components', async () => {
-    const {runFixers, writeFile, removeTempDir, appTree} = await createTestCaseSetup(
+    const {runFixers, writeFile, appTree} = await createTestCaseSetup(
       'migration-v6', MIGRATION_PATH,
       [resolveBazelPath(__dirname, './external-resource-resolution_input.ts')]);
 
@@ -24,7 +24,5 @@ describe('ng-update external resource resolution', () => {
         .toBe(expected, 'Expected relative paths with parent segments to work.');
     expect(appTree.readContent('/projects/cdk-testing/src/test-cases/local.html'))
         .toBe(expected, 'Expected relative paths without explicit dot to work.');
-
-    removeTempDir();
   });
 });

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

@@ -6,7 +6,7 @@ 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(
+    const {runFixers, writeFile, appTree} = await createTestCaseSetup(
       'migration-v6', MIGRATION_PATH,
       [resolveBazelPath(__dirname, './global-stylesheets_input.ts')]);
 
@@ -25,12 +25,10 @@ describe('global stylesheets migration', () => {
     // the same replacements were recorded for the same source file.
     expect(appTree.readContent(testStylesheetPath))
         .toBe(`[cdkPortalOutlet] {\n  color: red;\n}\n`);
-
-    removeTempDir();
   });
 
   it('should not check stylesheets outside of project target', async () => {
-    const {runFixers, writeFile, removeTempDir, appTree} = await createTestCaseSetup(
+    const {runFixers, writeFile, appTree} = await createTestCaseSetup(
       'migration-v6', MIGRATION_PATH, []);
     const subProjectStylesheet = '[cdkPortalHost] {\n  color: red;\n}\n';
 
@@ -43,7 +41,5 @@ describe('global stylesheets migration', () => {
     // if the external stylesheet that is not of a project target would have been checked
     // by accident, the stylesheet would differ from the original file content.
     expect(appTree.readContent('/sub_project/assets/test.css')).toBe(subProjectStylesheet);
-
-    removeTempDir();
   });
 });

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

@@ -4,7 +4,7 @@ import {createTestCaseSetup} from '../../../testing';
 
 describe('v6 method call checks', () => {
   it('should properly report invalid method calls', async () => {
-    const {runFixers, removeTempDir} = await createTestCaseSetup(
+    const {runFixers} = await createTestCaseSetup(
         'migration-v6', MIGRATION_PATH,
         [resolveBazelPath(__dirname, './method-call-checks_input.ts')]);
 
@@ -14,7 +14,5 @@ describe('v6 method call checks', () => {
         /@15:5 - Found call to "FocusMonitor\.monitor".*renderer.*has been removed/);
     expect(logOutput).toMatch(
         /@16:5 - Found call to "FocusMonitor\.monitor".*renderer.*has been removed/);
-
-    removeTempDir();
   });
 });

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

@@ -5,12 +5,10 @@
  * 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 {getSystemPath, JsonParseMode, normalize, parseJson, Path} from '@angular-devkit/core';
-import {TempScopedNodeJsSyncHost} from '@angular-devkit/core/node/testing';
-import * as virtualFs from '@angular-devkit/core/src/virtual-fs/host';
+import {getSystemPath, JsonParseMode, parseJson, Path} from '@angular-devkit/core';
 import {HostTree, Tree} from '@angular-devkit/schematics';
 import {SchematicTestRunner, UnitTestTree} from '@angular-devkit/schematics/testing';
-import {readFileSync, removeSync} from 'fs-extra';
+import {readFileSync} from 'fs-extra';
 import {sync as globSync} from 'glob';
 import {basename, extname, join, relative, sep} from 'path';
 import {EMPTY} from 'rxjs';
@@ -38,28 +36,22 @@ export function readFileContent(filePath: string): string {
  * schematic tree. This would allow us to fully take advantage of the virtual file system.
  */
 export async function createFileSystemTestApp(runner: SchematicTestRunner) {
-  const tempFileSystemHost = new TempScopedNodeJsSyncHost();
-  const hostTree = new HostTree(tempFileSystemHost);
+  const hostTree = new HostTree();
   const appTree: UnitTestTree = await createTestApp(runner, {name: 'cdk-testing'}, hostTree);
-  const tempPath = getSystemPath(tempFileSystemHost.root);
 
   // Since the TypeScript compiler API expects all files to be present on the real file system, we
   // map every file in the app tree to a temporary location on the file system.
   appTree.files.forEach(f => writeFile(f, appTree.readContent(f)));
 
   return {
     appTree,
-    tempFileSystemHost,
-    tempPath,
     writeFile,
-    removeTempDir: () => removeSync(tempPath),
   };
 
   function writeFile(filePath: string, content: string) {
     // Update the temp file system host to reflect the changes in the real file system.
     // This is still necessary since we depend on the real file system for parsing the
     // TypeScript project.
-    tempFileSystemHost.sync.write(normalize(filePath), virtualFs.stringToFileBuffer(content));
     if (hostTree.exists(filePath)) {
       hostTree.overwrite(filePath, content);
     } else {
@@ -72,12 +64,11 @@ export async function createTestCaseSetup(migrationName: string, collectionPath:
                                    inputFiles: string[]) {
 
   const runner = new SchematicTestRunner('schematics', collectionPath);
-  const initialWorkingDir = process.cwd();
 
   let logOutput = '';
   runner.logger.subscribe(entry => logOutput += `${entry.message}\n`);
 
-  const {appTree, tempPath, writeFile, removeTempDir} =
+  const {appTree, writeFile} =
     await createFileSystemTestApp(runner);
 
   _patchTypeScriptDefaultLib(appTree);
@@ -105,10 +96,6 @@ export async function createTestCaseSetup(migrationName: string, collectionPath:
   writeFile(testAppTsconfigPath, JSON.stringify(testAppTsconfig, null, 2));
 
   const runFixers = async function() {
-    // Switch to the new temporary directory to simulate that "ng update" is ran
-    // from within the project.
-    process.chdir(tempPath);
-
     // Patch "executePostTasks" to do nothing. This is necessary since
     // we cannot run the node install task in unit tests. Rather we just
     // assert that certain async post tasks are scheduled.
@@ -117,13 +104,10 @@ export async function createTestCaseSetup(migrationName: string, collectionPath:
 
     await runner.runSchematicAsync(migrationName, {}, appTree).toPromise();
 
-    // Switch back to the initial working directory.
-    process.chdir(initialWorkingDir);
-
     return {logOutput};
   };
 
-  return {runner, appTree, writeFile, tempPath, removeTempDir, runFixers};
+  return {runner, appTree, writeFile, runFixers};
 }
 
 /**
@@ -192,21 +176,17 @@ export function defineJasmineTestCases(versionName: string, collectionFile: stri
 
   let appTree: UnitTestTree;
   let testCasesOutputPath: string;
-  let cleanupTestApp: () => void;
 
   beforeAll(async () => {
-    const {appTree: _tree, runFixers, removeTempDir} =
+    const {appTree: _tree, runFixers} =
       await createTestCaseSetup(`migration-${versionName}`, collectionFile, inputFiles);
 
     await runFixers();
 
     appTree = _tree;
     testCasesOutputPath = '/projects/cdk-testing/src/test-cases/';
-    cleanupTestApp = removeTempDir;
   });
 
-  afterAll(() => cleanupTestApp());
-
   // Iterates through every test case directory and generates a jasmine test block that will
   // verify that the update schematics properly updated the test input to the expected output.
   inputFiles.forEach(inputFile => {