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: make run and watch modes friends #53457

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Jun 14, 2024

This is without tests until some conversation is had because it is a bit wonky. This makes --watch work with --run so you can do things like node --watch-path src --run start with the following package.json:

{
  "devDependencies": {
    "esbuild": "^0.21.5"
  },
  "scripts": {
    "start": "esbuild --platform=node --bundle src/main.mts --format=cjs --outfile=dist/main.cjs && node dist/main.cjs"
  }
}

Demo

Screen.Recording.2024-06-14.at.2.48.26.PM.mov

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @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 Jun 14, 2024
@GeoffreyBooth
Copy link
Member

@nodejs/tooling

@GeoffreyBooth GeoffreyBooth added the cli Issues and PRs related to the Node.js command line interface. label Jun 14, 2024
@mcollina
Copy link
Member

Why is it wonky?

@bmeck
Copy link
Member Author

bmeck commented Jun 17, 2024

@mcollina

  1. the task runner in C++ doesn't quite match the behavior of the shim I did to make it work with the JS based watch mode (perhaps this is fine since it is just around process management).
  2. the CLI option --run is per_process based but the --watch CLI option is per env. So I had to do a weird pulling out of a env based value in a per_process check https://github.com/nodejs/node/pull/53457/files#diff-6c1386cbc6e4a11941649af31a0257d5c3e40b5f41223cdddf87ab2e984b705aR995
  3. the design of --run (drops positional args) and --watch (uses positional args) have a slight conflict so I banned positional arguments and was seeing some weird results anyway if allowed:
% ../node/node --watch --run a test this a b
Running 'arr=("$@"); echo $# ${#arr} ${#@}'
0 0 0 test this a b

@bmeck bmeck marked this pull request as ready for review June 21, 2024 18:44
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 24, 2024
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

test/sequential/test-watch-mode.mjs Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 24, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

still lgtm

@mcollina mcollina requested a review from anonrig June 24, 2024 17:27
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 24, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 24, 2024
@nodejs-github-bot
Copy link
Collaborator

@bmeck
Copy link
Member Author

bmeck commented Jun 25, 2024

@anonrig said via twitter:

I think this is not the correct solution. We should reuse the existing task runner implementation instead of having 2.

I could take a look at exposing a JS binding for the task runner but it would need a variety of changes if it is to handle both scenarios due to IPC and some event management. If possible I think keeping them separated at least until the exact event management etc of --watch is ironed out would be preferable vs a fairly invasive modification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. cli Issues and PRs related to the Node.js command line interface. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants