refactor(webpack-cli): don't spawn needless shells and replace execa with cross-spawn #3146

ludofischer · 2022-02-25T21:15:41Z

What kind of change does this PR introduce?
Refactor: use a simple child process instead of a shell and replace execa with cross-spawn.

Did you add tests for your changes?
No, I only replaced the execa mocks with cross-spawn mocks, as the functionality should be identical as before.
I've tested webpack init on my local machine.

Summary
Executing a shell is less performant, less secure and less cross-platform, according to both the execa and cross-spawn documentation.

Replacing execa with cross-spawn removes 9 dependencies on a production install:

execa
get-stream
human-signals
is-stream
mimic-fn
npm-run-path
onetime
signal-exit
strip-final-newline

I can't find any options unique to execa in the codebase and cross-spawn should take care of the Windows compatibility issues.

Does this PR introduce a breaking change?
No.

Other information
execa uses cross-spawn under the hood to parse the command arguments, so nothing should change in that regard: https://github.com/sindresorhus/execa/blob/main/index.js

ludofischer · 2022-02-25T21:18:50Z

packages/webpack-cli/src/webpack-cli.ts


      try {
-        await execa(commandToBeRun, [], { stdio: "inherit", shell: true });
+        sync(packageManager, commandArguments, { stdio: "inherit" });


Can't understand why this is the only spot where sync was not used and shell was set to true. Could be that it was a way to make passing all the arguments in a single string work?

Good questions, maybe it was just mistake

With execa wee need shell: true for running npm commands.

npm is not a binary but a shell file. npm requires using cmd.exe or Bash to be loaded. The shell option does that.

Refers sindresorhus/execa#433 (comment)

Is there also a reason the shell must run async? Intuitively it does not seem the case, as there is no way the program can continue if the install does not finish.

It's true that npm is a bash script on Unix and a cmd file on Windows.

But later on the comment you've linked adds

If you specify a shell file (as opposed to a binary), on Windows, and do not use the shell option, Execa emulates that option for you (through the dependency node-cross-spawn) by basically running cmd /d /s /c /q "npm run tsc" instead of npm run tsc

I think that must be true, because f the shell option is required, the previous execa calls, all without shell: true would fail and I think someone would have reported the bug. I think the shell: true is really needed if you want the shell to expand or replace something in the command string.

ludofischer · 2022-02-25T21:20:15Z

packages/webpack-cli/src/webpack-cli.ts

@@ -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];


We can re-use the argument array if we keep it separate instead of concatenating immediately into a single string.

ludofischer · 2022-02-26T15:53:52Z

I should have fixed the failing install tests now. I hope all tests pass on CI because I can't fix one test that's always failed for me locally (even on master). test/watch/analyze/analyze-flag.test.js reports:

● "analyze" option › should load webpack-bundle-analyzer plugin with --analyze flag

expect(received).toContain(expected) // indexOf

Expected substring: "Webpack Bundle Analyzer is started at"
Received string:    "Webpack Bundle Analyzer is started at http://127.0.0.1:8888
Use Ctrl+C to close it"

The string does contain the substring so I have little idea of what's wrong.

ludofischer · 2022-02-26T16:46:36Z

The smoketests all pass on my machine, so I can't say why they fail now. They also passed on the previous CI run.

alexander-akait · 2022-02-26T17:22:20Z

Can you try to update lock file?

ludofischer · 2022-02-26T18:46:38Z

Can you try to update lock file?

I've updated all versions in yarn.lock.

alexander-akait · 2022-02-27T18:58:43Z

Oh, looks like we should update types when we updating lock file

ludofischer · 2022-03-04T22:36:49Z

Oh, looks like we should update types when we updating lock file

I think it's the opposite, updating @types/yeoman-environment from 2.1.0.5 to 2.10.6 breaks the type check. I've removed the commit that updated all dependencies and rebased, since some dependencies have been updated on master in the meantime.

alexander-akait · 2022-03-05T10:41:32Z

hm, we have a problems with:

RUN  Missing webpack-dev-server
  swapping webpack-dev-server with .webpack-dev-server
  timeout: killing process
  swapping .webpack-dev-server with webpack-dev-server
FAIL  Missing webpack-dev-server

RUN  Missing webpack
  swapping webpack with .webpack
  timeout: killing process
  swapping .webpack with webpack
FAIL  Missing webpack

RUN  Missing webpack-bundle-analyzer
  swapping webpack-bundle-analyzer with .webpack-bundle-analyzer
  timeout: killing process
  swapping .webpack-bundle-analyzer with webpack-bundle-analyzer
FAIL  Missing webpack-bundle-analyzer

That is weird...

ludofischer · 2022-03-05T20:33:22Z

That is weird...

I think the problem is timeout: killing process, not the missing ... messages, because the missing messages also appear when the tests pass.
The smoketests all pass on my machine, even when I follow the exact same steps as in CI. I think the timeout comes from smoketests/helpers.js

webpack-cli/smoketests/helpers.js

Line 45 in 80ae4a2

console.log(" timeout: killing process");

but it's set to 30 seconds, which should be plenty (on my machine each tests runs in a second or so).

alexander-akait · 2022-03-06T13:58:49Z

Can we try to increase timeout? Sorry for this, don't have time on deep investigation for this

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

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.

