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

Adding namespaced environment variables to DefinePlugin under REACT_APP_ #342

Merged
merged 1 commit into from
Aug 3, 2016

Conversation

eliperelman
Copy link
Contributor

This enables users to specify custom environment variables under a REACT_APP_* namespace. To verify it works, I ran the start script via:

REACT_APP_TITLE="Env-enabled React" npm start

And changed the <h2> in src/App.js:

<h2>Welcome to {process.env.REACT_APP_TITLE}</h2>

Which rendered:

screenshot

@ghost
Copy link

ghost commented Aug 3, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@eliperelman
Copy link
Contributor Author

This would resolve #102.

@ghost
Copy link

ghost commented Aug 3, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

var REACT_APP = /^REACT_APP_/i;

module.exports = Object
.keys(process.env)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: we use two spaces for indenting these

@gaearon
Copy link
Contributor

gaearon commented Aug 3, 2016

Looks great and I like the prefix choice. Happy to get this in after resolving minor style nitpicks. Thanks for contributing!

@eliperelman
Copy link
Contributor Author

Gah, not sure how the 4 spaces got in there. I'll fix and push.

env['process.env.' + key] = JSON.stringify(process.env[key]);
return env;
}, {
'process.env.NODE_ENV': NODE_ENV
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand correctly that this works in prod because we explicitly reset it as part of build script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. It is development by default, and it has been explicitly overridden in config.prod and the top of scripts/test.

@gaearon
Copy link
Contributor

gaearon commented Aug 3, 2016

I would appreciate if you could also add a section on this to "how to" list in template/README.md.

It would need to have a disclaimer that this is a new feature since 0.3.0 (you can find similar disclaimers but searching for "0.2.0" in that file).

@eliperelman eliperelman force-pushed the custom-env-vars branch 2 times, most recently from 9f28825 to 14b6597 Compare August 3, 2016 13:45
@ghost ghost added the CLA Signed label Aug 3, 2016
@@ -404,6 +405,44 @@ Note that GitHub Pages doesn't support routers that use the HTML5 `pushState` hi

Use the [Heroku Buildpack for create-react-app](https://github.com/mars/create-react-app-buildpack).

### Adding Custom Environment Variables
Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s either change this to Add or change all other heading to remove ing (I think that actually might be the way to go). You can do this in a separate PR if you’d like.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s also move this section before Deploy, I’d like to keep it the last one.

@eliperelman eliperelman force-pushed the custom-env-vars branch 5 times, most recently from 2cfe323 to 910ae34 Compare August 3, 2016 15:29
@eliperelman
Copy link
Contributor Author

Thanks for the feedback! Updates have been made.

@ghost ghost added the CLA Signed label Aug 3, 2016
set REACT_APP_SECRET_CODE=abcdef

# Create an environment variable on macOS/Linux/Unix
REACT_APP_SECRET_CODE=abcdef
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a shell expert, but will this work without export? http://stackoverflow.com/a/1158231

Copy link
Contributor

@gaearon gaearon Aug 3, 2016

Choose a reason for hiding this comment

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

Maybe we can just show two commands:

Windows

set REACT_APP_SECRET_CODE=abcdef&npm start

Bash (macOS, Linux)

REACT_APP_SECRET_CODE=abcdef npm start

Copy link
Contributor

Choose a reason for hiding this comment

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

(Also let’s keep the commands for different systems in separate blocks)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I probably won't use the words "Bash" since bash is becoming a thing on Windows. But yes, I'll separate the blocks.

@alexzherdev yes, just like @gaearon's snippet between us, it doesn't use export to set the variable. Export is used for permanence of variables, but as I mentioned that's outside the scope of this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn’t Bash on windows work with Bash syntax? Just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I suppose that's right 😜

I'll think of something and see what everyone thinks.

@ghost ghost added the CLA Signed label Aug 3, 2016
@eliperelman eliperelman force-pushed the custom-env-vars branch 3 times, most recently from 6154653 to 76f7541 Compare August 3, 2016 19:44
@ghost ghost added the CLA Signed label Aug 3, 2016
@gaearon gaearon added this to the 0.3.0 milestone Aug 3, 2016
@gaearon gaearon merged commit 8f59cad into facebook:master Aug 3, 2016
@gaearon
Copy link
Contributor

gaearon commented Aug 3, 2016

Thanks!

@thg303
Copy link

thg303 commented Oct 26, 2016

Does not work for setting "port" in version "0.5.0", instead of setting port this way, user has to create a .env file and set the port there like:
PORT=80

@gaearon
Copy link
Contributor

gaearon commented Feb 24, 2017

Does not work for setting "port" in version "0.5.0", instead of setting port this way, user has to create a .env file and set the port there like:

The feature in this PR is only for custom variables. There are a few built-in ones documented here, and they don't need the prefix.

@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants