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

Added environment variables #67

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@jtj9817
Copy link

commented Nov 19, 2018

Added config_vars.env for the production-stage front-end component of NodeChat. PR fix for #52

Added environment variables
Added config_vars.env for the production-stage front-end component of NodeChat.
@@ -0,0 +1 @@
LOCAL_HOST_FRONTEND = 'http://localhost:9876'

This comment has been minimized.

Copy link
@joshghent

joshghent Nov 19, 2018

Collaborator

Let's rename this to be "PORT" and call this file "sample.env"

This comment has been minimized.

Copy link
@joshghent

joshghent Nov 19, 2018

Collaborator

Create another variable called "URL"

This comment has been minimized.

Copy link
@jtj9817

jtj9817 Nov 20, 2018

Author

I can rename it to 'REACT_APP_PORT' based on @dnguneratne 's suggestions on using environment variables for a React app. However, I'm not so sure if "sample.env" is a good name for it, "config_vars" seems more appropriate because it's where we store environment variables that we use for configuration in the project.

@@ -8,7 +8,7 @@ class LoginPage extends Component {
constructor(props) {
super(props);

this.socket = io('http://localhost:9876');
this.socket = io(process.env.LOCAL_HOST_FRONTEND);

This comment has been minimized.

Copy link
@joshghent

joshghent Nov 19, 2018

Collaborator

Please default the url and port to localhost:9876 or throw an error if we do not have this config item passed

@@ -17,6 +17,7 @@
"url": "git@github.com:joshghent/NodeChat.git"
},
"dependencies": {
"dotenv": "^6.1.0",

This comment has been minimized.

Copy link
@dnguneratne

This comment has been minimized.

Copy link
@jtj9817

jtj9817 Nov 20, 2018

Author

Just quickly skimmed that doc. So based on understanding, I just have to rename the variables with a prefix of 'REACT_APP' and I can remove the dotenv package? Once I get confirmation on this, I'll make the necessary changes accordingly.

This comment has been minimized.

Copy link
@dnguneratne

dnguneratne Nov 20, 2018

Contributor

Yes, that's right. You'll also need to rename config_vars.env file to .env.

The environment variable can be accessed like this: process.env.REACT_APP_[variable_name].

This comment has been minimized.

Copy link
@joshghent

joshghent Nov 20, 2018

Collaborator

Nah .env should be gitignored. .env has the personal tokens and private stuff. That is why you need to provide a sample.env that is then duplicated and renamed to .env

This comment has been minimized.

Copy link
@dnguneratne

dnguneratne Nov 20, 2018

Contributor

Normally I'd agree, but React docs recommend otherwise.

react_add_env_usage

@joshghent
Copy link
Collaborator

left a comment

Could you please fix up the environment variable names please? And add the .env file to the gitignore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.