From 805d3aef8b722ab2b19e6bcb5e13c849b21f2abf Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Wed, 16 Jun 2021 03:45:40 +0200 Subject: [PATCH] fix(cdk/schematics): avoid runtime errors thrown by devkit tree (#22982) TypeScript resolves modules using a rather complicated module resolution algorithm. The algorithm tries various paths to determine a possible entry-point for a module. e.g. it also respects a containing `package.json` file, or respects the closest `node_modules` parent directory. In some situations, TypeScript could end up trying a path where a parent directory segment resolves to an existent file. e.g. consider the following directory structure: ``` node_modules/my-pkg/package.json node_modules/my-pkg/styles.css ``` TypeScript could end up trying a path like: `node_modules/my-pkg/styles.css/package.json` or `node_modules/my-pkg/styles.css/a/b/package.json`. This depends on how the module resolution executes, and how the module is referenced. In the example above though, TypeScript checks if the files exist. Our update logic delegates this check to our virtual file system. The virtual file system currently would throw an error by accident as it walks up the path and discovers that `styles.css` is not a directory, _but_ a file. This results in an error as seen in https://github.com/angular/components/issues/22919. Fixes #22919. --- .../ng-update/devkit-file-system.ts | 44 +++++++++---------- .../test-cases/misc/module-resolution.spec.ts | 24 ++++++++++ .../component-resource-collector.ts | 2 +- src/cdk/schematics/update-tool/file-system.ts | 6 ++- .../update-tool/utils/virtual-host.ts | 4 +- .../hammer-gestures-migration.ts | 6 +-- 6 files changed, 54 insertions(+), 32 deletions(-) create mode 100644 src/cdk/schematics/ng-update/test-cases/misc/module-resolution.spec.ts diff --git a/src/cdk/schematics/ng-update/devkit-file-system.ts b/src/cdk/schematics/ng-update/devkit-file-system.ts index a6646af6667c..926a006905f6 100644 --- a/src/cdk/schematics/ng-update/devkit-file-system.ts +++ b/src/cdk/schematics/ng-update/devkit-file-system.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {basename, dirname, normalize, NormalizedRoot, Path} from '@angular-devkit/core'; +import {normalize, Path} from '@angular-devkit/core'; import {Tree, UpdateRecorder} from '@angular-devkit/schematics'; import {DirectoryEntry, FileSystem} from '../update-tool/file-system'; import * as path from 'path'; @@ -42,10 +42,25 @@ export class DevkitFileSystem extends FileSystem { this._updateRecorderCache.clear(); } - exists(fileOrDirPath: Path) { - // We need to check for both file or directory existence, in order - // to comply with the expectation from the TypeScript compiler. - return this._tree.exists(fileOrDirPath) || this._isExistingDirectory(fileOrDirPath); + fileExists(filePath: Path) { + return this._tree.exists(filePath); + } + + directoryExists(dirPath: 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 { + this._tree.get(dirPath); + } catch (e) { + // Note: We do not use an `instanceof` check here. It could happen that the devkit version + // used by the CLI is different than the one we end up loading. This can happen depending + // on how Yarn/NPM hoists the NPM packages / whether there are multiple versions installed. + if (e instanceof Error && e.constructor.name === 'PathIsDirectoryException') { + return true; + } + } + return false; } overwrite(filePath: Path, content: string) { @@ -69,23 +84,4 @@ export class DevkitFileSystem extends FileSystem { const {subdirs: directories, subfiles: files} = this._tree.getDir(dirPath); return {directories, files}; } - - private _isExistingDirectory(dirPath: Path) { - if (dirPath === NormalizedRoot) { - return true; - } - - const parent = dirname(dirPath); - const dirName = basename(dirPath); - // TypeScript also checks potential entry points, so e.g. importing - // package.json will result in a lookup of /package.json/package.json - // and /package.json/index.ts. In order to avoid failure, we check if - // the parent is an existing file and return false, if that is the case. - if (this._tree.exists(parent)) { - return false; - } - - const dir = this._tree.getDir(parent); - return dir.subdirs.indexOf(dirName) !== -1; - } } diff --git a/src/cdk/schematics/ng-update/test-cases/misc/module-resolution.spec.ts b/src/cdk/schematics/ng-update/test-cases/misc/module-resolution.spec.ts new file mode 100644 index 000000000000..11f2c1f93b40 --- /dev/null +++ b/src/cdk/schematics/ng-update/test-cases/misc/module-resolution.spec.ts @@ -0,0 +1,24 @@ +import {MIGRATION_PATH} from '../../../paths'; +import {createTestCaseSetup} from '../../../testing'; + +describe('ng update typescript program module resolution', () => { + + // Regression test for: https://github.com/angular/components/issues/22919. + it('should not error if module resolution tries a non-existent path where a path segment ' + + 'matches an existing file', async () => { + const {runFixers, writeFile} = await createTestCaseSetup( + 'migration-v6', MIGRATION_PATH, []); + + writeFile('/node_modules/some-other-module/package.json', `{}`); + writeFile('/node_modules/some-other-module/styles.css', '') + + // We add an import to a non-existent sub-path of `some-other-module/styles`. The TypeScript + // module resolution logic could try various sub-paths. This previously resulted in an error + // as the devkit tree `getDir` logic accidentally walked up the path and threw an error if + // a path segment is an actual file. + writeFile('/projects/cdk-testing/src/main.ts', + `import 'some-other-module/styles.css/non/existent';`); + + await expectAsync(runFixers()).toBeResolved(); + }); +}); diff --git a/src/cdk/schematics/update-tool/component-resource-collector.ts b/src/cdk/schematics/update-tool/component-resource-collector.ts index 8509edfc0808..15ccaa7ae27b 100644 --- a/src/cdk/schematics/update-tool/component-resource-collector.ts +++ b/src/cdk/schematics/update-tool/component-resource-collector.ts @@ -148,7 +148,7 @@ export class ComponentResourceCollector { // In case the template does not exist in the file system, skip this // external template. - if (!this._fileSystem.exists(templatePath)) { + if (!this._fileSystem.fileExists(templatePath)) { return; } diff --git a/src/cdk/schematics/update-tool/file-system.ts b/src/cdk/schematics/update-tool/file-system.ts index 459487f6aca9..511ba24bf92e 100644 --- a/src/cdk/schematics/update-tool/file-system.ts +++ b/src/cdk/schematics/update-tool/file-system.ts @@ -43,8 +43,10 @@ export interface DirectoryEntry { * changes. This is necessary to support virtual file systems as used in the CLI devkit. */ export abstract class FileSystem { - /** Checks whether the given file or directory exists. */ - abstract exists(path: WorkspacePath): boolean; + /** Checks whether the given file exists. */ + abstract fileExists(path: WorkspacePath): boolean; + /** Checks whether the given directory exists. */ + abstract directoryExists(path: WorkspacePath): boolean; /** Gets the contents of the given file. */ abstract read(filePath: WorkspacePath): string|null; /** Reads the given directory to retrieve children. */ diff --git a/src/cdk/schematics/update-tool/utils/virtual-host.ts b/src/cdk/schematics/update-tool/utils/virtual-host.ts index 02598dc18114..1010057dc632 100644 --- a/src/cdk/schematics/update-tool/utils/virtual-host.ts +++ b/src/cdk/schematics/update-tool/utils/virtual-host.ts @@ -40,7 +40,7 @@ export class FileSystemHost implements ts.ParseConfigHost { constructor(private _fileSystem: FileSystem) {} fileExists(path: string): boolean { - return this._fileSystem.exists(this._fileSystem.resolve(path)); + return this._fileSystem.fileExists(this._fileSystem.resolve(path)); } readFile(path: string): string|undefined { @@ -84,7 +84,7 @@ export function createFileSystemCompilerHost( host.readFile = virtualHost.readFile.bind(virtualHost); host.readDirectory = virtualHost.readDirectory.bind(virtualHost); host.fileExists = virtualHost.fileExists.bind(virtualHost); - host.directoryExists = (dirPath) => fileSystem.exists(fileSystem.resolve(dirPath)); + host.directoryExists = (dirPath) => fileSystem.directoryExists(fileSystem.resolve(dirPath)); host.getCurrentDirectory = () => '/'; host.getCanonicalFileName = p => fileSystem.resolve(p); diff --git a/src/material/schematics/ng-update/migrations/hammer-gestures-v9/hammer-gestures-migration.ts b/src/material/schematics/ng-update/migrations/hammer-gestures-v9/hammer-gestures-migration.ts index ea11a9cf90f4..1ebbff85d435 100644 --- a/src/material/schematics/ng-update/migrations/hammer-gestures-v9/hammer-gestures-migration.ts +++ b/src/material/schematics/ng-update/migrations/hammer-gestures-v9/hammer-gestures-migration.ts @@ -483,13 +483,13 @@ export class HammerGesturesMigration extends DevkitMigration { * be stored in the specified file path. */ private _getAvailableGestureConfigFileName(sourceRoot: Path) { - if (!this.fileSystem.exists(join(sourceRoot, `${GESTURE_CONFIG_FILE_NAME}.ts`))) { + if (!this.fileSystem.fileExists(join(sourceRoot, `${GESTURE_CONFIG_FILE_NAME}.ts`))) { return `${GESTURE_CONFIG_FILE_NAME}.ts`; } let possibleName = `${GESTURE_CONFIG_FILE_NAME}-`; let index = 1; - while (this.fileSystem.exists(join(sourceRoot, `${possibleName}-${index}.ts`))) { + while (this.fileSystem.fileExists(join(sourceRoot, `${possibleName}-${index}.ts`))) { index++; } return `${possibleName + index}.ts`; @@ -630,7 +630,7 @@ export class HammerGesturesMigration extends DevkitMigration { private _removeHammerFromIndexFile() { const indexFilePaths = getProjectIndexFiles(this.context.project); indexFilePaths.forEach(filePath => { - if (!this.fileSystem.exists(filePath)) { + if (!this.fileSystem.fileExists(filePath)) { return; }