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

Change in installation cmd & added a project to other projects list #288

Merged
merged 4 commits into from Mar 26, 2018

Conversation

yTakkar
Copy link
Contributor

@yTakkar yTakkar commented Mar 12, 2018

No description provided.

@coveralls
Copy link

coveralls commented Mar 12, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling b4277cf on yTakkar:readme-change into ad86907 on motdotla:master.

README.md Outdated
@@ -13,7 +13,11 @@ Dotenv is a zero-dependency module that loads environment variables from a `.env
## Install

```bash
npm install dotenv --save
# with npm
npm i dotenv
Copy link
Contributor

Choose a reason for hiding this comment

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

does npm save by default now? if not, should keep --save flag. I also have a strong preference for install over i for documentation for anyone not familiar with shorthand

Copy link

Choose a reason for hiding this comment

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

Yep, npm saves by default now. And yeah I agree with @maxbeatty that the explicit long-from command names are better for documentation

Copy link
Contributor Author

@yTakkar yTakkar Mar 13, 2018

Choose a reason for hiding this comment

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

Yes npm saves to package.json by default now. And I too agree now with @maxbeatty as install command is better for documentation and i might confuse any beginner.
Replaced it :)

README.md Outdated
@@ -255,3 +259,4 @@ Here's some projects that expand on dotenv. Check them out.
* [lookenv](https://github.com/RodrigoEspinosa/lookenv)
* [run.env](https://www.npmjs.com/package/run.env)
* [dotenv-webpack](https://github.com/mrsteele/dotenv-webpack)
* [Handy-Browser-Env](https://github.com/yTakkar/Handy-Browser-Env)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain how your module expands dotenv and encourages storing configuration in the environment separate from code?

Copy link
Contributor Author

@yTakkar yTakkar Mar 13, 2018

Choose a reason for hiding this comment

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

When I was developing React-Instagram-Clone-2.0, I used dotenv, it worked like a charm on nodejs as I can access env variables very easily. But there was no package which allowed me to access those env variables on the front-side of the app, so I developed Handy-Browser-Env.
It takes help of dotenv's parse method.

Copy link
Contributor

Choose a reason for hiding this comment

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

A key distinction between server-side config and client-side config is that nothing sensitive can go into client-side config because you're distributing it to everyone (no matter how uglified it is, it's still being distributed). Config may still change between deploys for client-side apps but you're better off using JavaScript (or the language of your choice) directly to decide on that config, and it can be stored in git (or your vcs of choice) because, again, this configuration isn't sensitive.

I want to make sure this distinction is clear to developers. I see a lot of issues related to using dotenv in the browser and don't completely understand it. Adding a link to this module won't discourage using dotenv for browser-based projects. If your module helps you, keep using it. I'm going to request that this link be removed so that there's no added confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll remove it if you want :)

@maxbeatty maxbeatty merged commit 1945a05 into motdotla:master Mar 26, 2018
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

4 participants