-
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
Allow 'production' ENV to take precedence over NODE_ENV #2057
Conversation
a5295df
to
e2d8c62
Compare
Huh. The test is passing locally. Digging in to see why it isn't working on CI. |
ce2ba3d
to
8d1b78b
Compare
I'm not sure why my first approach wasn't working. It worked locally for some reason. (Anyone have any ideas?) But using the same method as #1455 worked. This is all good to go. |
@joeyespo, do we have a support for
|
@bestander Rebased. One test is failing due to There's no |
@joeyespo, this is a known problem, we'll sort it out separately |
@@ -210,7 +210,10 @@ export default class Config { | |||
await fs.mkdirp(this.cacheFolder); | |||
await fs.mkdirp(this.tempFolder); | |||
|
|||
if (this.getOption('production') || process.env.NODE_ENV === 'production') { | |||
if (this.getOption('production') || ( |
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.
There's no --no-production to my knowledge, but this does work exactly like npm where you can set NPM_CONFIG_PRODUCTION=false in your config.
My concern is that if NODE_ENV is set to production
overriding it can only be done with another env variable, e.g. YARN_PRODUCTION.
Sometimes people don't have full control over environment variables in the system and overriding with an arg can be a more obvious API.
In npm you can do --production=false
and it will override env variables.
Can you do similar thing for Yarn?
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.
What if the opposite is true, where people only have control over env variables rather than CLI arguments? E.g. when running on Heroku under the constraints as described in #2283
I agree with you on the usefulness of that. Yarn currently doesn't support it. (It appears npm does.) This PR fixes real production compatibility issue though (heroku/heroku-buildpack-nodejs#337). As of #1455, you can already set The |
related #2167 |
@chadly Perfect 👍 @bestander Given that the test timeout is a known thing to be sorted out separately, this should be ready to go. Need anything else from me? |
Sounds reasonable, @joeyespo, thanks for the contribution! |
This fixes #1975.
The motivation is to fix the compatibility issue so that Yarn can be used in place of npm on Heroku (related buildpack issue). In this case, when you need to install devDependencies, as documented here.
Test plan
A test is included.
(I also tested locally on my setup and observed that the devDependencies were indeed installed to
node_modules
when bothNODE_ENV=production
andNPM_CONFIG_PRODUCTION=false
were set.)