add webpack-env-define-plugin package #1030

meldiner · 2016-11-09T23:48:02Z

I found this code useful for our project configuration solution.
I didn't want to duplicate it so thought it could be wrapped in a handy webpack plugin.

facebook-github-bot · 2016-11-09T23:48:09Z

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!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

facebook-github-bot · 2016-11-10T00:02:34Z

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

camwest

Overall looks good. The show stopper is really the way that react-scripts is referencing the webpack plugin.

camwest · 2016-11-10T00:00:00Z

.gitignore

@@ -6,3 +6,4 @@ my-app*
 template/src/__tests__/__snapshots__/
 lerna-debug.log
 npm-debug.log
+.idea/


Add .idea to your global .gitignore not this one. See https://help.github.com/articles/ignoring-files/ for details

camwest · 2016-11-10T00:01:17Z

packages/react-scripts/config/webpack.config.prod.js

-// Assert this just to be safe.
-// Development builds of React are slow and not intended for production.
-if (env['process.env'].NODE_ENV !== '"production"') {
-  throw new Error('Production builds must have NODE_ENV=production.');


Why did you remove this throw?

I changed the default for NODE_ENV in webpack.config.prod.js to be production so the risk of running a production build with NODE_ENV !== 'production' is low.

I wouldn't say it's low. People try to run NODE_ENV=my-server npm run build all the time and get non-prod builds by mistake.

camwest · 2016-11-10T00:02:36Z

packages/react-scripts/package.json

@@ -64,6 +64,7 @@
    "url-loader": "0.5.7",
    "webpack": "1.13.2",
    "webpack-dev-server": "1.16.2",
+    "webpack-env-define-plugin": "../webpack-env-define-plugin",


Don't reference a local path. Refer to the package the way it would appear on npmjs.com

See https://github.com/lerna/lerna for more details on how these packages link together.

camwest · 2016-11-10T00:03:24Z

packages/webpack-env-define-plugin/package.json

@@ -0,0 +1,14 @@
+{


You should add yourself as a contributor to the package.json. See http://browsenpm.org/package.json#contributors for details.

gaearon · 2016-11-20T23:01:30Z

Ooh, I like this. But can we move this into react-dev-utils as a separate export, just like all the other custom plugins we have? This helps reduce devDependency bloat after ejecting.

gaearon

Let's move it into react-dev-utils.

meldiner · 2016-11-22T05:38:35Z

@gaearon done!

existentialism · 2016-11-23T20:06:06Z

packages/react-dev-utils/package.json

@@ -28,6 +29,7 @@
    "html-entities": "1.2.0",
    "opn": "4.0.2",
    "sockjs-client": "1.0.3",
-    "strip-ansi": "3.0.1"
+    "strip-ansi": "3.0.1",
+    "webpack": "^1.13.3"


There isn't an explicit webpack dependency to support both 1.x and 2.x.

See #963 and #950 (comment) for more info.

@gaearon what do you want to do regarding the webpack dependency?

You can remove it.

gaearon · 2016-12-05T22:32:22Z

packages/react-dev-utils/README.md

@@ -201,3 +201,35 @@ module.exports = {
  // ...
 }
 ```
+
+#### `new webpackEnvDefinePlugin({regex: string, customVariables: Object.<string, string>})`


Please remove webpack prefix.

gaearon · 2016-12-05T22:33:03Z

packages/react-dev-utils/README.md

+       regex: /^MY_PREFIX_/i,
+       customVariables: {
+         // Useful for determining whether we’re running in production mode.
+         // Most importantly, it switches React into the correct mode.


Both of these comments are a bit out of context (and the second one is actually misleading).
Can we come up with better realistic examples?

gaearon · 2016-12-05T22:35:49Z

packages/react-scripts/config/webpack.config.prod.js

-// Assert this just to be safe.
-// Development builds of React are slow and not intended for production.
-if (env['process.env'].NODE_ENV !== '"production"') {
-  throw new Error('Production builds must have NODE_ENV=production.');


I wouldn't say it's low. People try to run NODE_ENV=my-server npm run build all the time and get non-prod builds by mistake.

gaearon · 2016-12-05T22:36:34Z

packages/react-scripts/config/webpack.config.dev.js

+        // Useful for determining whether we’re running in production mode.
+        // Most importantly, it switches React into the correct mode.
+        'NODE_ENV': JSON.stringify(
+          process.env.NODE_ENV || 'development'


We don't need to read process.env.NODE_ENV here at all. We don't allow overriding it. So we can just pass 'development'.

gaearon · 2016-12-05T22:36:51Z

packages/react-scripts/config/webpack.config.prod.js

+        // Useful for determining whether we’re running in production mode.
+        // Most importantly, it switches React into the correct mode.
+        'NODE_ENV': JSON.stringify(
+          process.env.NODE_ENV || 'production'


Same here. Just pass 'production'?

You'd need to update comments to make sense after the changes.

meldiner · 2016-12-07T00:19:31Z

@gaearon I fixed the issues you referred to but removing the webpack dependency caused a build error.

~~Should I add webpack as a peer dependency to react-dev-utils?~~ - doesn't fix the build

Should I remove the webpack dependency from the EnvDefinePlugin code altogether?
By making EnvDefinePluginreturn a DefinePlugin json configuration and wrapping it with a DefinePlugin instance only in the react-scripts package (that already includes webpack as a dependency) I can remove webpack from being a react-dev-utils dependency.

meldiner · 2016-12-26T06:30:47Z

@gaearon, removed the webpack.DefinePlugin dependency

gaearon · 2017-02-24T18:56:00Z

Hey, sorry for the trouble. I think we won't go ahead with this after all. 😞

We've been changing how env variables work recently (since they're also useful in index.html) and we might change that code later again. So it's premature to abstract this away.

I'm sorry I didn't realize this earlier.

facebook-github-bot added the CLA Signed label Nov 9, 2016

camwest suggested changes Nov 10, 2016

View reviewed changes

add webpack-env-define-plugin

9ef53fd

meldiner force-pushed the master branch from 7bf7c06 to 9ef53fd Compare November 10, 2016 00:30

add missing npm link to webpack-env-define-plugin

863e640

gaearon suggested changes Nov 20, 2016

View reviewed changes

move webpackEnvDefinePlugin to react-dev-utils

35fbc21

existentialism reviewed Nov 23, 2016

View reviewed changes

gaearon suggested changes Dec 5, 2016

View reviewed changes

fix documentation and NODE_ENV values in dev and prod

7962c10

remove webpack DefinePlugin dependency

e82dce4

gaearon closed this Feb 24, 2017

lock bot locked and limited conversation to collaborators Jan 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add webpack-env-define-plugin package #1030

add webpack-env-define-plugin package #1030

meldiner commented Nov 9, 2016 •

edited

Loading

facebook-github-bot commented Nov 9, 2016

facebook-github-bot commented Nov 10, 2016

camwest left a comment

camwest Nov 10, 2016

camwest Nov 10, 2016

meldiner Nov 10, 2016

gaearon Dec 5, 2016

camwest Nov 10, 2016

camwest Nov 10, 2016

meldiner Nov 10, 2016

gaearon commented Nov 20, 2016

gaearon left a comment

meldiner commented Nov 22, 2016

existentialism Nov 23, 2016

meldiner Dec 3, 2016

gaearon Dec 5, 2016

gaearon Dec 5, 2016

gaearon Dec 5, 2016

gaearon Dec 5, 2016

gaearon Dec 5, 2016

gaearon Dec 5, 2016

gaearon Dec 5, 2016

meldiner commented Dec 7, 2016 •

edited

Loading

meldiner commented Dec 26, 2016

gaearon commented Feb 24, 2017

add webpack-env-define-plugin package #1030

add webpack-env-define-plugin package #1030

Conversation

meldiner commented Nov 9, 2016 • edited Loading

facebook-github-bot commented Nov 9, 2016

facebook-github-bot commented Nov 10, 2016

camwest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Nov 20, 2016

gaearon left a comment

Choose a reason for hiding this comment

meldiner commented Nov 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meldiner commented Dec 7, 2016 • edited Loading

meldiner commented Dec 26, 2016

gaearon commented Feb 24, 2017

meldiner commented Nov 9, 2016 •

edited

Loading

meldiner commented Dec 7, 2016 •

edited

Loading