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

Orthogonal Environment Variables #101

Merged
merged 3 commits into from
Jan 20, 2016
Merged

Orthogonal Environment Variables #101

merged 3 commits into from
Jan 20, 2016

Conversation

maxbeatty
Copy link
Contributor

That’s a nice way of saying removing variable expansion. I haven’t been presented with a good use case for variable expansion where copying and pasting (TEST=$BASIC) or string concatenation (process.env.TEST_BASIC = process.env.TEST + process.env.BASIC) wouldn’t be a better solution. By supporting these limited use cases, we're encouraging sub-optimal configuration management and forcing people to escape their variables.

  • removed variable expansion code, tests, and docs
  • removed variable escaping code, tests, and docs
  • added license
  • added changelog
  • removed link to ruby gem of same name to eliminate confusion about feature parity
  • add link to process.env in nodejs docs to make it easier to learn about what we're populating
  • documented load as an alias to config so documentation is more consistent

This would be a breaking change so outlined the changelog as this being a v2 candidate

@motdotla
Copy link
Owner

motdotla commented Nov 5, 2015

I haven’t been presented with a good use case for variable expansion where copying and pasting (TEST=$BASIC) or string concatenation (process.env.TEST_BASIC = process.env.TEST + process.env.BASIC) wouldn’t be a better solution. By supporting these limited use cases, we're encouraging sub-optimal configuration management and forcing people to escape their variables.

My sentiments as well

Love the changelog

I'm going to think on this. @jcblw let's get your thoughts too.

@maxbeatty this is nicely proposed.

@jcblw
Copy link
Collaborator

jcblw commented Nov 7, 2015

Im down for this I think we start encouraging people to start building things ontop of dotenv rather then wanting the functionality inside of dotenv. Maybe along with the bump we should try try to formalize the way to integrate things like variable expansion into dotenv. The same way we added the way to load the .environment file when removing that feature.

eg.

// kinda works like arr.map 
function variableExpansion (key, value, keyValues) {
  ...
  return newValue;
}

dotenv.config({mapValues:[variableExpansion]});

@maxbeatty
Copy link
Contributor Author

We could return the parsed object instead of a boolean from config. This should give someone everything they need to continue setting values on process.env.

const dotenv = require('dotenv')
const variableExpansion = require('dotenv-plugin-varexp')

const myEnv = dotenv.config()
variableExpansion(myEnv)

@jcblw
Copy link
Collaborator

jcblw commented Nov 15, 2015

I don't know if that would be a good integration point tho. It really still leaves alot that would need to be done to emulate the default behavior of dotenv.

const parseObj = dotenv.config()
for (let key in parseObj) {
  ... // do stuff
  // apply to process.env
}

I pushed up an example of working code that emulates that example I have above. orthogonal-var-intergrations branch. What I like is that it add a new option to the parse as well as expose that same functionality to the config method.

Let me know your thoughts @maxbeatty and @motdotla .

@maxbeatty
Copy link
Contributor Author

Feels premature to add an integration point with no proposed integrations. I suspect someone will want variable expansion back in the not-so-distance future. The proposed mapping function is good for that, but what happens when someone wants to do more than one thing? parse might start having more code to handle integrations than parsing.

Since we're ultimately working with one object (process.env), "chaining" calls feels more natural (probably because I've spent a lot of time with jQuery and underscore/lodash).

// jQuery
$('p').find('a').each(...)

// lodash
_.pluck(_.filter(users, 'active', false), 'user');

By exposing dotenv's core competency of a parsed object from a text file, we can allow people to easily continue to alter process.env without maintaining hooks into the core part of our logic. There will be some extra for loops, but node is pretty good at those and it's a one time run.

dotenvValidate(dotenvVarExp(dotenv.config()))

@maxbeatty
Copy link
Contributor Author

Any more thoughts on this? All open issues and PRs could be closed as a result of this change.

@motdotla
Copy link
Owner

The chaining approach is good. Thanks mucho @maxbeatty. Merging.

motdotla added a commit that referenced this pull request Jan 20, 2016
Orthogonal Environment Variables
@motdotla motdotla merged commit a46073a into master Jan 20, 2016
@motdotla motdotla deleted the orthogonal-vars branch January 20, 2016 03:33
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

3 participants