facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.84k stars 46.51k forks source link

Proposal: Replace NODE_ENV with REACT_ENV for __DEV__ replacement #6582

Closed zpao closed 6 years ago

zpao commented 8 years ago

This has come up a couple times lately as being an issue (#6479, #6581, #6511), I think perhaps because we added the minification warning and people are ending up seeing they aren't getting prod code when they expected it. But there's also the argument that you want React to be production and still use NODE_ENV for other purposes.

There would be a few things to figure out to make sure envify works, and deciding what we do for other projects which currently also use the NODE_ENV pattern (eg, Relay, fbjs, third-party code, etc).

This might not be a good idea at all though and definitely isn't happening immediately, but wanted to start the discussion.

ryanflorence commented 8 years ago

Seems like NODE_ENV is a solid pattern for all libs to use in the bundled-front-end-generation of apps, but not REACT_ENV.

Makes no sense for redux to use REACT_ENV, it's used in other contexts, so then we get REDUX_ENV too? RELAY_ENV, REACT_ROUTER_ENV, REDUX_ENV, EVERY_DEPENDENCY_IN_MY_SYSTEM_ENV :|

If you must not conflict with NODE_ENV, maybe BUNDLE_ENV or BUILD_ENV and the world can follow suit.

stubailo commented 8 years ago

Maybe REACT_ENV can override NODE_ENV but it would be optional?

ryankshaw commented 8 years ago

I like the idea of people being able to set REACT_ENV (or whatever else you want to call it) but fall back on NODE_ENV by default. that way, like @ryanflorence pointed out, you do not have to have a special environment variable for every library you are using by default, but if there is something tricky or fancy you are trying to do you could explicitly set an environment variable to override it

zpao commented 8 years ago

EVERY_DEPENDENCY_IN_MY_SYSTEM_ENV

Definitely want to avoid that.

If you must not conflict with NODE_ENV, maybe BUNDLE_ENV or BUILD_ENV and the world can follow suit.

Yea, this is interesting and could be an approach to take. There's also the concern that sometimes you are running in Node and do want NODE_ENV so fallback might be necessary as others have proposed. That would make our transformed code even uglier (at least until we have more confidence minifiers can handle the output of https://github.com/facebook/fbjs/pull/86)

Thanks for the quick feedback all!

gaearon commented 8 years ago

Babel is an interesting case because people use it both on the client and in production, and env in .babelrc reads from BABEL_ENV with a fallback to NODE_ENV (which is what most people specify). Introducing BUNDLE_ENV could be a solution but I feel like it would make the Babel situation more confusing.

ericclemmons commented 8 years ago

Historically, I run code in both "qa" and "staging" NODE_ENVs.

For minification to work and accurately reflect what will soon be on "production", the builds run with NODE_ENV as "production", but client-side code checks against STAGE for the true environment (as our loggers and reporters call differently based on the actually environment).

Point being, I think it's exceedingly difficult to solve this on the vendor side when the NODE_ENV pattern is so prevalent.

There's a point where the lookup will always be faked to meet the needs of the build.

I wonder if there's something more we can do in user land to resolve this?

ForbesLindesay commented 8 years ago

NODE_ENV is a really well established standard across the entire JavaScript ecosystem. As I see it, there's nothing wrong with people having something like APP_ENV for supporting people's own list of dev, qa, staging, prod etc. We need a consistent way to determine whether an app is running/being built in an optimised mode, or a development mode. NODE_ENV provides that brilliantly.

There may be cases where it is useful to put one specific library/part of your code into development mode, while everything else is in prod mode (or visa versa) but I think it's a corner case. This is not what's being asked for in any of the three linked issues. I think we should wait until someone actually wants the feature before we build it, and if we build it it should be as @stubailo (an optional variable to override the default NODE_ENV behaviour).

satya164 commented 8 years ago

I think it's worth noting that it should be possible to write a browserify transform/webpack loader which applies a different environment variable to a specific library. This way it will be useful to all without needing each library implement a separate environment variable.

ForbesLindesay commented 8 years ago

And babel-plugin for that matter (which would then work server side). I really like that suggestion.

koistya commented 8 years ago

It seems like NODE_ENV variable is often abused in Node.js community. In React Starter Kit we recommend to use NODE_ENV only to distinguish between "production" (optimized, aka "release") and "development" (non-optimized, aka "debug") builds. And use some other env variable to differentiate between multiple deployment types e.g. process.env.DEPLOYMENT = production | staging | qa | test.

lime commented 8 years ago

It sounds like people are (cautiously) positive to an optional, overriding env var. Since we are only toggling between a release/optimized/minified build and a debug/dev build, could we maybe have that reflected in the name & values of that var?

The pattern X_ENV = production | staging | test | etc is used in many overlapping ways across different languages, ecosystems and projects. This seems to be the main source of confusion. I would avoid copying that pattern into REACT_ENV, since it does not actually express what we're trying to do.

Our variable

With those requirements, I would suggest something along these lines:

REACT_RELEASE_BUILD = <any value> | <empty string>

or maybe

REACT_BUILD = release | debug

Using strings for a "boolean" value is problematic; you might expect "false" or "0" to be handled the same as "".

On the hand, using specific keywords easily devolves into bikeshedding ("release" vs "optimized" vs "production" etc).

Edit: accidentally posted early, revised text a bit.

gaearon commented 6 years ago

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.

gaearon commented 6 years ago

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.