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

feat: support for overriding environment vars. fix #199 #370

Closed

Conversation

alexewerlof
Copy link

This PR adds support for an override option to config() that is false by default.

The truth is that the workaround suggested here is not the most elegant way of solving the problem and creates a messy setupfilesafterenv script in jest. This could easily be avoided if this feature was natively supported.
This feature is popularly demanded (#199) and can be great to add it.

I've updated the documentations with an example.
Also I've added tests with 100% coverage.

@alexewerlof alexewerlof changed the title add support for overriding environment vars. fix #199 feat: support for overriding environment vars. fix #199 Jan 31, 2019
@maxbeatty maxbeatty closed this Jan 31, 2019
@maxbeatty
Copy link
Contributor

Please fork if you want this dangerous behavior

@alexewerlof
Copy link
Author

alexewerlof commented Jan 31, 2019

@maxbeatty why is it a dangerous behavior? At my job we have gone to great extent to work around it and that code is not less dangerous than having the feature in the dependency itself.

According to #115 #199 it is a popularly requested feature. The PR implements it as is opt-in which means people have to read the docs and warnings.

I understand that this is not a feature that everyone uses it but when you need it, the workaround is limited to copy/pasting from the Readme.

We appreciate if you could consider adding this feature. Forking is one way to add it but it feels rather unproductive to create exactly the same module with a different name and publish it to NPM just for this feature.

@maxbeatty
Copy link
Contributor

I encourage you to also read through...

#34
#55
#63
#72
#123
#130
#137
#231
#331

This isn’t a new request. I am firm on not supporting it. If you do not want to use the documented workaround, then the my next suggestion is to fork and maintain separately. I hope you can understand why I do not wish to spend any more time discussing.

@alexewerlof
Copy link
Author

alexewerlof commented Jan 31, 2019

Thanks for listing the relevant issues. I guess the discussion is over then. 🏳️


I just leave this for anyone who wants to spends the time to contribute a PR to add this feature:

The main counterargument is that you may accidentally override variables that screws up your code given three conditions:

  1. You have environment variables with names like PATH, PWD, etc in your .env file
  2. You explicitly choose the override: true in your config() options
  3. Your code (or your dependencies) go crazy with values that you assign to them

dotenv chooses a safe default behavior which other packages like this one also use.

The unique selling point of dotenv module is to parse and set the environment variables. It is unfortunate that it doesn't complete the job but it gets you pretty far and you can always copy/paste a workaround from its own site.

I'm not going to publish my fork just for this feature but the possibility to create a sister module that does exactly env var overriding is open.

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

2 participants