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

fix: allow overriding npm_ env vars via env #222

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

Conversation

legobeat
Copy link

@legobeat legobeat commented Sep 30, 2024

Allow user to override the following env vars via env params:

  • npm_package_json
  • npm_lifecycle_event
  • npm_lifecycle_script

Package defaults still take precedence over process.env.

Precendence order before:

  1. Package defaults
  2. env param
  3. process.env

Precendence order after:

  1. env param
  2. Package defaults
  3. process.env

Related

@legobeat legobeat changed the title env var overrides fix: allow overrideing npm_ env vars via env Sep 30, 2024
@legobeat legobeat changed the title fix: allow overrideing npm_ env vars via env fix: allow overriding npm_ env vars via env Sep 30, 2024
@legobeat legobeat marked this pull request as ready for review September 30, 2024 01:43
@legobeat legobeat requested a review from a team as a code owner September 30, 2024 01:43
prevents runtime error in environments where `require.resolve` is not
available

Fall back to any existing `npm_config_node_gyp` provided by environment
@wraithgar
Copy link
Member

Thanks for your patience on this one. The reason it has sat like this is because node-gyp is not in fact valid npm config. We had planned on (and have just now started work on) making npm warn on configs it sees that aren't valid. This is so that folks are not surprised anymore when they do something like --loglvel or some other typo in any config.

This will mean finding the edges where we are still allowing for invalid configs, and formalize them in some way. node-gyp is one such config. The solution is going to be much bigger than just this PR though. We wanted to wait till work had started so we could address all the unknowns together. This PR is on the list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants