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

Load environment file in development #695

Merged
merged 3 commits into from
Sep 23, 2016

Conversation

ayrton
Copy link
Contributor

@ayrton ayrton commented Sep 21, 2016

Adds support to load environment variables via a .env file for development using the very stable and battle tested ⚔ npm package dotenv. For more context see #687.

I've tested this out by doing the following:

npm run create-react-app my-app
cd my-app
echo "NODE_PATH=." >> .env
npm start

In my-app/src/index.js I changed import App from './App'; to import App from 'src/App'; and verified all still works (including tests)

@ghost
Copy link

ghost commented Sep 21, 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!

@ghost ghost added the CLA Signed label Sep 21, 2016
@ghost
Copy link

ghost commented Sep 21, 2016

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

@@ -434,7 +434,7 @@ REACT_APP_SECRET_CODE=abcdef npm start
```

> Note: Defining environment variables in this manner is temporary for the life of the shell session. Setting
permanent environment variables is outside the scope of these docs.
permanent environment variables in development can be done in a .env file in the root of your project.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should link to dotenv documentation.
Also please use backticks for .env so it’s highlighted.

// Load environment variables from .env file. Surpress warnings using silent
// if this file is missing. dotenv will never modify any environment variables
// that have already been set.
require('dotenv').config({silent: true})
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: let’s put a semicolon here (in all files).

@@ -40,6 +40,7 @@
"cross-spawn": "4.0.0",
"css-loader": "0.24.0",
"detect-port": "1.0.0",
"dotenv": "^2.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please keep the version pinned (2.0.0). We’re a bit more on the conservative side about updates.

@gaearon gaearon added this to the 0.5.0 milestone Sep 22, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 22, 2016

Overall this looks good to me and I’m OK with merging this after nits are addressed.
Thanks!

@ayrton
Copy link
Contributor Author

ayrton commented Sep 22, 2016

@gaearon awesome. I've rebased and addressed your feedback

@ghost ghost added the CLA Signed label Sep 22, 2016
@jhorneman
Copy link
Contributor

jhorneman commented Sep 22, 2016

(As mentioned on Twitter, writing it here at @ayrton's request.)

Heroku uses a local .env file for its toolchain, to allow local testing before pushing to their servers. See https://devcenter.heroku.com/articles/heroku-local#copy-heroku-config-vars-to-your-local-env-file. Would this use of .env clash with that? They theoretically have the same purpose: in practice perhaps they represent different use cases.

(I can't comment or defend this beyond this link, because I don't use JavaScript with Heroku. I'm just bringing it up in case people didn't know Heroku's toolchain did this.)

Update: I just realized that if I were to use React in the JavaScript part of my mainly-Python app, and I were use to create-react-app, I'd be upset if:

  1. my JS code were to behave oddly due to the .env file I set up so I can locally test my Python app on Heroku. (I'm assuming here create-react-app would never write to the .env file: that'd be catastrophic since it's not under revision control.)
  2. I couldn't have create-react-app AND local Heroku because I'd need two different .env files.

@ayrton
Copy link
Contributor Author

ayrton commented Sep 22, 2016

@jhorneman good question, this use case would go as follows:

npm run build
cd build/
heroku config:get CONFIG-VAR-NAME -s  >> .env
foreman start

The copied environment variables from heroku will live inside the build/ file (not inside the root of the project). The environment variables defined in the root .env are the ones used to build the actual project. So I don't believe it's an issue.

@gaearon
Copy link
Contributor

gaearon commented Sep 22, 2016

I would like to get feedback from @mars before we merge this.

@jhorneman
Copy link
Contributor

@ayrton: Thanks for replying. I honestly can't say if my case is an edge case, so I apologize if I sound pedantic, but I have an existing Python-and-some-JS Heroku app with a .env in the root of the project, which is for the Python part. The solution you propose wouldn't work for me (without rearranging the entire project), but perhaps I could (or should?) run create-react-app in a subfolder?

I guess if there are no catastrophic consequences from people doing this "wrong", an FAQ entry would suffice.

@ayrton
Copy link
Contributor Author

ayrton commented Sep 22, 2016

@gaearon I think that's a good idea, I've been using CRA together with the create-react-app-buildpack to deploy on heroku. What's imo key to dotenv is the following:

We will never modify any environment variables that have already been set. In particular, if there is a variable in your .env file which collides with one that already exists in your environment, then that variable will be skipped. This behavior allows you to override all .env configurations with a machine-specific environment, although it is not recommended.

Meaning if somehow the .env was to land on the server/CI to create a production build that it will never collide with previously set environment variables.

@gaearon
Copy link
Contributor

gaearon commented Sep 22, 2016

Can you write a more comprehensive Usage Guide entry that explains env variables more holistically? Like, what you’d normally do in development, and what you’d normally do in production.

@@ -9,6 +9,11 @@

process.env.NODE_ENV = 'test';

// Load environment variables from .env file. Surpress warnings using silent
// if this file is missing. dotenv will never modify any environment variables
// that have already been set.
Copy link
Contributor

Choose a reason for hiding this comment

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

It’s a good idea to link to dotenv docs from this comment.

@ghost ghost added the CLA Signed label Sep 22, 2016
@ayrton
Copy link
Contributor Author

ayrton commented Sep 22, 2016

I'll try to write some more complete documentation tonight

@mars
Copy link
Contributor

mars commented Sep 22, 2016

As a Heroku dev who's synced up local runtime with deployments many times, this dotenv feature is implemented perfectly ✅

That said, please allow me to call attention to an existing issue with CRA env vars that impact how the app works on Heroku: mars/create-react-app-buildpack#7

TL;DR these vars are compiled into the bundle, which means the build is not necessarily stateless. This can result in multi-stage deployments (pipelines) having stale values for env vars.

Please be careful/aware of this snag and voice you opinions/concerns in that Github issue.

// Load environment variables from .env file. Surpress warnings using silent
// if this file is missing. dotenv will never modify any environment variables
// that have already been set.
require('dotenv').config({silent: true});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be moved to packages/react-scripts/config/env.js? That's the only place where these environment variables would be used anyway and then we don't need this in each script separately?

Copy link
Contributor Author

@ayrton ayrton Sep 22, 2016

Choose a reason for hiding this comment

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

I liked this idea, but I can't get it to work (using NODE_PATH). I think somehow that config/env.js is not loaded or not loaded early enough:

diff --git a/packages/react-scripts/config/env.js b/packages/react-scripts/config/env.js
index 66acf11..08faa82 100644
--- a/packages/react-scripts/config/env.js
+++ b/packages/react-scripts/config/env.js
@@ -14,2 +14,8 @@

+// Load environment variables from .env file. Surpress warnings using silent
+// if this file is missing. dotenv will never modify any environment variables
+// that have already been set.
+// https://github.com/motdotla/dotenv
+require('dotenv').config({silent: true});
+
 var REACT_APP = /^REACT_APP_/i;
diff --git a/packages/react-scripts/config/webpack.config.dev.js b/packages/react-scripts/config/webpack.config.dev.js
index 8d1de1c..5596de6 100644
--- a/packages/react-scripts/config/webpack.config.dev.js
+++ b/packages/react-scripts/config/webpack.config.dev.js
@@ -11,2 +11,3 @@

+var env = require('./env');
 var path = require('path');
@@ -18,3 +19,2 @@ var WatchMissingNodeModulesPlugin = require('../scripts/utils/WatchMissingNodeMo
 var paths = require('./paths');
-var env = require('./env');

diff --git a/packages/react-scripts/config/webpack.config.prod.js b/packages/react-scripts/config/webpack.config.prod.js
index 5790e8e..5c793c8 100644
--- a/packages/react-scripts/config/webpack.config.prod.js
+++ b/packages/react-scripts/config/webpack.config.prod.js
@@ -11,2 +11,3 @@

+var env = require('./env');
 var path = require('path');
@@ -18,3 +19,2 @@ var url = require('url');
 var paths = require('./paths');
-var env = require('./env');

diff --git a/packages/react-scripts/scripts/build.js b/packages/react-scripts/scripts/build.js
index 7d87acc..71dcc79 100644
--- a/packages/react-scripts/scripts/build.js
+++ b/packages/react-scripts/scripts/build.js
@@ -14,8 +14,2 @@ process.env.NODE_ENV = 'production';

-// Load environment variables from .env file. Surpress warnings using silent
-// if this file is missing. dotenv will never modify any environment variables
-// that have already been set.
-// https://github.com/motdotla/dotenv
-require('dotenv').config({silent: true});
-
 var chalk = require('chalk');
diff --git a/packages/react-scripts/config/webpack.config.prod.js b/packages/react-scripts/config/webpack.config.prod.js
index 5790e8e..5c793c8 100644
--- a/packages/react-scripts/config/webpack.config.prod.js
+++ b/packages/react-scripts/config/webpack.config.prod.js
@@ -11,2 +11,3 @@

+var env = require('./env');
 var path = require('path');
@@ -18,3 +19,2 @@ var url = require('url');
 var paths = require('./paths');
-var env = require('./env');

diff --git a/packages/react-scripts/scripts/build.js b/packages/react-scripts/scripts/build.js
index 7d87acc..71dcc79 100644
--- a/packages/react-scripts/scripts/build.js
+++ b/packages/react-scripts/scripts/build.js
@@ -14,8 +14,2 @@ process.env.NODE_ENV = 'production';

-// Load environment variables from .env file. Surpress warnings using silent
-// if this file is missing. dotenv will never modify any environment variables
-// that have already been set.
-// https://github.com/motdotla/dotenv
-require('dotenv').config({silent: true});
-
 var chalk = require('chalk');
diff --git a/packages/react-scripts/scripts/start.js b/packages/react-scripts/scripts/start.js
index a82e68d..b0290bf 100644
--- a/packages/react-scripts/scripts/start.js
+++ b/packages/react-scripts/scripts/start.js
@@ -13,8 +13,2 @@ process.env.NODE_ENV = 'development';

-// Load environment variables from .env file. Surpress warnings using silent
-// if this file is missing. dotenv will never modify any environment variables
-// that have already been set.
-// https://github.com/motdotla/dotenv
-require('dotenv').config({silent: true});
-
 var path = require('path');
diff --git a/packages/react-scripts/scripts/test.js b/packages/react-scripts/scripts/test.js
index 0c123ec..d13aff3 100644
--- a/packages/react-scripts/scripts/test.js
+++ b/packages/react-scripts/scripts/test.js
@@ -11,8 +11,2 @@ process.env.NODE_ENV = 'test';

-// Load environment variables from .env file. Surpress warnings using silent
-// if this file is missing. dotenv will never modify any environment variables
-// that have already been set.
-// https://github.com/motdotla/dotenv
-require('dotenv').config({silent: true});
-
 const createJestConfig = require('./utils/createJestConfig');

Copy link
Contributor

Choose a reason for hiding this comment

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

but I can't get it to work (using NODE_PATH)

I thought this would only be used to specify the REACT_APP_* environment variables that we officially support? But there's some other use case for this that I'm missing? Can you clarify what you mean by "using NODE_PATH"?

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 think there are use cases for this PR outside the REACT_APP_* usecases eg. absolute imports.

By setting NODE_PATH=. I can start using absolute imports in my codebase:

Relative:

import Button from '../../shared/button';

vs.

import Button from 'src/shared/button';

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that absolute imports aren't officially supported in create react app as of now, so even if this environment variable might work, there is no guarantees of it continuing to be supported in the future. Therefore I wouldn't encourage using it until we add official support for absolute imports.

With this in mind, I think it's fine if the environment only works for REACT_APP_* variables – nothing else is officially supported right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually NODE_PATH is already supported since 0.4.0 (#476), sorry I was wrong about this 😊

@juanpaco
Copy link

juanpaco commented Sep 22, 2016

I ❤️ dotenv. I was on a CRA project that recently ejected so that we could use it, so this is most welcome news!

This might be less of a concern for a UI that is completely separated from its server side the way a CRA app is, but on a development machine, I generally have to have a .env.development and a .env.test. If it's all in one file, then running tests with, say, a different API endpoint could be tricky.

I had a discussion with the dotenv folks about this here: motdotla/dotenv#150

so having a separate test and development file is legitimate in their eyes. How to make that easy for CRA users, I don't know right now.

I think that having the .env at all is a step up from not having it, but I would further ❤️ being able to have different values for both test and development.

@gaearon
Copy link
Contributor

gaearon commented Sep 22, 2016

I just merged a PR that rename env.js and moves it to scripts/utils/getClientEnvironment so this would need to be rebased. Thanks!

@benpickles
Copy link
Contributor

I use https://github.com/direnv/direnv which keeps environment variables in the environment and removes the need for an app dependency (in any language).

@ayrton
Copy link
Contributor Author

ayrton commented Sep 22, 2016

@gaearon added more documentation + rebased

@ghost ghost added the CLA Signed label Sep 22, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 22, 2016

@ayrton Can you please address the previous comment and explain the choice of dotenv over direnv?

@ayrton
Copy link
Contributor Author

ayrton commented Sep 22, 2016

@benpickles I like the idea direnv but I don't think it's a perfect fit for CRA. You can't expect every user that wants to leverage environment variables to install an external dependency.

Afaik the goal of CRA is to provide an easy-to-use experience to get started with React with sensible defaults. Accepting this PR's approach means there is zero added friction for the end user to start using environment variables (if they wish) in their application (and following best practices as specified in the Twelve-Factor App methodology.

@ghost ghost added the CLA Signed label Sep 22, 2016
@ayrton
Copy link
Contributor Author

ayrton commented Sep 22, 2016

@juanpaco the docs in dotenv do not recommend this:

We strongly recommend against having a "main" .env file and an "environment" .env file like .env.test. Your config should vary between deploys, and you should not be sharing values between environments.

@ghost ghost added the CLA Signed label Sep 22, 2016
@juanpaco
Copy link

@ayrton If you check out the thread I linked to, I asked them about it. Their concern was about having a "main" .env file that then gets a few things overridden with with env-specific values. Having complete sets is okay though. I found that the way the FAQ was written didn't really convey that point, so I asked them for clarification in that issue I linked to.

@ghost ghost added the CLA Signed label Sep 22, 2016
@mars
Copy link
Contributor

mars commented Sep 23, 2016

@ayrton CRA's use of env vars does not honor 12-factor, because the vars are compiled into the bundle. To honor 12-f, the env vars must be read at runtime. See: mars/create-react-app-buildpack#7

@ghost ghost added the CLA Signed label Sep 23, 2016
@ayrton
Copy link
Contributor Author

ayrton commented Sep 23, 2016

@mars I never experienced it any other way where environment variables are not compiled into the bundle for JS apps. I totally applaud the effort for experimentation in mars/create-react-app-buildpack#7 but I don't see how this specific PR could address this right now

@ayrton
Copy link
Contributor Author

ayrton commented Sep 23, 2016

@gaearon what do we need to do to proceed here or did we hit a roadblock? I can see there are still open questions (for edge cases) which this PR doesn't address, but this was honestly never intended to fix those yet. I think this PR gets 99% of the people up and running to use environment variables on development (while not being an annoyance to the people who don't use them).

If this is going nowhere I'm happy to close for now.

@ghost ghost added the CLA Signed label Sep 23, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 23, 2016

No roadblocks, this looks good to me. Can you please remove unrelated spacing changes in PR? Space is significant in Markdown and is used to break likes in those places.

@ayrton
Copy link
Contributor Author

ayrton commented Sep 23, 2016

Wasn't aware of that 😄 (TIL) - Done!

@gaearon gaearon merged commit 8e5183a into facebook:master Sep 23, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 23, 2016

Thanks!

@ayrton ayrton deleted the environment-variables branch September 23, 2016 12:19
@gaearon
Copy link
Contributor

gaearon commented Sep 23, 2016

0.5.0 has been released with this feature.

feiqitian pushed a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
* Load environment file via dotenv if .env file is present

* Document loading environment variables in .env file

* Minor doc tweaks
@lock lock bot locked and limited conversation to collaborators Jan 22, 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.

8 participants