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

Task runner (node --run) should set lifecycle_event environment variable #52673

Open
justinfagnani opened this issue Apr 24, 2024 · 11 comments
Open
Assignees
Labels
feature request Issues that request new features to be added to Node.js. stale

Comments

@justinfagnani
Copy link

What is the problem this feature will solve?

Some npm scripts delegate to another task runner (like Wireit) and require the environment variable npm_lifecycle_event to be set in order to tell which script to run. Yarn and pnpm both set this variable.

The way a Wireit package.json is configured is like this:

{
  "scripts": {
    "build": "wireit",
  },
  "wireit": {
    "build": {
      "command": "tsc"
    }
  }
}

and it's run with npm run build, yarn run build, etc. So Wireit needs npm_lifecycle_event to find the script to run.

Supporting task runner like Wireit would be great because Wireit caches script results, so some invocations don't perform any work after checking the cache and the npm overhead could be noticeable.

Downstream issue: google/wireit#1094

What is the feature you are proposing to solve the problem?

node --run set the npm_lifecycle_event environment variable to the current script.

What alternatives have you considered?

Node could set a different variable and task runners like Wireit could detect that also.

@justinfagnani justinfagnani added the feature request Issues that request new features to be added to Node.js. label Apr 24, 2024
@anonrig
Copy link
Member

anonrig commented Apr 24, 2024

node task runner is not a replacement for npm. Please take a look at the intentional limitations part in the documentation.

@anonrig anonrig closed this as not planned Won't fix, can't repro, duplicate, stale Apr 24, 2024
@justinfagnani
Copy link
Author

node task runner is not a replacement for npm. Please take a look at the intentional limitations part in the documentation.

This issue isn't asking for a replacement to npm, but a way to know which script was run so that delegated task runners can work.

@anonrig
Copy link
Member

anonrig commented Apr 24, 2024

@justinfagnani The limitations include any npm or other package managers specific environment variables. Your request is to add the npm_lifecycle_event environment variable right? We can introduce "lifecycle_event" environment variable but this won't solve your issue.

@justinfagnani
Copy link
Author

A lifecycle_event variable would be great. Then task runners could look for that variable too. Right now there appears to be no way to know which script was requested.

@anonrig anonrig reopened this Apr 24, 2024
@anonrig anonrig self-assigned this Apr 24, 2024
@anonrig
Copy link
Member

anonrig commented Apr 24, 2024

Makes sense. Looks like a good feature. I've reopened it and will work on it after #52609 lands.

@justinfagnani justinfagnani changed the title Task runner (node --run) should set npm_lifecycle_event environment variable Task runner (node --run) should set lifecycle_event environment variable Apr 24, 2024
@justinfagnani
Copy link
Author

Thank you @anonrig!

Looking at the Wireit source code, it looks like it would need an equivalent to npm_package_json (ie, just package_json) as well to identify the package.json file to read configuration from.

It also reads additional args, in npm via another env variable, but it might work with the additional args support you've already added (cc @aomarks). I realize there would be an understandable resistance to adding too many environment variables.

@aomarks
Copy link

aomarks commented Apr 25, 2024

Cool! Wireit maintainer here. I'm excited for node --run, and would love for wireit to support it.

What wireit needs to know

  1. Which script runner are we using (npm, node --run, pnpm, yarn)? So that we know where to look for the next 4 items.
  2. Which package.json are we working with?
  3. Which script is running?
  4. Should we run in --watch mode? (Wireit has its own watch mode which watches all files across the entire transitive dependency graph of a script, re-executing only each script as needed).
  5. Which additional arguments should we pass down to the process?

Example

npm, pnpm, and yarn all provide this information one way or another. It varies across package managers and across their versions. Here's an example of how it works for npm:

npm run myscript --watch -- arg1 arg2

This means "run myscript, with wireit's watch mode enabled, and pass arg1 and arg2 down to the process." The way we figure this out is:

  1. Read the npm_config_user_agent environment variable. It will be "npm/10.5.1 node/v22.0.0 darwin arm64 workspaces/false" in this example.

  2. Read the npm_package_json environment variable if set (npm 8 onwards). If it's not set, walk up the filesystem from the CWD until we find a package.json file. It will be "/path/to/package.json" in this example.

  3. Read the npm_lifecycle_event environment variable. It will be "myscript" in this example.

  4. Read the npm_config_watch environment variable. It will be "true" in this example.

  5. Read argv.slice(2). It will be ["arg1", "arg2"] in this example.

The relevant code is at https://github.com/google/wireit/blob/main/src/cli-options.ts

Suggestions

My thoughts about how this all affects node --run:

npm_lifecycle_event

I agree with the assessment above that setting npm_lifecycle_event or equivalent is the most important thing. That would unlock most of the functionality of wireit.

I would gently encourage actually just using the npm_lifecycle_event name instead of lifecycle_event, because it has sort of become a defacto standard, despite the npm_ prefix. npm, pnpm, yarn, wireit itself, and probably other runners all set npm_lifecycle_event. Another name is also fine, though, since we can just add special support for it in wireit.

npm_config_user_agent

The npm_config_user_agent environment variable would be very nice to set, since that would give us a clear signal of which runner we're dealing with, so that we can then interpret all the other environment data correctly. npm, pnpm, and yarn all set npm_config_user_agent today, so it would be nice to follow that defacto standard.

