Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Mar 22, 2024

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
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 22, 2024
@timfish

This comment was marked as resolved.

@styfle

This comment was marked as outdated.

@H4ad

This comment was marked as outdated.

@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 23, 2024
@targos

This comment was marked as outdated.

@anonrig

This comment was marked as resolved.

@anonrig anonrig force-pushed the anonrig/add-node-run-command branch from 424a48c to dae84d3 Compare March 23, 2024 14:35
@anonrig

This comment was marked as resolved.

@joyeecheung

This comment was marked as outdated.

@anonrig anonrig force-pushed the anonrig/add-node-run-command branch from dae84d3 to 0c7a248 Compare March 23, 2024 15:09
@wesleytodd

This comment was marked as outdated.

@wesleytodd

This comment was marked as resolved.

@joyeecheung

This comment was marked as off-topic.

@tniessen
Copy link
Member

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.

@styfle

This comment was marked as resolved.

doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
@anonrig anonrig force-pushed the anonrig/add-node-run-command branch 2 times, most recently from 05d4b8a to 0277f43 Compare March 25, 2024 15:07
@anonrig

This comment was marked as resolved.

@anonrig anonrig requested a review from mcollina March 25, 2024 15:27
@anonrig anonrig added the notable-change PRs with changes that should be highlighted in changelogs. label Mar 25, 2024
Copy link
Contributor

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.

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 25, 2024
doc/api/cli.md Outdated Show resolved Hide resolved
@meyfa

This comment was marked as outdated.

@bnb
Copy link
Contributor

bnb commented Apr 7, 2024

@GeoffreyBooth

Not necessary for this PR, but it would be great to add a benchmark for this.

I'd love to learn how to add benchmarks by doing this, happy to pair with someone on this if anyone's interested 👍🏻

doc/api/cli.md Outdated Show resolved Hide resolved
lib/internal/main/run.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@anonrig anonrig removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Apr 7, 2024
Co-authored-by: Daniel Lemire <daniel@lemire.me>
@anonrig anonrig force-pushed the anonrig/add-node-run-command branch from 0a38c95 to 2771947 Compare April 7, 2024 18:32
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 7, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 7, 2024
@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 7, 2024
@nodejs-github-bot
Copy link
Collaborator

@richardlau richardlau added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 8, 2024
@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 8, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 8, 2024
@nodejs-github-bot nodejs-github-bot merged commit 128c60d into nodejs:main Apr 8, 2024
59 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 128c60d

Copy link

@LuiSauter LuiSauter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

if (args.length > 0) {
command += ' ' + escapeShell(args.map((arg) => arg.trim()).join(' '));
}
execSync(command, { stdio: 'inherit', env, shell: true });
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was re-written in cpp 🙂

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

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);
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @atlowChemi for clarifying!

@marco-ippolito marco-ippolito added the baking-for-lts PRs that need to wait before landing in a LTS release. label May 21, 2024
@anonrig
Copy link
Member Author

anonrig commented May 30, 2024

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

@marco-ippolito
Copy link
Member

marco-ippolito commented May 31, 2024

@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 marco-ippolito removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Jun 17, 2024
@marco-ippolito
Copy link
Member

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

@marco-ippolito marco-ippolito added the backport-blocked-v20.x PRs that should land on the v20.x-staging branch but are blocked by another PR's pending backport. label Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-blocked-v20.x PRs that should land on the v20.x-staging branch but are blocked by another PR's pending backport. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.