-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Conversation
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 [email protected]. Thanks! |
This would resolve #102. |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
02519ed
to
2a51266
Compare
var REACT_APP = /^REACT_APP_/i; | ||
|
||
module.exports = Object | ||
.keys(process.env) |
There was a problem hiding this comment.
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
Looks great and I like the prefix choice. Happy to get this in after resolving minor style nitpicks. Thanks for contributing! |
Gah, not sure how the 4 spaces got in there. I'll fix and push. |
2a51266
to
7aad7b3
Compare
env['process.env.' + key] = JSON.stringify(process.env[key]); | ||
return env; | ||
}, { | ||
'process.env.NODE_ENV': NODE_ENV |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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). |
9f28825
to
14b6597
Compare
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
2cfe323
to
910ae34
Compare
Thanks for the feedback! Updates have been made. |
set REACT_APP_SECRET_CODE=abcdef | ||
|
||
# Create an environment variable on macOS/Linux/Unix | ||
REACT_APP_SECRET_CODE=abcdef |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
6154653
to
76f7541
Compare
76f7541
to
2507eef
Compare
Thanks! |
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. |
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:Which rendered: