React: RFC: Throw in CommonJS entry points if NODE_ENV is not "development", "test", or "production"

[gaearon]

Whether it's the best way to do it or not, the fact is we rely on process.env.NODE_ENV checks to tell if we're in a production environment or not. This catches people by surprise because the default build of React is the development one, and so they often ship slow and large development bundles to production.

This gives an advantage to libraries that ship without a development mode at all. We could do this too. However, I think that this would be a net loss for the ecosystem. Having a way to differentiate between debug and release builds is possible on every native platform, and we should find a way to preserve this distinction on the web. The UX vs DX compromise is a false and unnecessary choice in this case.

Since React CommonJS entry points rely on the distinction, I propose that we don't allow any empty or custom NODE_ENV values. If NODE_ENV is nether "development", "test", or "production", we throw a hard error. This might not be the best solution in long term but at least it is consistent with how we are treating the flag now. We could then add React-specific aliases (REACT_ENV) for finer grained configuration or for people who use NODE_ENV for different purposes. Still, in my opinion, there should be no default: you need to pick the environment you’re working in.

The error would link to a page that provides an exhaustive list of how to configure NODE_ENV in different environments, as well as possible workarounds (use the right UMD build). For UMD builds, we will rename them to react.development.js and react.production.min.js so that the distinction is more useful. This is where people inexperienced with build systems start anyway so I don’t think we’ll hurt beginners.

I know this is probably going to be very controversial. I believe explicit opting into either mode is the right decision, but it will undoubtedly annoy people. What do you think?

Update: perhaps it is better to just go with REACT_ENV instead of NODE_ENV for everyone.

[acdlite]

I propose that we don't allow any empty or custom NODE_ENV values. If NODE_ENV is nether "development", "test", or "production", we throw a hard error.

I'm all for this 👍

[sergiodxa]

What if I use a staging enviroment? (Or some other value)

[trueadm]

@gaearon I think this change would help improve the understanding of the production and development builds of React. It's still far too common that "production" apps are shipped with development versions of React, that can impact performance.

[addyosmani]

Since React CommonJS entry points rely on the distinction, I propose that we don't allow any empty or custom NODE_ENV values. If NODE_ENV is nether "development", "test", or "production", we throw a hard error.

Having seen how many folks run into this problem daily (shipping slower DEV mode to production), I'm strongly supportive of throwing. It would also help with the issues we were seeing over in https://github.com/facebook/react/issues/8784.

[acdlite]

@sergiodxa

What if I use a staging enviroment? (Or some other value)

Maybe we could support a separate environment variable that takes precedence.

• If REACT_ENV is set, use that. Throw if invalid.
• Otherwise, fallback to NODE_ENV. Throw if undefined or invalid.

[ThugDebugger]

This catches people by surprise because the default build of React is the development one, and so they often ship slow and large development bundles to production.

This is very true, I can't begin to explain how many React apps I've encountered that are using the development rather than production build. Ultimately, enforcing a stronger build pipeline would be a headache in the beginning, but is needed for keeping everyone in line with shipping products

[myklt]

Fallback mechanism would be really appreciated in following order:

• REACT_NAV;
• NODE_ENV;
• production by default.

[dosaki]

I propose that we don't allow any empty or custom NODE_ENV values

What's the problem with custom NODE_ENV values?

[gaearon]

One problem with falling back to process.env.REACT_ENV is that everyone would have to specify it. Otherwise dead code stripping via Uglify wouldn’t work because it wouldn’t be able to turn it into expression known at compile time (basically same problem we have with NODE_ENV now 😄 ).

I don’t have a great solution to this except maybe https://github.com/facebook/fbjs/pull/86#issuecomment-285204734 but that would require folks to update Uglify which isn’t great.

[ljharb]

At compile time it'd be known to be undefined, which already works fine with NODE_ENV to eliminate production checks (when the env is unspecified). Why would it have to be specified?

[abritinthebay]

custom values should absolutely be supported - else React is being a bad Node citizen.

[gaearon]

At compile time it'd be known to be undefined, which already works fine with NODE_ENV to eliminate production checks (when the env is unspecified). Why would it have to be specified?

That’s not how Webpack works: it just leaves a polyfill with empty { env: {} } and doesn’t touch process.env.WHATEVER. Only if you use DefinePlugin explicitly will it replace those matches. Not sure how envify or others work.

[gaearon]

custom values should absolutely be supported - else React is being a bad Node citizen.

React is already being a “bad“ Node citizen because if you use a custom value, you get a slow version in production. So it’s punishing your users instead of you. At least punishing the developers seems fairer.

[trueadm]

I personally think we should drop NODE_ENV entirely going forward with the new React 16 and instead adopt REACT_ENV. This would throw for everyone currently, but at least the error would give context and they'd have to explicitly set REACT_ENV.