npm_package_json

The npm_package_json environment variable would be nice, but its only benefit for wireit would be a slight performance optimization, since we can always just look at the filesystem if it's not set.

--watch flag

Wireit has its own watch mode, which is enabled by setting the --watch flag. It's also likely we'll add more wireit flags in the future, things like --clean to bust the cache, or --verbose for more debugging info.

The way this works with npm is that any argument after npm run myscript but before -- is converted into the environment variable npm_config_<name>. I concede this is a bit odd, since it means something like npm run myscript --typo means the --typo flag is silently ignored. It does enable some useful functionality though, since it lets us distinguish runner flags from script flags.

Currently, node --run interprets all arguments before the -- as being arguments to node itself, so something like node --run myscript --watch -- arg1 arg2 errors.

We can't really use node --run myscript -- --watch arg1 arg2 as the syntax, because then we can't distinguish between arguments for wireit vs arguments for underlying script process.

I think the only syntax that would currently work is node --run myscript -- --watch -- arg1 arg2.

I'm not sure what I'd suggest here -- allowing delegated runner arguments would be really nice, and would match the behavior of npm/pnpm/yarn, but it does have that downside of invalid arguments getting ignored in some cases.

@anonrig
Copy link
Member

anonrig commented May 17, 2024

@aomarks Thank you for the response. Sorry I couldn't respond in a timely manner, but I've opened a PR to add NODE_LIFECYCLE_EVENT environment variable: #53032

nodejs-github-bot pushed a commit that referenced this issue May 19, 2024
PR-URL: #53032
Refs: #52673
Reviewed-By: Daniel Lemire <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@anonrig
Copy link
Member

anonrig commented May 19, 2024

@aomarks I've opened one more pull-request to add NODE_RUN_PACKAGE_JSON_PATH #53058 to node --run

npm_config_user_agent

Unfortunately, I don't think we should add a user_agent kind environment variable. I think it's unnecessary at this point. NODE_RUN_SCRIPT_NAME is only available when node --run is executed, and you can use the existence of that to validate if it's node --run that runs the context.

--watch

I didn't quite understand the --watch flag argument. Do you want to get access to any cli arguments passed after the script name and before the --? For example, for the following command node --run test --my-flag -- --help, do you want to know --my-flag in an environment variable?

@aomarks
Copy link

aomarks commented May 20, 2024

@aomarks I've opened one more pull-request to add NODE_RUN_PACKAGE_JSON_PATH #53058 to node --run

Awesome!

npm_config_user_agent

Unfortunately, I don't think we should add a user_agent kind environment variable. I think it's unnecessary at this point. NODE_RUN_SCRIPT_NAME is only available when node --run is executed, and you can use the existence of that to validate if it's node --run that runs the context.

👍

--watch

I didn't quite understand the --watch flag argument. Do you want to get access to any cli arguments passed after the script name and before the --? For example, for the following command node --run test --my-flag -- --help, do you want to know --my-flag in an environment variable?

Yes, exactly.

targos pushed a commit that referenced this issue May 21, 2024
PR-URL: #53032
Refs: #52673
Reviewed-By: Daniel Lemire <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
nodejs-github-bot pushed a commit that referenced this issue May 21, 2024
PR-URL: #53058
Refs: #52673
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Daniel Lemire <[email protected]>
targos pushed a commit that referenced this issue Jun 1, 2024
PR-URL: #53058
Refs: #52673
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Daniel Lemire <[email protected]>
EliphazBouye pushed a commit to EliphazBouye/node that referenced this issue Jun 20, 2024
PR-URL: nodejs#53032
Refs: nodejs#52673
Reviewed-By: Daniel Lemire <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
EliphazBouye pushed a commit to EliphazBouye/node that referenced this issue Jun 20, 2024
PR-URL: nodejs#53058
Refs: nodejs#52673
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Daniel Lemire <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
PR-URL: nodejs#53032
Refs: nodejs#52673
Reviewed-By: Daniel Lemire <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
PR-URL: nodejs#53058
Refs: nodejs#52673
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Daniel Lemire <[email protected]>
@RedYetiDev RedYetiDev moved this from Awaiting Triage to In Progress in Node.js feature requests Jun 26, 2024
aomarks added a commit to google/wireit that referenced this issue Aug 22, 2024
Adds support for the new node --run feature which is available starting in Node 22 (basically npm run but faster, see https://nodejs.org/en/blog/announcements/v22-release-announce#running-packagejson-scripts and https://www.yagiz.co/developing-fast-builtin-task-runner/).

Note that node --run is stricter than the other runners when it comes to distinguishing between arguments for the runner vs the script, so an additional -- is needed to set wireit flags and script flags (explained in the README).

Thank you very much to @anonrig for adding the environment variables this required (nodejs/node#53032, nodejs/node#53058) and @justinfagnani for filing the issue (nodejs/node#52673)!

There is a problem with recursive invocations on Windows that I believe is a Node bug but need to double-check, tracking at #1168.

Fixes #1094
Copy link
Contributor

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale Mark issue so that it is never considered stale label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. stale
Projects
Status: In Progress
Development

No branches or pull requests

3 participants