cli: implement node --run <script-in-package-json>

[anonrig]

This is a more up to date implementation of @joyeecheung's original pull-request.

Co-authored-by: Daniel Lemire

The purpose of this pull-request to offer a fast alternative to npm run xxx. With this pull-request, node supports node run test which executes test command inside package.json scripts.

Locally the overhead that node run <cmd> adds to running cmd is ~200ms less than npm run <cmd>

TODO

• Add documentation
• Add tests
• Add node_modules/.bin to PATH.
• Pass additional arguments to execSync

Benchmark

❯ hyperfine './out/Release/node run test' 'npm run test' -i
Benchmark 1: ./out/Release/node run test
  Time (mean ± σ):      29.3 ms ±   1.1 ms    [User: 23.2 ms, System: 3.1 ms]
  Range (min … max):    27.6 ms …  33.2 ms    97 runs

  Warning: Ignoring non-zero exit code.

Benchmark 2: npm run test
  Time (mean ± σ):     185.7 ms ±   9.2 ms    [User: 136.7 ms, System: 30.3 ms]
  Range (min … max):   174.7 ms … 212.9 ms    15 runs

  Warning: Ignoring non-zero exit code.

Summary
  ./out/Release/node run test ran
    6.34 ± 0.40 times faster than npm run test

[nodejs-github-bot]

Review requested:

• @nodejs/startup

[tniessen]

I don't have a strong opinion on this, but I do share some concerns:

• npm does set many environment variables for scripts, and this already causes headaches when running through yarn, for example. It seems that node run might further complicate things by adding yet another implementation.
• scripts are often spawned recursively, e.g., npm run foo && npm run bat. It doesn't matter much how fast node run is if recursive calls don't use it.

[github-actions]

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @anonrig.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

[ShogunPanda]

Other than the --run updates everywhere, it LGTM!
Nice job!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/58183/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/58194/

[nodejs-github-bot]

Landed in 128c60d

[LuiSauter]

nice

[manoraj]

Question: Have you considered using spawn instead of execSync, which has limitations in buffer size?

[atlowChemi]

This was re-written in cpp 🙂

node/src/node_task_runner.cc

Lines 286 to 288 in b1c1faf

	auto runner =
	ProcessRunner(result, std::string(command_id), command, positional_args);
	runner.Run();

node/src/node_task_runner.cc

Lines 216 to 222 in b1c1faf

	void ProcessRunner::Run() {
	if (int r = uv_spawn(loop_, &process_, &options_)) {
	fprintf(stderr, "Error: %s\n", uv_strerror(r));
	}
	uv_run(loop_, UV_RUN_DEFAULT);
	}

[manoraj]

Thanks, @atlowChemi for clarifying!

[anonrig]

@marco-ippolito what's the reason for making this "baking for lts"?

[marco-ippolito]

@marco-ippolito what's the reason for making this "baking for lts"?

Being a big new feature, I think we should have waited more than two weeks before landing it in LTS. I'm ok with landing this in v20.15.0

[marco-ippolito]

@anonrig this pr doesnt land cleanly on v20, I think it requires #50322 to be landed first