Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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
#22919.

Fixes #22919.
  • Loading branch information
devversion committed Jun 16, 2021
1 parent 3c140fd commit 805d3ae
Show file tree
Hide file tree
Showing 6 changed files with 54 additions and 32 deletions.
44 changes: 20 additions & 24 deletions src/cdk/schematics/ng-update/devkit-file-system.ts
Expand Up @@ -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';
Expand Down Expand Up @@ -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) {
Expand All @@ -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;
}
}
@@ -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();
});
});
Expand Up @@ -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;
}

Expand Down
6 changes: 4 additions & 2 deletions src/cdk/schematics/update-tool/file-system.ts
Expand Up @@ -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. */
Expand Down
4 changes: 2 additions & 2 deletions src/cdk/schematics/update-tool/utils/virtual-host.ts
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);

Expand Down
Expand Up @@ -483,13 +483,13 @@ export class HammerGesturesMigration extends DevkitMigration<null> {
* 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`;
Expand Down Expand Up @@ -630,7 +630,7 @@ export class HammerGesturesMigration extends DevkitMigration<null> {
private _removeHammerFromIndexFile() {
const indexFilePaths = getProjectIndexFiles(this.context.project);
indexFilePaths.forEach(filePath => {
if (!this.fileSystem.exists(filePath)) {
if (!this.fileSystem.fileExists(filePath)) {
return;
}

Expand Down

0 comments on commit 805d3ae

Please sign in to comment.