This would also give context to Webpack/Rollup/Browserify users who strip/replace the variable and use Uglify to remove the dead-code. As to why we can't really use a fallback variable with dead-code elimination, take the following code:

if (process.env.NODE_ENV === 'production' || process.env.REACT_ENV === 'production') {
  module.exports = require('./react.node-prod.min.js');
} else {
  module.exports = require('./react.node-dev.js');
}

Current webpack users who strip process.env.NODE_ENV, would get this:

if ('production' === 'production' || process.env.REACT_ENV === 'production') {
  module.exports = require('./react.node-prod.min.js');
} else {
  module.exports = require('./react.node-dev.js');
}

The above won't get stripped down by Uglify unless people additionally strip process.env.REACT_ENV. This wouldn't be apparent to them though and their bundle would include both the dev and prod code.

[gaearon]

Going 100% with REACT_ENV would also let us introduce a profiling environment without screwing up our __DEV__ checks that currently check NODE_ENV for being anything but "production".

[acdlite]

I have no idea how Webpack or Uglify works but seems like this would be stripped out by Uglify even if REACT_ENV is unknown because stripping out NODE_ENV results in an empty block.

if (process.env.REACT_ENV !== 'production') {
  if (process.env.NODE_ENV !== 'production') {
    // ...dev only code...
  }
}

Also on board with just going 100% REACT_ENV

[trueadm]

@acdlite In regards to the nested if statements, I'm guessing each else statement would also load the production code? In which case, wouldn't the production and dev code still get loaded because Uglify would not strip either if you were in development mode?

[acdlite]

@trueadm Yeah ignore that comment, didn't think it through :D

[ljharb]

@gaearon iirc envify just reifies every process.env value inline; I'm kind of shocked webpack doesn't do that.

[gaearon]

@ljharb Common Webpack configurations don’t really do envification at all, the usual suggested way is to use DefinePlugin which is basically a way to replace arbitrary globals. So it happens to work but is not env-specific which is why you bump into these things.

[abritinthebay]

At least punishing the developers seems fairer.

😂 A fair point. I do strongly prefer the REACT_ENV system (Babel uses a similar system) as long as it FIRST checks for NODE_ENV === "production" then falls back to that.

That seems the best of both worlds - you get to use the "standard" system if you're sensible and you get a warning/failure if you aren't.

[abritinthebay]

@ljharb WebPack has a plugin to do just that (like Envify) called inline-environment-variables-webpack-plugin on npm.

Like Browserify it's not done by default.

[gaearon]

Unfortunately I don't see a good way of getting dead code elimination work with a fallback (see above discussion). So either we use NODE_ENV or REACT_ENV. I think migrating fully to REACT_ENV makes sense in this case.

[abritinthebay]

if (process.env.REACT_ENV !== 'production' && process.env.NODE_ENV !== 'production') is the full check, surely? Doesn't have to be nested.

Unless I'm being dense and just need more coffee.

[trueadm]

@abritinthebay check my comment above for some reasoning: https://github.com/facebook/react/issues/9245#issuecomment-288813823

[abritinthebay]

Hmm... that seems to be an argument for catering to some users poor development practice (replacing just one env var like that is asking for problems).

I guess that's key here anyhow tho - trying to avoid people foot-gunning themselves. Not sure it's something React can take upon itself entirely (people will screw up REACT_ENV somehow too) but I get the desire.

REACT_ENV at least fixes some of the problems without causing other issues with NODE_ENV.

[petetnt]

