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

Add ability to shut off dotenv invocation #84

Merged

Conversation

gfmartinez
Copy link
Contributor

So, this might be the wrong way to go about doing this, but I've run into two major issues in dealing with this dotenv wrapper for Ember:

  1. When deploying to production in Heroku, the warnings can be confusing and alarming, when they really are meaning less because the environment variables are actually set within the Heroku environment; and

  2. Sometimes the invocation of dotenv and the errors can cause build errors.

So, I've attempted to add the ability for users to bypass the use of dotenv in certain environments.

It seems there may be some consensus that dotenv should only be used for development (https://stackoverflow.com/questions/52546426/is-module-dotenv-for-development-only), but either way, giving control over the use to developers makes sense.

Happy to re-work, delete, or whatever seems appropriate.

Thanks!

index.js Outdated
@@ -24,6 +24,7 @@ module.exports = {
clientAllowedKeys: [],
fastbootAllowedKeys: [],
failOnMissingKey: false,
useDotenvFile: true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are your thoughts on renaming this to enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasonmit That sounds fine to me. Will update and push up shortly.

@andrejcremoznik
Copy link

andrejcremoznik commented May 18, 2020

This definitely needs to be merged. A missing .env file should be silently ignored. It's up to the devs to configure the env via env vars or dotenv. As it is currently, devs are forced into a .env file. That's just not how apps run in production or CI environments.

Edit: actually, this change isn't good enough. You shouldn't have to configure whether it's enabled or not when you can interpret that condition from the presence or absence of the .env file.

@jasonmit jasonmit merged commit dd6cb1b into fivetanley:master May 19, 2020
@jasonmit
Copy link
Collaborator

Part of v3.1.0, thanks

@jasonmit
Copy link
Collaborator

jasonmit commented May 19, 2020

@andrejcremoznik feel free to submit a PR. I'm fine with this PR going in as-is since it's simply adding behavior to toggle the addon on and off.

@gfmartinez
Copy link
Contributor Author

@jasonmit thanks for the merge. Glad to help out! Psyched for the changes.
@andrejcremoznik explicitly enabling or disabling gives the developer conditional control, whereas deriving use based on presence could allow for a failed build. I know several folks that use a .env file in their production build. If for some reason that file was missing in the build and they were expecting it to be there, the warnings would be helpful. If you go down that path, I'd just be sure to consider that use case, so that the app can Expect an .env file...

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.

3 participants