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 dotenv support. #72

Merged
merged 3 commits into from Nov 27, 2019
Merged

Add dotenv support. #72

merged 3 commits into from Nov 27, 2019

Conversation

@XhmikosR
Copy link
Contributor

XhmikosR commented Oct 24, 2019

Fixes #52.

Now one can use a .env file in the project root with the needed environment variables. If the environment variables are already set, they take precedence.

Opening for discussion. It does seem to work fine so far for me locally.

@XhmikosR XhmikosR force-pushed the XhmikosR:dotenv branch 3 times, most recently from 48cd49d to 3fcb462 Oct 25, 2019
@emedvedev

This comment has been minimized.

Copy link
Owner

emedvedev commented Oct 28, 2019

I think it's fine, README just needs to be updated.

@XhmikosR

This comment has been minimized.

Copy link
Contributor Author

XhmikosR commented Oct 28, 2019

I don't think this is ready in the sense, should it throw? Does it throw? Can we simplify npm start now?

And should probably go along with #80 so that the vars are loaded in the API too and not only through the bin script.

Maybe we should do this in steps, though.

@emedvedev

This comment has been minimized.

Copy link
Owner

emedvedev commented Oct 28, 2019

I don't think it should throw (should definitely log when something is overwritten though), but npm start and the bin script will have to be updated to allow launching the script without positional arguments.

@XhmikosR

This comment has been minimized.

Copy link
Contributor Author

XhmikosR commented Oct 28, 2019

IIRC dotenv does not override any already set variables, so nothing to worry there.

@emedvedev

This comment has been minimized.

Copy link
Owner

emedvedev commented Oct 28, 2019

I mean the reverse: when already set env variables override the ones specified in .env. If being explicit about it is possible, I think that might reduce confusion in some cases where people could expect .env to take precedence.

@XhmikosR

This comment has been minimized.

Copy link
Contributor Author

XhmikosR commented Oct 28, 2019

https://www.npmjs.com/package/dotenv#what-happens-to-environment-variables-that-were-already-set

But I don't think we should worry about this. People should use one way not mix and match.

Let's leave this change on hold for now. I'll try to move the config in the API later today.

@emedvedev

This comment has been minimized.

Copy link
Owner

emedvedev commented Nov 23, 2019

We should probably mention this in README, and then it's fine to merge, imo.

Now one can use a .env file in the project root with the needed environment variables.
If the environment variables are already set, they take precedence.
emedvedev added 2 commits Nov 27, 2019
@emedvedev emedvedev marked this pull request as ready for review Nov 27, 2019
@emedvedev emedvedev merged commit 287cd1a into emedvedev:master Nov 27, 2019
4 checks passed
4 checks passed
Node 10
Details
Node 12
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
now Deployment has completed
Details
@XhmikosR XhmikosR deleted the XhmikosR:dotenv branch Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.