-
Notifications
You must be signed in to change notification settings - Fork 340
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dev: Pass .env vars as an object to child process #764
Conversation
src/commands/dev/index.js
Outdated
@@ -424,6 +424,7 @@ class DevCommand extends Command { | |||
const envFile = path.resolve(site.root, '.env') | |||
if (fs.existsSync(envFile)) { | |||
const vars = dotenv.parse(fs.readFileSync(envFile)) || {} | |||
settings.env = { ...settings.env, ...vars } | |||
console.log(`${NETLIFYDEVLOG} Overriding the following env variables with ${chalk.blue('.env')} file:`, chalk.yellow(Object.keys(vars))) | |||
Object.entries(vars).forEach(([key, val]) => process.env[key] = val) |
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.
process.env[key]
being set here. Is not accessible two lines down when accessed in startDevServer
.
cc @erquhart
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.
The cause for this bug is the spread order of properties here:
Line 341 in 3085892
env: { ...process.env, ...settings.env, FORCE_COLOR: 'true' }, |
settings.env
properties override process.env
properties.
Changing that line to env: { ...settings.env,...process.env, FORCE_COLOR: 'true' },
fixes it
a3a26f5
to
519c81b
Compare
Ah I see. Thanks a bunch @erezrokah. This was a head scratcher. I never looked at the detectors logic, I thought they only returned application specific env variables. |
Can you please take another look @erezrokah |
Sure, reviewing now |
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 change works since each detector inherits env
from process.env
right?
So future detectors will have to make sure to do the same?
Don't know if that is a real issue or not
Yeah. All of the detectors inherit |
- Summary
This is to fix an inconsistency in NodeJS which results in
process.env
not realizing changes if they're accessed shortly after.Fixes: #747
- Test plan
create-react-app
project connected to Netlify production withnetlify link
..env
file in project root locally that overrides that env variablenetlify dev
and observe the value of that env variable- Description for the changelog
Pass the variables from
.env
file assetting.env
object tostartDevServer
, so that, they're set explicitly on the new process.- A picture of a cute animal (not mandatory but encouraged)
馃惓