node icon indicating copy to clipboard operation
node copied to clipboard

cli: make run and watch modes friends

Open bmeck opened this issue 1 year ago • 7 comments
trafficstars

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

https://github.com/nodejs/node/assets/234659/bd5c4fb0-bbbf-48ab-9e6c-b2679f08b3fa

bmeck avatar Jun 14 '24 19:06 bmeck

Review requested:

  • [ ] @nodejs/loaders
  • [ ] @nodejs/startup

nodejs-github-bot avatar Jun 14 '24 19:06 nodejs-github-bot

@nodejs/tooling

GeoffreyBooth avatar Jun 14 '24 20:06 GeoffreyBooth

Why is it wonky?

mcollina avatar Jun 15 '24 06:06 mcollina

@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 avatar Jun 17 '24 14:06 bmeck

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

nodejs-github-bot avatar Jun 24 '24 15:06 nodejs-github-bot

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

nodejs-github-bot avatar Jun 24 '24 17:06 nodejs-github-bot

@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.

bmeck avatar Jun 25 '24 16:06 bmeck