-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add npm_lifecycle_script to env in makeEnv #4330
Add npm_lifecycle_script to env in makeEnv #4330
Conversation
0ed4712
to
c6537f9
Compare
const manifest = await config.maybeReadManifest(cwd); | ||
if (manifest) { | ||
if (manifest.scripts && Object.prototype.hasOwnProperty.call(manifest.scripts, stage)) { | ||
env.npm_lifecycle_script = manifest.scripts[stage]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already exposed as npm_lifecycle_event
on line 50 above. Isn't that enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry. I read the original issue now and I understand what this is doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@arcanis - any concerns?
Nope, was waiting for the tests to end. Good to merge! |
Wow thanks for the quick review and edits and merge everyone! 😄 |
So I was using process.env.npm_lifecycle_script to determine if it was yarn or npm running the preinstall script. I would use this to subtly educate team members to use Yarn over NPM. Is there another way I could accomplish this? |
@palmerj3, maybe |
@adityavohra7 cool thanks! Hope my comment didn't seem like I was putting down this PR - you did the right thing :) The link you sent seems like a more proper way to do what i'm looking to do. |
* Add npm_lifecycle_script to env in makeEnv * Update execute-lifecycle-script.js
The environment variable `npm_config_user_agent` can be used to guess the package manager that was used to execute wrangler. It's imperfect (just like regular user agent sniffing!) but the package managers we support all set this property: - [npm](https://github.com/npm/cli/blob/1415b4bdeeaabb6e0ba12b6b1b0cc56502bd64ab/lib/utils/config/definitions.js#L1945-L1979) - [pnpm](https://github.com/pnpm/pnpm/blob/cd4f9341e966eb8b411462b48ff0c0612e0a51a7/packages/plugin-commands-script-runners/src/makeEnv.ts#L14) - [yarn](https://yarnpkg.com/advanced/lifecycle-scripts#environment-variables) - [yarn classic](yarnpkg/yarn#4330)
The environment variable `npm_config_user_agent` can be used to guess the package manager that was used to execute wrangler. It's imperfect (just like regular user agent sniffing!) but the package managers we support all set this property: - [npm](https://github.com/npm/cli/blob/1415b4bdeeaabb6e0ba12b6b1b0cc56502bd64ab/lib/utils/config/definitions.js#L1945-L1979) - [pnpm](https://github.com/pnpm/pnpm/blob/cd4f9341e966eb8b411462b48ff0c0612e0a51a7/packages/plugin-commands-script-runners/src/makeEnv.ts#L14) - [yarn](https://yarnpkg.com/advanced/lifecycle-scripts#environment-variables) - [yarn classic](yarnpkg/yarn#4330)
Summary
Fixes #4003.
The motivation behind this PR is to get more parity with npm wrt the
process.env
variables available when running scripts.Test plan
Made these exact changes in my globally installed
yarn
's node_modules. Using the followingpackage.json
I ran
yarn test
to yield:npm_lifecycle_script
now appears inprocess.env
.