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

Return inserted, in addition to parsed #182

Closed
wants to merge 1 commit into from
Closed

Conversation

motdotla
Copy link
Owner

@motdotla motdotla commented Mar 31, 2017

@jcblw and @maxbeatty I'd like to get your guys' thoughts on this scenario:

  1. An environment variable, SOME_ENV, is preset on the machine
  2. SOME_ENV is also set in the .env file
  3. dotenv runs and does not override the preset SOME_ENV, correctly.
  4. the return object returns:
{ 
  parsed: {
    NODE_ENV: 'value-from-the-env-file'
  }
}

In this scenario the value returned is not representative of what is set in memory.

This PR is proposing to additionally return an inserted object, but what do you guys think is the right move here.

I came across this when patching up dotenv-expand. Any others building libs on top of dotenv would come across the same current bug and have to write their own patch. motdotla/dotenv-expand#6

@coveralls
Copy link

coveralls commented Mar 31, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 37dade6 on sm-inserted into 825c1b2 on master.

@coveralls
Copy link

coveralls commented Mar 31, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 37dade6 on sm-inserted into 825c1b2 on master.

@maxbeatty
Copy link
Contributor

What are the advantages of inserted over process.env?

@motdotla
Copy link
Owner Author

Less noise. process.env typically has ~30 preset envs.

and also arguably less confusion for the developer. Dotenv is not operating on any of those preset envs (except in the case it's also defined in the .env file) so returning all of them could feel odd.

@maxbeatty
Copy link
Contributor

Sorry, I wasn't trying to suggest returning process.env as inserted, but rather trying to clarify the benefits of using config.inserted instead of process.env directly. Take your patch for example:

-var value = process.env[configKey] || config.parsed[configKey]
+var value = config.inserted[configKey]

Feels like you're just trading where you ultimately have to do this check (from there to here) if you want to know what was already defined in process.env.

Ultimately if you as a developer want some values from a file guaranteed to be present, why not just require a JSON file and assign it to your own custom global leaving process.env alone?

global.my_example = require('./git-ignored-config.json')
db.connect(global.my_example.db_host)

If you're shipping your code somewhere where you don't control the environment, it's your responsibility as a developer to choose unique (most likely namespaced) environment variables (e.g. not PATH).

I agree that parsed should not be interpreted as "assigned" or "inserted" and perhaps that's best addressed in documentation.

@motdotla
Copy link
Owner Author

motdotla commented Apr 2, 2017

Let me make sure I follow, are you suggesting modules that add functionality on top of dotenv (like dotenv-expand) should just iterate through all the keys in process.env rather than hook into config.parsed?

@maxbeatty
Copy link
Contributor

If you want to know what dotenv parsed, check config.parsed. If you want to know what was inserted into the environment, check process.env (the ultimate source of truth).

@jcblw
Copy link
Collaborator

jcblw commented Apr 2, 2017

👍 I do think this good information to expose for plugins.

dotenv-expand probably should not do anything with variables that are already in process.env. @motdotla do you know if there are any other practical uses for this?

@motdotla
Copy link
Owner Author

I agree with @jcblw. And it is very little overhead. I'll get this merged and released this week.

@motdotla motdotla closed this Jul 5, 2017
@maxbeatty maxbeatty deleted the sm-inserted branch June 19, 2018 06:16
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.

None yet

4 participants