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

Ability to override process.env #39

Closed
shawnmclean opened this issue Apr 1, 2019 · 6 comments
Closed

Ability to override process.env #39

shawnmclean opened this issue Apr 1, 2019 · 6 comments

Comments

@shawnmclean
Copy link
Contributor

In a case of using Create-React-App, it seems they are doing something funky with their process variable. That is, if you access process via require or import, it will be null. So this lib will never get the env vars set in by this app.

Would you mind adding an extra param for get(envVar, default, container) that allows us to pass in our own process.env as the 3rd param?

I could do a PR if you prefer that route.

@evanshortiss
Copy link
Owner

evanshortiss commented Apr 1, 2019

That's interesting. I've only used create-react-app a few times so never came across this. Nice find.

If we just removed this line would the problem be fixed?

Or would a better solution be to change the module behaviour so it works like so:

// Pass in the container
const env = require('env-var')(process)

env.get('name', DEFAULT_NAME).asString()

This would be a major version bump of course, and would mean we could remove env.mock. This is probably better since it's a more flexible approach, and I'd rather not pass the container to every env.get call.

What do you think?

@shawnmclean
Copy link
Contributor Author

Removing the import seems like a quick solution that may not need a major bump.

The second solution to pass in the container will make it easier to test/mock, which would be the way I'd take. I agree with passing in the container once.

@evanshortiss
Copy link
Owner

evanshortiss commented Apr 1, 2019

@shawnmclean want to PR the removal of the require? I'll merge and release it as a MINOR PATCH release.

@evanshortiss evanshortiss mentioned this issue Apr 8, 2019
Merged
@evanshortiss
Copy link
Owner

@shawnmclean #42 should solve your issue. It also changes the env.mock function name to something more appropriate so developers know they can use it to create env instances that read from whatever object they need, not just process.env

@evanshortiss
Copy link
Owner

evanshortiss commented Apr 9, 2019

@shawnmclean I'd still welcome a PR on previous master to release as a patch for 3.x so if you have something I'd merge it into a 3.x branch and make a release 👍

@evanshortiss
Copy link
Owner

Closing this @shawnmclean. Feel free to reopen if you need

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

No branches or pull requests

2 participants