What if you'd always get the production version if you didn't explicitly define NODE_ENV=development (or REACT_ENV=development)? People would be missing out on debug features, but for a first timer (or more advanced developer that's unaware of setting the NODE/REACT_ENV) at least the first experience wouldn't be nothing working at all.

It's not the best idea either but at least it wouldn't break every single tutorial and similar document that starts with npm install react react-dom or similar. 👀 It's a non-issue for the browser builds,

Going all the way with REACT_ENV would be even better and I think that going with _no custom NODE_ENV_s would end up in all kinds of weird edge cases (like say someone using "staging") like mentioned above.

[jamonholmgren]

A less disruptive option is to warn if REACT_ENV is not set and use some copy that compels action.

Warning: REACT_ENV not set. Your build will not be optimized for production.

[sunjay]

What if you'd always get the production version if you didn't explicitly define NODE_ENV=development (or REACT_ENV=development)? People would be missing out on debug features, but for a first timer (or more advanced developer that's unaware of setting the NODE/REACT_ENV) at least the first experience wouldn't be nothing working at all.

I think a lot of the development build parts are specifically for newer users to warn them of possible problems. Defaulting to production would mess up a lot of people's assumptions.

I'm a fan of REACT_ENV throwing with no fallback. As long as react follows semver when releasing, people shouldn't accidentally hit this problem in their existing projects.

[selbekk]

Great idea, but it would make it harder for beginners to get started. Yet another step in the build system tutorial of hell. When I started coding I had no idea what an environment variable even was.

[gaearon]

The alternative is that the beginners ship development code in production, and later find a library that doesn’t have the development mode at all, and their bundle becomes much smaller.

[petetnt]

Yet another idea would be rendering the warning directly to the view so it's impossible to miss (but at least the build works).

[cs-miller]

Seems like a great idea, as long as it is made clear how to set the environment variable.

[jaitaiwan]

A lot of people who get into react might not even know about environment variables and this means we have to teach them first before they can use react. Is it going to be easy to teach people to use ENV vars on any platform?

[arcanis]

I'm personally not too fond of the idea of requiring environment values for a JS library to work. It would require more work to start hacking with React from a blank project, which is not ideal for devs that just want to try the thing.

I like the idea of rendering something obvious, but it might be a bit too intrusif (no doubt it will eventually be deployed in prod from time to time), and it doesn't play well with the package separation (it would only work for react-dom).

Wouldn't a console.warn suffice? Each web dev working on a website always has their devtools open anyway, so they would quickly catch the warning.

[gaearon]

This is not requiring environment variables. These values are embedded at build time, and don't have to have any relationship to actual environment variables on your system (unless we are talking about server rendering). I think this discussion is just showing how much confusion there is about enabling the right behavior. 😔

I don't agree it will affect beginners that much. Beginners either use UMD browser bundles (which keep working) or Create React App (which requires no configuration). This would only affect people who have custom Webpack configs. But if they have a Webpack config they should learn that they need two: one for development and one for production.

[gaearon]

Oh, and of course I'm proposing an error with a link to a page clearly explaining how to fix the issue. So I don't think it's that big of a problem for a beginner to click that link and follow the instructions.

[StephenGrider]

Can't get behind this one, seems to fly in the face of the pit of success. Its just another configuration step that beginners have to deal with. Feels like a console.warn is the right approach here.

[jaitaiwan]

@gaearon I personally only heard about create-react-app after I'd been doing React dev using webpack for awhile. The majority of beginner resources I've read were effectively, here's webpack and here's some config that works. I'm worried that it could break beginners accidentally going through that.

Then again Webpack 2 has been pretty detrimental to those tutorials too...

[jaitaiwan]

Can you also explain:

This is not requiring environment variables

Are you saying env variables aren't necessarily part of the beginner discussion because of UMD or create react app?

[aweary]

Wouldn't a console.warn suffice? Each web dev working on a website always has their devtools open anyway, so they would quickly catch the warning.

Not everyone pays attention to the console, especially those developers like beginners for whom this is an issue.

I personally only heard about create-react-app after I'd been doing React dev using webpack for awhile.

@jaitaiwan create-react-app has only been around since July of last year and the installation documentation now mentions it right at the top. The landscape has likely changed a lot for beginners since you were one 😃

Are you saying env variables aren't necessarily part of the beginner discussion because of UMD or create react app?

See this section in @gaearon's initial post:

For UMD builds, we will rename them to react.development.js and react.production.min.js so that the distinction is more useful.

[gaearon]

The majority of beginner resources I've read were effectively, here's webpack and here's some config that works. I'm worried that it could break beginners accidentally going through that.

If there is a link with clear instructions on fixing the issue, is that a problem?

Are you saying env variables aren't necessarily part of the beginner discussion because of UMD or create react app?

I also mean that, contrary to what some comments in this thread seem to imply, you don’t need to set actual environment variables on your system. You only need to add two lines to Webpack config.

[jaitaiwan]

@gaearon Cool I'm on board now.

[aweary]

Can't get behind this one, seems to fly in the face of the pit of success. It's just another configuration step that beginners have to deal with.

@StephenGrider the problem is that the current setup is far from a pit of success. Right now it's too easy for users to run minified development builds in production, which severely hurts performance. I think "success" in this context isn't as simple as fewer configuration steps, it's also about running a performant build in production. Developers pay the cost of learning this requirement once and their users reap the rewards for the rest of the application's lifetime.

[jquense]

oof requiring one of 3 NODE_ENV values would break a lot of folks build flows :(

if we had to require something I agree with going full REACT_ENV, the ecosystem can adjust and the path there is fine it's another line in the webpack config. we should tell everyone to use @taion s copy of the dev-expression Babel plugin as well :P Bonus it doesn't hijack the dang NODE_ENV variable!

let's not make experienced folks lives more frustrating to make the beginner case easier :) (I'm sure we can meet both needs)

[jquense]

Side question. if we can't count on ppl looking at the console to see a warning, why do we think they are going to check there for an error? seems like just having react not work inexplicably isn't super beginner friendly either.

[jacobmischka]

I bring this up every time, but I think Vue does this right. Default to development, and always show a message about how to build for production (not just when minified without setting NODE_ENV).

[taion]

I really don't like this idea. Is there some idea here that React developers are especially in need of hand-holding? We don't see problems in practice with people accidentally shipping production apps without -O3 or with -g, as far as I'm aware.

Again, I really think this is a build-time thing, and one handled correctly by the latest webpack release by default, at that.

Maybe a better idea would be to split out a REACT_DEBUG setting from NODE_ENV, though, instead of using an unidiomatic enum for NODE_ENV. Have REACT_DEBUG control the specific debug instrumentation, while leaving NODE_ENV to control things like minifying debug messages. Frankly given the less-than-necessary support burden added to maintainers of third-party libs around things like the React 14 portal warnings, and the "unknown prop" warnings, it might be better to not enable specific debug instrumentation by default anyway.

[alonbardavid]

I think the whole issue is a bit of a vocal minority situation - the vast majority of react developers either work with production/development NODE_ENV or don't care about the performance or size of react.

But if somethings needs to change then I think it boils down to this:

• Forcing NODE_ENV to specific values will break current usage and it might require considerable code changes by the user (perhaps adding a new environment value for his custom staging enviornment and such and getting everything to use that)

• Changing the behavior to be production by default will probably cause more headache then what exists now - the node ecosystem is populated by a lot of code that relies on the fact that empty NODE_ENV != 'production' is development. It is against the path of least surprise.

• Changing to REACT_ENV is simple, and breaking changes are very easy to deal with. Inheriting the value from NODE_ENV can probably be done in such a way that most code optimizers can remove dead code.

• Choosing between production by default to development by default is really just prioritizing between whether developers without enough experience or support will have a harder time getting started with the react or will ship slower, bigger bundles to production.
  Personally, I vote for new developers to be able to open devtools, get type errors and such without having to mess with finding out how to change environment variables or banging their head on why their devtools is blank.
  Easy access is key for ecosystem growth and react already has a more complicated setup then other alternatives. Experienced devs will find the production flag easily enough and new developers are probably not very worried about build size and speed of their production.

Forcing people to choose REACT_ENV and not resulting to any defaults is the best option if something must change.

[alonbardavid]

To expand on the idea that this is a Vocal minority issue:

Every skeleton, boilerplate or guide discussing React uses NODE_ENV == production to determine if code should be minified.

So either you have your own weird setup, or you ship your code unminified to production.
If the latter, then react's size and speed is the least of your problems.
If the former then you should be able to handle adding a environment variable or using the minified/unminified version of react.

Where does this happen?

[successkrisz]

We shouldn't worry too much about people who want to develop on react yet can't be bothered to learn how to set an environment variable. Confusion is the worst, so I support the idea of throwing an error if NODE_ENV isn't set. I'm leaning towards using NODE_ENV rather having another one (REACT_ENV..)

[addyosmani]

I personally think we should drop NODE_ENV entirely going forward with the new React 16 and instead adopt REACT_ENV. This would throw for everyone currently, but at least the error would give context and they'd have to explicitly set REACT_ENV.

Reading through the different responses on this thread so far, it sounds like REACT_ENV would be the cleanest path forward messaging and breakage wise.

This would only affect people who have custom Webpack configs. But if they have a Webpack config they should learn that they need two: one for development and one for production.

💯

[taion]

REACT_ENV is overspecified if it's just a debug on/off toggle. Is React ever going to do something differently in test v. non-test environments, per this flag? I'd hope not – NODE_ENV seems like a better setting there, so why not just make it REACT_DEBUG?

IMO the precedents elsewhere of having -O0 be default, but -g not be default – these make sense.

[gaearon]

I've gone back and forth on this and currently I think it's too much churn for the ecosystem for little gain. Quoting myself from https://github.com/facebook/react/issues/6582#issuecomment-334012889:

I just feel like this ship has sailed. A ton of libraries in the React ecosystem already rely on NODE_ENV for better or worse, and changing this will involve a ton of churn. What’s worse, if we change it but people don’t update their configs, their code will silently fail to dead-code eliminate in popular bundlers like Webpack.

Also, as I mention in https://github.com/facebook/react/issues/7512#issuecomment-334137262 regarding staging environment:

React 16 ships with separate entry points for production CommonJS bundles in react/cjs/react.production.min.js and react-dom/cjs/react-dom.production.min.js. You can alias react and react-dom to these bundles to force a production environment even if NODE_ENV says otherwise.

So there is a workaround.