refactor(webpack-cli): don't spawn needless shells and replace execa …

…with cross-spawn (#3146) * chore(webpack-cli): replace execa with cross-spawn Remove 9 dependencies on a production install: - execa - get-stream - human-signals - is-stream - mimic-fn - npm-run-path - onetime - signal-exit - strip-final-newline * chore(webpack-cli): don't spawn a shell when installing packages Execute install command synchronously instead of spawning a shell asynchronously. - The requested action cannot proceed until the required install completes, so I don't see an advantage to an asynchronous process - Starting the subprocess directly should be more performant. It should also be more cross-plattform and secure, since it prevents interpolations and expansions. * test: fix cross-spawn mock in install tests Mock the `sync` property. Split command into command name and argument array. * test: increase smoke tests timeout to 60 seconds
webpack · Mar 8, 2022 · ce74898 · ce74898
1 parent b1ad914
commit ce74898
Show file tree

Hide file tree

Showing 5 changed files with 38 additions and 34 deletions.
diff --git a/packages/webpack-cli/package.json b/packages/webpack-cli/package.json
@@ -35,7 +35,7 @@
     "@webpack-cli/serve": "^1.6.1",
     "colorette": "^2.0.14",
     "commander": "^7.0.0",
-    "execa": "^5.0.0",
+    "cross-spawn": "^7.0.3",
     "fastest-levenshtein": "^1.0.12",
     "import-local": "^3.0.2",
     "interpret": "^2.2.0",

diff --git a/packages/webpack-cli/src/webpack-cli.ts b/packages/webpack-cli/src/webpack-cli.ts
@@ -154,7 +154,7 @@ class WebpackCLI implements IWebpackCLI {
   }
 
   getAvailablePackageManagers(): PackageManager[] {
-    const { sync } = require("execa");
+    const { sync } = require("cross-spawn");
     const installers: PackageManager[] = ["npm", "yarn", "pnpm"];
     const hasPackageManagerInstalled = (packageManager: PackageManager) => {
       try {
@@ -179,7 +179,7 @@ class WebpackCLI implements IWebpackCLI {
   }
 
   getDefaultPackageManager(): PackageManager | undefined {
-    const { sync } = require("execa");
+    const { sync } = require("cross-spawn");
     const hasLocalNpm = fs.existsSync(path.resolve(process.cwd(), "package-lock.json"));
 
     if (hasLocalNpm) {
@@ -244,13 +244,6 @@ class WebpackCLI implements IWebpackCLI {
       options.preMessage();
     }
 
-    // yarn uses 'add' command, rest npm and pnpm both use 'install'
-    const commandToBeRun = `${packageManager} ${[
-      packageManager === "yarn" ? "add" : "install",
-      "-D",
-      packageName,
-    ].join(" ")}`;
-
     const prompt = ({ message, defaultResponse, stream }: PromptOptions) => {
       const readline = require("readline");
       const rl = readline.createInterface({
@@ -275,6 +268,10 @@ class WebpackCLI implements IWebpackCLI {
       });
     };
 
+    // yarn uses 'add' command, rest npm and pnpm both use 'install'
+    const commandArguments = [packageManager === "yarn" ? "add" : "install", "-D", packageName];
+    const commandToBeRun = `${packageManager} ${commandArguments.join(" ")}`;
+
     let needInstall;
 
     try {
@@ -294,10 +291,10 @@ class WebpackCLI implements IWebpackCLI {
     }
 
     if (needInstall) {
-      const execa = require("execa");
+      const { sync } = require("cross-spawn");
 
       try {
-        await execa(commandToBeRun, [], { stdio: "inherit", shell: true });
+        sync(packageManager, commandArguments, { stdio: "inherit" });
       } catch (error) {
         this.logger.error(error);
 

diff --git a/smoketests/helpers.js b/smoketests/helpers.js
@@ -44,7 +44,7 @@ const runTest = (pkg, cliArgs = [], logMessage, isSubPackage = false) => {
     const timeout = setTimeout(() => {
       console.log("  timeout: killing process");
       proc.kill();
-    }, 30000);
+    }, 60000);
 
     const prompt = "Would you like to install";
     let hasLogMessage = false,

diff --git a/test/api/do-install.test.js b/test/api/do-install.test.js
@@ -17,9 +17,9 @@ jest.mock("readline", () => {
   };
 });
 
-const execaMock = jest.fn();
+const spawnMock = jest.fn();
 
-jest.mock("execa", () => execaMock);
+jest.mock("cross-spawn", () => ({ sync: spawnMock }));
 
 describe("doInstall", () => {
   let cli;
@@ -45,13 +45,14 @@ describe("doInstall", () => {
 
     expect(installResult).toBe("test-package");
     expect(readlineQuestionMock.mock.calls.length).toEqual(1);
-    expect(execaMock.mock.calls.length).toEqual(1);
+    expect(spawnMock.mock.calls.length).toEqual(1);
     expect(stripAnsi(readlineQuestionMock.mock.calls[0][0])).toContain(
       "Would you like to install 'test-package' package? (That will run 'npm install -D test-package')",
     );
 
     // install the package using npm
-    expect(execaMock.mock.calls[0][0]).toEqual("npm install -D test-package");
+    expect(spawnMock.mock.calls[0][0]).toEqual("npm");
+    expect(spawnMock.mock.calls[0][1]).toEqual(["install", "-D", "test-package"]);
   });
 
   it("should prompt to install using yarn if yarn is package manager", async () => {
@@ -62,13 +63,14 @@ describe("doInstall", () => {
 
     expect(installResult).toBe("test-package");
     expect(readlineQuestionMock.mock.calls.length).toEqual(1);
-    expect(execaMock.mock.calls.length).toEqual(1);
+    expect(spawnMock.mock.calls.length).toEqual(1);
     expect(stripAnsi(readlineQuestionMock.mock.calls[0][0])).toContain(
       "Would you like to install 'test-package' package? (That will run 'yarn add -D test-package')",
     );
 
     // install the package using yarn
-    expect(execaMock.mock.calls[0][0]).toEqual("yarn add -D test-package");
+    expect(spawnMock.mock.calls[0][0]).toEqual("yarn");
+    expect(spawnMock.mock.calls[0][1]).toEqual(["add", "-D", "test-package"]);
   });
 
   it("should prompt to install using pnpm if pnpm is package manager", async () => {
@@ -79,13 +81,14 @@ describe("doInstall", () => {
 
     expect(installResult).toBe("test-package");
     expect(readlineQuestionMock.mock.calls.length).toEqual(1);
-    expect(execaMock.mock.calls.length).toEqual(1);
+    expect(spawnMock.mock.calls.length).toEqual(1);
     expect(stripAnsi(readlineQuestionMock.mock.calls[0][0])).toContain(
       "Would you like to install 'test-package' package? (That will run 'pnpm install -D test-package')",
     );
 
     // install the package using npm
-    expect(execaMock.mock.calls[0][0]).toEqual("pnpm install -D test-package");
+    expect(spawnMock.mock.calls[0][0]).toEqual("pnpm");
+    expect(spawnMock.mock.calls[0][1]).toEqual(["install", "-D", "test-package"]);
   });
 
   it("should support pre message", async () => {
@@ -98,13 +101,14 @@ describe("doInstall", () => {
     expect(installResult).toBe("test-package");
     expect(preMessage.mock.calls.length).toEqual(1);
     expect(readlineQuestionMock.mock.calls.length).toEqual(1);
-    expect(execaMock.mock.calls.length).toEqual(1);
+    expect(spawnMock.mock.calls.length).toEqual(1);
     expect(stripAnsi(readlineQuestionMock.mock.calls[0][0])).toContain(
       "Would you like to install 'test-package' package? (That will run 'npm install -D test-package')",
     );
 
     // install the package using npm
-    expect(execaMock.mock.calls[0][0]).toEqual("npm install -D test-package");
+    expect(spawnMock.mock.calls[0][0]).toEqual("npm");
+    expect(spawnMock.mock.calls[0][1]).toEqual(["install", "-D", "test-package"]);
   });
 
   it("should prompt to install and install using 'y'", async () => {
@@ -115,13 +119,14 @@ describe("doInstall", () => {
 
     expect(installResult).toBe("test-package");
     expect(readlineQuestionMock.mock.calls.length).toEqual(1);
-    expect(execaMock.mock.calls.length).toEqual(1);
+    expect(spawnMock.mock.calls.length).toEqual(1);
     expect(stripAnsi(readlineQuestionMock.mock.calls[0][0])).toContain(
       "Would you like to install 'test-package' package? (That will run 'npm install -D test-package')",
     );
 
     // install the package using npm
-    expect(execaMock.mock.calls[0][0]).toEqual("npm install -D test-package");
+    expect(spawnMock.mock.calls[0][0]).toEqual("npm");
+    expect(spawnMock.mock.calls[0][1]).toEqual(["install", "-D", "test-package"]);
   });
 
   it("should prompt to install and install using 'yes'", async () => {
@@ -132,13 +137,14 @@ describe("doInstall", () => {
 
     expect(installResult).toBe("test-package");
     expect(readlineQuestionMock.mock.calls.length).toEqual(1);
-    expect(execaMock.mock.calls.length).toEqual(1);
+    expect(spawnMock.mock.calls.length).toEqual(1);
     expect(stripAnsi(readlineQuestionMock.mock.calls[0][0])).toContain(
       "Would you like to install 'test-package' package? (That will run 'npm install -D test-package')",
     );
 
     // install the package using npm
-    expect(execaMock.mock.calls[0][0]).toEqual("npm install -D test-package");
+    expect(spawnMock.mock.calls[0][0]).toEqual("npm");
+    expect(spawnMock.mock.calls[0][1]).toEqual(["install", "-D", "test-package"]);
   });
 
   it("should prompt to install and install using 'yEs'", async () => {
@@ -149,13 +155,14 @@ describe("doInstall", () => {
 
     expect(installResult).toBe("test-package");
     expect(readlineQuestionMock.mock.calls.length).toEqual(1);
-    expect(execaMock.mock.calls.length).toEqual(1);
+    expect(spawnMock.mock.calls.length).toEqual(1);
     expect(stripAnsi(readlineQuestionMock.mock.calls[0][0])).toContain(
       "Would you like to install 'test-package' package? (That will run 'npm install -D test-package')",
     );
 
     // install the package using npm
-    expect(execaMock.mock.calls[0][0]).toEqual("npm install -D test-package");
+    expect(spawnMock.mock.calls[0][0]).toEqual("npm");
+    expect(spawnMock.mock.calls[0][1]).toEqual(["install", "-D", "test-package"]);
   });
 
   it("should not install if install is not confirmed", async () => {
@@ -167,7 +174,7 @@ describe("doInstall", () => {
     expect(installResult).toBeUndefined();
     expect(readlineQuestionMock.mock.calls.length).toEqual(1);
     // runCommand should not be called, because the installation is not confirmed
-    expect(execaMock.mock.calls.length).toEqual(0);
+    expect(spawnMock.mock.calls.length).toEqual(0);
     expect(mockExit.mock.calls[0][0]).toEqual(2);
 
     mockExit.mockRestore();
@@ -182,7 +189,7 @@ describe("doInstall", () => {
     expect(installResult).toBeUndefined();
     expect(readlineQuestionMock.mock.calls.length).toEqual(1);
     // runCommand should not be called, because the installation is not confirmed
-    expect(execaMock.mock.calls.length).toEqual(0);
+    expect(spawnMock.mock.calls.length).toEqual(0);
     expect(mockExit.mock.calls[0][0]).toEqual(2);
 
     mockExit.mockRestore();
@@ -197,7 +204,7 @@ describe("doInstall", () => {
     expect(installResult).toBeUndefined();
     expect(readlineQuestionMock.mock.calls.length).toEqual(1);
     // runCommand should not be called, because the installation is not confirmed
-    expect(execaMock.mock.calls.length).toEqual(0);
+    expect(spawnMock.mock.calls.length).toEqual(0);
     expect(mockExit.mock.calls[0][0]).toEqual(2);
 
     mockExit.mockRestore();
@@ -212,7 +219,7 @@ describe("doInstall", () => {
     expect(installResult).toBeUndefined();
     expect(readlineQuestionMock.mock.calls.length).toEqual(1);
     // runCommand should not be called, because the installation is not confirmed
-    expect(execaMock.mock.calls.length).toEqual(0);
+    expect(spawnMock.mock.calls.length).toEqual(0);
     expect(mockExit.mock.calls[0][0]).toEqual(2);
 
     mockExit.mockRestore();

diff --git a/test/api/get-default-package-manager.test.js b/test/api/get-default-package-manager.test.js
@@ -8,7 +8,7 @@ const syncMock = jest.fn(() => {
     stdout: "1.0.0",
   };
 });
-jest.setMock("execa", {
+jest.setMock("cross-spawn", {
   sync: syncMock,
 });