Skip to content

Commit

Permalink
refactor(fs): Replace custom directory delete implementation with nod…
Browse files Browse the repository at this point in the history
…e fs method

With ff8808a we switched the cli to
node.js 14.14.0 or higher. With this version the `recursive` option was
added to the rm method in the node.js fs module.
Now we can use this integrated method as we don't need to support
the older node.js versions anymore.
  • Loading branch information
hlxid committed Oct 5, 2022
1 parent ff8808a commit 8046fe9
Show file tree
Hide file tree
Showing 10 changed files with 31 additions and 72 deletions.
6 changes: 3 additions & 3 deletions src/install/development.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as chalk from "chalk";
import * as git from "isomorphic-git";
import * as fs from "fs";
import * as http from "isomorphic-git/http/node";
import { directoryExists, removeDirectory } from "../utils/fs";
import { directoryExists } from "../utils/fs";
import { DevelopmentInstallation, writeInstallInfo } from "../utils/installation";
import { logger } from "../utils/log";
import * as path from "path";
Expand Down Expand Up @@ -43,7 +43,7 @@ async function manageDocs(nodecgIODir: string, cloneDocs: boolean): Promise<void
} else if (await directoryExists(docsPath)) {
// Docs are not wanted but exists (they probably were selected previously) => delete
logger.debug("Removing docs...");
await removeDirectory(docsPath);
await fs.promises.rm(docsPath, { recursive: true, force: true });
}
}

