Skip to content

Commit 9c30d95

Browse files
committed
fix(cdk/schematics): avoid runtime errors thrown by devkit tree
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.
1 parent 3284496 commit 9c30d95

File tree

6 files changed

+54
-32
lines changed

6 files changed

+54
-32
lines changed

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

Lines changed: 20 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import {basename, dirname, normalize, NormalizedRoot, Path} from '@angular-devkit/core';
9+
import {normalize, Path} from '@angular-devkit/core';
1010
import {Tree, UpdateRecorder} from '@angular-devkit/schematics';
1111
import {DirectoryEntry, FileSystem} from '../update-tool/file-system';
1212
import * as path from 'path';
@@ -42,10 +42,25 @@ export class DevkitFileSystem extends FileSystem {
4242
this._updateRecorderCache.clear();
4343
}
4444

45-
exists(fileOrDirPath: Path) {
46-
// We need to check for both file or directory existence, in order
47-
// to comply with the expectation from the TypeScript compiler.
48-
return this._tree.exists(fileOrDirPath) || this._isExistingDirectory(fileOrDirPath);
45+
existsFile(filePath: Path) {
46+
return this._tree.exists(filePath);
47+
}
48+
49+
existsDirectory(dirPath: Path) {
50+
// The devkit tree does not expose an API for checking whether a given
51+
// directory exists. It throws a specific error though if a directory
52+
// is being read as a file. We use that to check if a directory exists.
53+
try {
54+
this._tree.get(dirPath);
55+
} catch (e) {
56+
// Note: We do not use an `instanceof` check here. It could happen that the devkit version
57+
// used by the CLI is different than the one we end up loading. This can happen depending
58+
// on how Yarn/NPM hoists the NPM packages / whether there are multiple versions installed.
59+
if (e instanceof Error && e.constructor.name === 'PathIsDirectoryException') {
60+
return true;
61+
}
62+
}
63+
return false;
4964
}
5065

5166
overwrite(filePath: Path, content: string) {
@@ -69,23 +84,4 @@ export class DevkitFileSystem extends FileSystem {
6984
const {subdirs: directories, subfiles: files} = this._tree.getDir(dirPath);
7085
return {directories, files};
7186
}
72-
73-
private _isExistingDirectory(dirPath: Path) {
74-
if (dirPath === NormalizedRoot) {
75-
return true;
76-
}
77-
78-
const parent = dirname(dirPath);
79-
const dirName = basename(dirPath);
80-
// TypeScript also checks potential entry points, so e.g. importing
81-
// package.json will result in a lookup of /package.json/package.json
82-
// and /package.json/index.ts. In order to avoid failure, we check if
83-
// the parent is an existing file and return false, if that is the case.
84-
if (this._tree.exists(parent)) {
85-
return false;
86-
}
87-
88-
const dir = this._tree.getDir(parent);
89-
return dir.subdirs.indexOf(dirName) !== -1;
90-
}
9187
}
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
import {MIGRATION_PATH} from '../../../paths';
2+
import {createTestCaseSetup} from '../../../testing';
3+
4+
describe('ng update typescript program module resolution', () => {
5+
6+
// Regression test for: https://github.com/angular/components/issues/22919.
7+
it('should not error if module resolution tries a non-existent path where a path segment ' +
8+
'matches an existing file', async () => {
9+
const {runFixers, writeFile} = await createTestCaseSetup(
10+
'migration-v6', MIGRATION_PATH, []);
11+
12+
writeFile('/node_modules/some-other-module/package.json', `{}`);
13+
writeFile('/node_modules/some-other-module/styles.css', '')
14+
15+
// We add an import to a non-existent sub-path of `some-other-module/styles`. The TypeScript
16+
// module resolution logic could try various sub-paths. This previously resulted in an error
17+
// as the devkit tree `getDir` logic accidentally walked up the path and threw an error if
18+
// a path segment is an actual file.
19+
writeFile('/projects/cdk-testing/src/main.ts',
20+
`import 'some-other-module/styles.css/non/existent';`);
21+
22+
await expectAsync(runFixers()).toBeResolved();
23+
});
24+
});

src/cdk/schematics/update-tool/component-resource-collector.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ export class ComponentResourceCollector {
148148

149149
// In case the template does not exist in the file system, skip this
150150
// external template.
151-
if (!this._fileSystem.exists(templatePath)) {
151+
if (!this._fileSystem.existsFile(templatePath)) {
152152
return;
153153
}
154154

src/cdk/schematics/update-tool/file-system.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,10 @@ export interface DirectoryEntry {
4343
* changes. This is necessary to support virtual file systems as used in the CLI devkit.
4444
*/
4545
export abstract class FileSystem {
46-
/** Checks whether the given file or directory exists. */
47-
abstract exists(path: WorkspacePath): boolean;
46+
/** Checks whether the given file exists. */
47+
abstract existsFile(path: WorkspacePath): boolean;
48+
/** Checks whether the given directory exists. */
49+
abstract existsDirectory(path: WorkspacePath): boolean;
4850
/** Gets the contents of the given file. */
4951
abstract read(filePath: WorkspacePath): string|null;
5052
/** Reads the given directory to retrieve children. */

src/cdk/schematics/update-tool/utils/virtual-host.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ export class FileSystemHost implements ts.ParseConfigHost {
4040
constructor(private _fileSystem: FileSystem) {}
4141

4242
fileExists(path: string): boolean {
43-
return this._fileSystem.exists(this._fileSystem.resolve(path));
43+
return this._fileSystem.existsFile(this._fileSystem.resolve(path));
4444
}
4545

4646
readFile(path: string): string|undefined {
@@ -84,7 +84,7 @@ export function createFileSystemCompilerHost(
8484
host.readFile = virtualHost.readFile.bind(virtualHost);
8585
host.readDirectory = virtualHost.readDirectory.bind(virtualHost);
8686
host.fileExists = virtualHost.fileExists.bind(virtualHost);
87-
host.directoryExists = (dirPath) => fileSystem.exists(fileSystem.resolve(dirPath));
87+
host.directoryExists = (dirPath) => fileSystem.existsDirectory(fileSystem.resolve(dirPath));
8888
host.getCurrentDirectory = () => '/';
8989
host.getCanonicalFileName = p => fileSystem.resolve(p);
9090

src/material/schematics/ng-update/migrations/hammer-gestures-v9/hammer-gestures-migration.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -483,13 +483,13 @@ export class HammerGesturesMigration extends DevkitMigration<null> {
483483
* be stored in the specified file path.
484484
*/
485485
private _getAvailableGestureConfigFileName(sourceRoot: Path) {
486-
if (!this.fileSystem.exists(join(sourceRoot, `${GESTURE_CONFIG_FILE_NAME}.ts`))) {
486+
if (!this.fileSystem.existsFile(join(sourceRoot, `${GESTURE_CONFIG_FILE_NAME}.ts`))) {
487487
return `${GESTURE_CONFIG_FILE_NAME}.ts`;
488488
}
489489

490490
let possibleName = `${GESTURE_CONFIG_FILE_NAME}-`;
491491
let index = 1;
492-
while (this.fileSystem.exists(join(sourceRoot, `${possibleName}-${index}.ts`))) {
492+
while (this.fileSystem.existsFile(join(sourceRoot, `${possibleName}-${index}.ts`))) {
493493
index++;
494494
}
495495
return `${possibleName + index}.ts`;
@@ -630,7 +630,7 @@ export class HammerGesturesMigration extends DevkitMigration<null> {
630630
private _removeHammerFromIndexFile() {
631631
const indexFilePaths = getProjectIndexFiles(this.context.project);
632632
indexFilePaths.forEach(filePath => {
633-
if (!this.fileSystem.exists(filePath)) {
633+
if (!this.fileSystem.existsFile(filePath)) {
634634
return;
635635
}
636636

0 commit comments

Comments
 (0)