Mock the `sync` property. Split command into command name and argument array.

ludofischer · 2022-03-07T10:10:20Z

Can we try to increase timeout? Sorry for this, don't have time on deep investigation for this

I've increased the timeout to 60 seconds.

codecov · 2022-03-07T11:32:09Z

Codecov Report

Merging #3146 (db9c10c) into master (b1ad914) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3146      +/-   ##
==========================================
- Coverage   91.50%   91.44%   -0.06%     
==========================================
  Files          23       23              
  Lines        1719     1719              
  Branches      519      519              
==========================================
- Hits         1573     1572       -1     
- Misses        146      147       +1

Impacted Files	Coverage Δ
packages/webpack-cli/src/webpack-cli.ts	`93.95% <100.00%> (-0.10%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1ad914...db9c10c. Read the comment docs.

alexander-akait · 2022-03-07T14:32:17Z

Maybe we need increase some more

ludofischer · 2022-03-07T15:20:35Z

Maybe we need increase some more

I'm not sure, that timeout only affects the smoketests but now it's the other tests hanging. Anyway, this time the smoketests passed in less than 1 min on Ubuntu, I think either the containers are too slow or there is some deadlock issue.

alexander-akait

/cc @webpack/cli-team Let's review

anshumanv

Thanks

webpack-bot added the PR: unreviewed label Feb 25, 2022

ludofischer commented Feb 25, 2022

View reviewed changes

ludofischer marked this pull request as ready for review February 25, 2022 21:20

ludofischer requested a review from a team as a code owner February 25, 2022 21:20

ludofischer force-pushed the remove-execa branch from ad239b5 to e9ca015 Compare March 4, 2022 22:33

ludofischer added 4 commits March 7, 2022 11:07

chore(webpack-cli): replace execa with cross-spawn

1f11ea1

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

test: fix cross-spawn mock in install tests

ef542db

Mock the `sync` property. Split command into command name and argument array.

test: increase smoke tests timeout to 60 seconds

db9c10c

ludofischer force-pushed the remove-execa branch from 70b1721 to db9c10c Compare March 7, 2022 10:08

alexander-akait approved these changes Mar 7, 2022

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: unreviewed labels Mar 7, 2022

rishabh3112 approved these changes Mar 8, 2022

View reviewed changes

snitin315 approved these changes Mar 8, 2022

View reviewed changes

anshumanv approved these changes Mar 8, 2022

View reviewed changes

snitin315 merged commit ce74898 into webpack:master Mar 8, 2022

ludofischer deleted the remove-execa branch March 8, 2022 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(webpack-cli): don't spawn needless shells and replace execa with cross-spawn #3146

refactor(webpack-cli): don't spawn needless shells and replace execa with cross-spawn #3146

ludofischer commented Feb 25, 2022

ludofischer Feb 25, 2022

alexander-akait Feb 26, 2022

snitin315 Feb 27, 2022 •

edited

Loading

ludofischer Feb 27, 2022

ludofischer Feb 25, 2022

ludofischer commented Feb 26, 2022

ludofischer commented Feb 26, 2022

alexander-akait commented Feb 26, 2022

ludofischer commented Feb 26, 2022

alexander-akait commented Feb 27, 2022

ludofischer commented Mar 4, 2022

alexander-akait commented Mar 5, 2022

ludofischer commented Mar 5, 2022 •

edited

Loading

alexander-akait commented Mar 6, 2022

ludofischer commented Mar 7, 2022

codecov bot commented Mar 7, 2022 •

edited

Loading

alexander-akait commented Mar 7, 2022

ludofischer commented Mar 7, 2022

alexander-akait left a comment

anshumanv left a comment

refactor(webpack-cli): don't spawn needless shells and replace execa with cross-spawn #3146

refactor(webpack-cli): don't spawn needless shells and replace execa with cross-spawn #3146

Conversation

ludofischer commented Feb 25, 2022

ludofischer Feb 25, 2022

Choose a reason for hiding this comment

alexander-akait Feb 26, 2022

Choose a reason for hiding this comment

snitin315 Feb 27, 2022 • edited Loading

Choose a reason for hiding this comment

ludofischer Feb 27, 2022

Choose a reason for hiding this comment

ludofischer Feb 25, 2022

Choose a reason for hiding this comment

ludofischer commented Feb 26, 2022

ludofischer commented Feb 26, 2022

alexander-akait commented Feb 26, 2022

ludofischer commented Feb 26, 2022

alexander-akait commented Feb 27, 2022

ludofischer commented Mar 4, 2022

alexander-akait commented Mar 5, 2022

ludofischer commented Mar 5, 2022 • edited Loading

alexander-akait commented Mar 6, 2022

ludofischer commented Mar 7, 2022

codecov bot commented Mar 7, 2022 • edited Loading

Codecov Report

alexander-akait commented Mar 7, 2022

ludofischer commented Mar 7, 2022

alexander-akait left a comment

Choose a reason for hiding this comment

anshumanv left a comment

Choose a reason for hiding this comment

snitin315 Feb 27, 2022 •

edited

Loading

ludofischer commented Mar 5, 2022 •

edited

Loading

codecov bot commented Mar 7, 2022 •

edited

Loading