refactor(compiler-cli): remove unused method `FileSystem#mkdir()` (#33237)

Previously, the `FileSystem` abstraction featured a `mkdir()` method. In
`NodeJSFileSystem` (the default `FileSystem` implementation used in
actual code), the method behaved similar to Node.js' `fs.mkdirSync()`
(i.e. failing if any parent directory is missing or the directory exists
already). In contrast, `MockFileSystem` (which is the basis or mock
`FileSystem` implementations used in tests) implemented `mkdir()` as an
alias to `ensureDir()`, which behaved more like Node.js'
`fs.mkdirSync()` with the `recursive` option set to `true` (i.e.
creating any missing parent directories and succeeding if the directory
exists already).

This commit fixes this inconsistency by removing the `mkdir()` method,
which was not used anyway and only keeping `ensureDir()` (which is
consistent across our different `FileSystem` implementations).

PR Close #33237
This commit is contained in:
George Kalpakas 2019-10-18 16:15:57 +03:00 committed by Matias Niemelä
parent 8017229292
commit d7dc6cbc04
7 changed files with 15 additions and 51 deletions

View File

@ -74,11 +74,6 @@ export class CachedFileSystem implements FileSystem {
this.existsCache.set(to, true); this.existsCache.set(to, true);
} }
mkdir(path: AbsoluteFsPath): void {
this.delegate.mkdir(path);
this.existsCache.set(path, true);
}
ensureDir(path: AbsoluteFsPath): void { ensureDir(path: AbsoluteFsPath): void {
this.delegate.ensureDir(path); this.delegate.ensureDir(path);
while (!this.isRoot(path)) { while (!this.isRoot(path)) {

View File

@ -27,7 +27,6 @@ export class InvalidFileSystem implements FileSystem {
extname(path: AbsoluteFsPath|PathSegment): string { throw makeError(); } extname(path: AbsoluteFsPath|PathSegment): string { throw makeError(); }
copyFile(from: AbsoluteFsPath, to: AbsoluteFsPath): void { throw makeError(); } copyFile(from: AbsoluteFsPath, to: AbsoluteFsPath): void { throw makeError(); }
moveFile(from: AbsoluteFsPath, to: AbsoluteFsPath): void { throw makeError(); } moveFile(from: AbsoluteFsPath, to: AbsoluteFsPath): void { throw makeError(); }
mkdir(path: AbsoluteFsPath): void { throw makeError(); }
ensureDir(path: AbsoluteFsPath): void { throw makeError(); } ensureDir(path: AbsoluteFsPath): void { throw makeError(); }
isCaseSensitive(): boolean { throw makeError(); } isCaseSensitive(): boolean { throw makeError(); }
resolve(...paths: string[]): AbsoluteFsPath { throw makeError(); } resolve(...paths: string[]): AbsoluteFsPath { throw makeError(); }

View File

@ -28,7 +28,6 @@ export class NodeJSFileSystem implements FileSystem {
pwd(): AbsoluteFsPath { return this.normalize(process.cwd()) as AbsoluteFsPath; } pwd(): AbsoluteFsPath { return this.normalize(process.cwd()) as AbsoluteFsPath; }
copyFile(from: AbsoluteFsPath, to: AbsoluteFsPath): void { fs.copyFileSync(from, to); } copyFile(from: AbsoluteFsPath, to: AbsoluteFsPath): void { fs.copyFileSync(from, to); }
moveFile(from: AbsoluteFsPath, to: AbsoluteFsPath): void { fs.renameSync(from, to); } moveFile(from: AbsoluteFsPath, to: AbsoluteFsPath): void { fs.renameSync(from, to); }
mkdir(path: AbsoluteFsPath): void { fs.mkdirSync(path); }
ensureDir(path: AbsoluteFsPath): void { ensureDir(path: AbsoluteFsPath): void {
const parents: AbsoluteFsPath[] = []; const parents: AbsoluteFsPath[] = [];
while (!this.isRoot(path) && !this.exists(path)) { while (!this.isRoot(path) && !this.exists(path)) {
@ -73,7 +72,7 @@ export class NodeJSFileSystem implements FileSystem {
private safeMkdir(path: AbsoluteFsPath): void { private safeMkdir(path: AbsoluteFsPath): void {
try { try {
this.mkdir(path); fs.mkdirSync(path);
} catch (err) { } catch (err) {
// Ignore the error, if the path already exists and points to a directory. // Ignore the error, if the path already exists and points to a directory.
// Re-throw otherwise. // Re-throw otherwise.

View File

@ -46,7 +46,6 @@ export interface FileSystem {
extname(path: AbsoluteFsPath|PathSegment): string; extname(path: AbsoluteFsPath|PathSegment): string;
copyFile(from: AbsoluteFsPath, to: AbsoluteFsPath): void; copyFile(from: AbsoluteFsPath, to: AbsoluteFsPath): void;
moveFile(from: AbsoluteFsPath, to: AbsoluteFsPath): void; moveFile(from: AbsoluteFsPath, to: AbsoluteFsPath): void;
mkdir(path: AbsoluteFsPath): void;
ensureDir(path: AbsoluteFsPath): void; ensureDir(path: AbsoluteFsPath): void;
isCaseSensitive(): boolean; isCaseSensitive(): boolean;
isRoot(path: AbsoluteFsPath): boolean; isRoot(path: AbsoluteFsPath): boolean;

View File

@ -234,22 +234,6 @@ describe('CachedFileSystem', () => {
}); });
}); });
describe('mkdir()', () => {
it('should call delegate', () => {
const spy = spyOn(delegate, 'mkdir');
fs.mkdir(xyzPath);
expect(spy).toHaveBeenCalledWith(xyzPath);
});
it('should update the "exists" cache', () => {
spyOn(delegate, 'mkdir');
const existsSpy = spyOn(delegate, 'exists');
fs.mkdir(xyzPath);
expect(fs.exists(xyzPath)).toEqual(true);
expect(existsSpy).not.toHaveBeenCalled();
});
});
describe('ensureDir()', () => { describe('ensureDir()', () => {
it('should call delegate', () => { it('should call delegate', () => {
const ensureDirSpy = spyOn(delegate, 'ensureDir'); const ensureDirSpy = spyOn(delegate, 'ensureDir');

View File

@ -105,14 +105,6 @@ describe('NodeJSFileSystem', () => {
}); });
}); });
describe('mkdir()', () => {
it('should delegate to fs.mkdirSync()', () => {
const spy = spyOn(realFs, 'mkdirSync');
fs.mkdir(xyzPath);
expect(spy).toHaveBeenCalledWith(xyzPath);
});
});
describe('ensureDir()', () => { describe('ensureDir()', () => {
it('should call exists() and fs.mkdir()', () => { it('should call exists() and fs.mkdir()', () => {
const aPath = absoluteFrom('/a'); const aPath = absoluteFrom('/a');

View File

@ -96,10 +96,21 @@ export abstract class MockFileSystem implements FileSystem {
delete folder[name]; delete folder[name];
} }
mkdir(path: AbsoluteFsPath): void { this.ensureFolders(this._fileTree, this.splitPath(path)); }
ensureDir(path: AbsoluteFsPath): void { ensureDir(path: AbsoluteFsPath): void {
this.ensureFolders(this._fileTree, this.splitPath(path)); const segments = this.splitPath(path);
let current: Folder = this._fileTree;
// Convert the root folder to a canonical empty string `''` (on Windows it would be `'C:'`).
segments[0] = '';
for (const segment of segments) {
if (isFile(current[segment])) {
throw new Error(`Folder already exists as a file.`);
}
if (!current[segment]) {
current[segment] = {};
}
current = current[segment] as Folder;
}
} }
isRoot(path: AbsoluteFsPath): boolean { return this.dirname(path) === path; } isRoot(path: AbsoluteFsPath): boolean { return this.dirname(path) === path; }
@ -173,21 +184,6 @@ export abstract class MockFileSystem implements FileSystem {
const file = segments.pop() !; const file = segments.pop() !;
return [path.substring(0, path.length - file.length - 1) as AbsoluteFsPath, file]; return [path.substring(0, path.length - file.length - 1) as AbsoluteFsPath, file];
} }
protected ensureFolders(current: Folder, segments: string[]): Folder {
// Convert the root folder to a canonical empty string `""` (on Windows it would be `C:`).
segments[0] = '';
for (const segment of segments) {
if (isFile(current[segment])) {
throw new Error(`Folder already exists as a file.`);
}
if (!current[segment]) {
current[segment] = {};
}
current = current[segment] as Folder;
}
return current;
}
} }
export interface FindResult { export interface FindResult {
path: AbsoluteFsPath; path: AbsoluteFsPath;