Expand Down Expand Up @@ -122,7 +122,7 @@ async function deleteNodeModuleDirectories(nodecgIODir: string): Promise<void> {

for (const nodeModuleDir of nodeModuleDirs) {
if (await directoryExists(nodeModuleDir)) {
await removeDirectory(nodeModuleDir);
await fs.promises.rm(nodeModuleDir, { recursive: true, force: true });
}
}

Expand Down
5 changes: 3 additions & 2 deletions src/install/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { CommandModule } from "yargs";
import * as path from "path";
import { directoryExists, removeDirectory } from "../utils/fs";
import * as fs from "fs";
import { directoryExists } from "../utils/fs";
import { createDevInstall } from "./development";
import { manageBundleDir } from "../utils/nodecgConfig";
import { promptForInstallInfo } from "./prompt";
Expand Down Expand Up @@ -76,7 +77,7 @@ async function install(opts: InstallCommandOptions): Promise<void> {
// If the minor version changed and we already have another one installed, we need to delete it, so it can be properly installed.
if (currentInstall && currentInstall.version !== requestedInstall.version && (await directoryExists(nodecgIODir))) {
logger.info(`Deleting nodecg-io version ${currentInstall.version}...`);
await removeDirectory(nodecgIODir);
await fs.promises.rm(nodecgIODir, { recursive: true, force: true });
currentInstall = undefined;
}

Expand Down
7 changes: 4 additions & 3 deletions src/uninstall/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import path = require("path");
import * as path from "path";
import * as fs from "fs";
import { CommandModule } from "yargs";
import { directoryExists, removeDirectory } from "../utils/fs";
import { directoryExists } from "../utils/fs";
import { logger } from "../utils/log";
import { manageBundleDir } from "../utils/nodecgConfig";
import { findNodeCGDirectory, getNodeCGIODirectory } from "../utils/nodecgInstallation";
Expand Down Expand Up @@ -36,7 +37,7 @@ export async function uninstall(): Promise<void> {

// Delete directory
logger.debug(`Uninstalling nodecg-io from nodecg installation at ${nodecgDir}...`);
await removeDirectory(nodecgIODir);
await fs.promises.rm(nodecgIODir, { recursive: true, force: true });

logger.success("Successfully uninstalled nodecg-io.");
}
30 changes: 0 additions & 30 deletions src/utils/fs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,36 +29,6 @@ export async function ensureDirectory(dir: string): Promise<void> {
}
}

/**
* Deletes a directory at the specified path in the filesystem.
* @param dirPath the directory which should be deleted.
*/
export async function removeDirectory(dirPath: string): Promise<void> {
// Delete all contents of this directory because otherwise we cannot remove it (why can't that be part of fs, oh node 14+ only...)
const contents = await fs.readdir(dirPath); // get entries of directory

const rmPromises = contents.reverse().map(async (f) => {
const subpath = path.join(dirPath, f);

try {
const stat = await fs.lstat(subpath);
// rm if file or symlink and use this function recursively if directory
if (stat.isDirectory() && !stat.isSymbolicLink()) {
await removeDirectory(subpath);
} else {
await fs.unlink(subpath);
}
} catch (_e) {
// ignore that file cannot be removed. Maybe was already removed.
}
});

await Promise.all(rmPromises);

// now that the directory is empty we can delete it.
await fs.rmdir(dirPath);
}

/**
* Executes the given command and optionally streams the output to the console.
* @param command the command that should be executed.
Expand Down
4 changes: 2 additions & 2 deletions src/utils/npm.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import axios, { AxiosRequestConfig, AxiosResponse } from "axios";
import * as fs from "fs";
import * as path from "path";
import { executeCommand, removeDirectory } from "./fs";
import { executeCommand } from "./fs";
import { exec } from "child_process";
import { maxSatisfying, satisfies, SemVer } from "semver";
import * as zlib from "zlib";
Expand Down Expand Up @@ -223,7 +223,7 @@ export async function createNpmSymlinks(packages: NpmPackage[], nodecgIODir: str
* @param nodecgIODir the directory in which nodecg-io is installed
*/
export async function removeNpmPackage(pkg: NpmPackage, nodecgIODir: string): Promise<void> {
await removeDirectory(buildNpmPackagePath(pkg, nodecgIODir));
await fs.promises.rm(buildNpmPackagePath(pkg, nodecgIODir), { recursive: true, force: true });
}

/**
Expand Down
4 changes: 2 additions & 2 deletions test/install/development.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { vol } from "memfs";
import * as git from "isomorphic-git";
import * as fs from "fs";
import * as fsUtils from "../../src/utils/fs";
import { fsRoot, validDevInstall, nodecgIODir } from "../test.util";
import * as dev from "../../src/install/development";
import { removeDirectory } from "../../src/utils/fs";

const defaultFetchResult: git.FetchResult = {
defaultBranch: "main",
Expand Down Expand Up @@ -69,7 +69,7 @@ describe("getGitRepo", () => {

test("should clone repo if directory does not exists", async () => {
// remove dir so it should clone
await removeDirectory(nodecgIODir);
await fs.promises.rm(nodecgIODir, { recursive: true, force: true });

await dev.getGitRepo(nodecgIODir, "nodecg-io");
expect(cloneSpy).toHaveBeenCalled();
Expand Down
10 changes: 6 additions & 4 deletions test/install/production.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { vol } from "memfs";
import * as path from "path";
import * as fs from "fs";
import { corePkg, dashboardPkg, nodecgIODir, twitchChatPkg, validProdInstall } from "../test.util";
import { diffPackages, installPackages, removePackages, validateInstall } from "../../src/install/production";
import * as installation from "../../src/utils/installation";
Expand Down Expand Up @@ -56,13 +57,14 @@ describe("diffPackages", () => {

describe("removePackages", () => {
test("should rm each package directory", async () => {
const rmMock = jest.spyOn(fsUtils, "removeDirectory").mockClear().mockResolvedValue();
const rmMock = jest.spyOn(fs.promises, "rm").mockClear().mockResolvedValue();
const i = { ...validProdInstall, packages: [...packages] };
await removePackages(packages, i, nodecgIODir);

expect(rmMock).toHaveBeenCalledTimes(2);
expect(rmMock).toHaveBeenCalledWith(path.join(nodecgIODir, corePkg.path));
expect(rmMock).toHaveBeenLastCalledWith(path.join(nodecgIODir, twitchChatPkg.path));
const rmOpts = { recursive: true, force: true };
expect(rmMock).toHaveBeenCalledWith(path.join(nodecgIODir, corePkg.path), rmOpts);
expect(rmMock).toHaveBeenLastCalledWith(path.join(nodecgIODir, twitchChatPkg.path), rmOpts);
expect(i.packages.length).toBe(0);
});

Expand Down Expand Up @@ -106,7 +108,7 @@ describe("installPackages", () => {

test("should revert changes if npm install fails", async () => {
npmInstallMock.mockRejectedValue(new Error("random error"));
const rmMock = jest.spyOn(fsUtils, "removeDirectory").mockClear().mockResolvedValue();
const rmMock = jest.spyOn(fs.promises, "rm").mockClear().mockResolvedValue();

// should return the error
await expect(installPackages(packages, createInstall(), nodecgIODir)).rejects.toThrow("random error");
Expand Down
10 changes: 5 additions & 5 deletions test/uninstall/index.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { vol } from "memfs";
import { uninstall } from "../../src/uninstall";
import * as fsUtils from "../../src/utils/fs";
import * as nodecgInstall from "../../src/utils/nodecgInstallation";
import * as nodecgConfig from "../../src/utils/nodecgConfig";
import { fsRoot } from "../test.util";
import * as path from "path";
import * as fs from "fs";

jest.mock("fs", () => vol);
afterEach(() => vol.reset());
Expand All @@ -13,18 +13,18 @@ const nodecgIODir = path.join(fsRoot, "nodecg-io");

jest.spyOn(nodecgInstall, "findNodeCGDirectory").mockResolvedValue(fsRoot);
const spyManageBundleDir = jest.spyOn(nodecgConfig, "manageBundleDir");
const spyRemoveDirectory = jest.spyOn(fsUtils, "removeDirectory");
const spyRm = jest.spyOn(fs.promises, "rm");

afterEach(() => {
spyManageBundleDir.mockClear();
spyRemoveDirectory.mockClear();
spyRm.mockClear();
});

describe("uninstall", () => {
test("should not do anything if there is no nodecg-io directory", async () => {
await uninstall();

expect(spyRemoveDirectory).not.toHaveBeenCalled();
expect(spyRm).not.toHaveBeenCalled();
expect(spyManageBundleDir).not.toHaveBeenCalled();
});

Expand All @@ -48,6 +48,6 @@ describe("uninstall", () => {
await vol.promises.mkdir(nodecgIODir);
await uninstall();

expect(spyRemoveDirectory).toHaveBeenCalledWith(nodecgIODir);
expect(spyRm).toHaveBeenCalledWith(nodecgIODir, { recursive: true, force: true });
});
});
20 changes: 1 addition & 19 deletions test/utils/fs.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { vol } from "memfs";
import { directoryExists, ensureDirectory, executeCommand, removeDirectory } from "../../src/utils/fs";
import { directoryExists, ensureDirectory, executeCommand } from "../../src/utils/fs";
import * as path from "path";
import * as child_process from "child_process";
import { testDir } from "../test.util";
Expand Down Expand Up @@ -40,24 +40,6 @@ describe("ensureDirectory", () => {
});
});

describe("removeDirectory", () => {
test("should remove directory recursively", async () => {
await vol.promises.mkdir(testDir);
await vol.promises.writeFile(path.join(testDir, "test.txt"), "abc");
await vol.promises.mkdir(path.join(testDir, "sub-dir"));

await removeDirectory(testDir);

// Directory should not be there anymore.
await expect(vol.promises.stat(testDir)).rejects.toThrow("no such file or directory");
});

test("should fail if directory does not exist", async () => {
// should fail because the directory does not exist
await expect(removeDirectory(testDir)).rejects.toThrow("no such file or directory");
});
});

describe("executeCommand", () => {
test("should not error if the command successfully exits (code 0)", async () => {
await executeCommand("exit", ["0"]);
Expand Down
7 changes: 5 additions & 2 deletions test/utils/npm/pkgManagement.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
import { tempDir, corePkg, fsRoot, twitchChatPkg, dashboardPkg } from "../../test.util";
import * as fsUtils from "../../../src/utils/fs";
import * as path from "path";
import * as fs from "fs";

jest.mock("fs", () => createFsFromVolume(vol));
afterEach(() => vol.reset());
Expand Down Expand Up @@ -69,10 +70,12 @@ describe("createNpmSymlinks", () => {
});

describe("removeNpmPackage", () => {
test("should call to fsUtils.removeDirectory", async () => {
const spy = jest.spyOn(fsUtils, "removeDirectory").mockResolvedValue();
test("should call to fs.promises.rm to delete directory", async () => {
const spy = jest.spyOn(fs.promises, "rm").mockResolvedValue();
await removeNpmPackage(corePkg, await tempDir());
expect(spy).toHaveBeenCalled();
// Ensure that recursive deletion is enabled
expect(spy.mock.calls[0]?.[1]?.recursive).toBe(true);
});
});

Expand Down

0 comments on commit 8046fe9

Please sign in to comment.