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

warn user when using minified code outside of NODE_ENV 'production' #1075

Merged
merged 1 commit into from
Jan 13, 2016

Conversation

conorhastings
Copy link
Contributor

@gaearon my attempt at #1029 . Likely still needs a bit of work but I wanted to get your opinion before i continued. thanks!

screen shot 2015-11-26 at 12 35 14 am

@gaearon
Copy link
Contributor

gaearon commented Nov 27, 2015

This looks correct. (Have you checked whether it works?)
Let's just move this check inline and keep comments to the minimum—it’s not part of a public API.
Something like a two-line comment would do because we explain the issue in the warning anyway.

@conorhastings
Copy link
Contributor Author

@gaearon updated, is this what you meant by moving the check inline?

Yes, I tested with this branch in a small app using redux and webpack with DefinePlugin with the following combinations:

  1. NODE_ENV='production' , minified - no warning.
  2. NODE_ENV='development' minified - warning
  3. NODE_ENV='development' not minified, no warning
  4. NODE_ENV='production' not minified, no warning.
  5. NODE_ENV='production' , minified, removed DefinePlugin , warning.

function isCrushed() {}

if (isCrushed.name !== 'isCrushed' && process.env.NODE_ENV !== 'production') {
var warningMessage = 'You are utilzing minified code outside of NODE_ENV === \'production\'. ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

We're only using warningMessage once, let's put it right inside the console.warn call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that makes sense, updated.

@gaearon
Copy link
Contributor

gaearon commented Nov 27, 2015

Thanks!
Let's leave this hanging for now—I'll come back to this after I have some time to cut a new release.

@conorhastings
Copy link
Contributor Author

@gaearon sounds good thanks for such a great project and for the quick review/feedback!

@sapegin
Copy link
Contributor

sapegin commented Nov 27, 2015

utilzing → utilizing. Or much better: using.

@gaearon
Copy link
Contributor

gaearon commented Nov 27, 2015

I'll tweak the error message anyway before merging, no worries!

@timdorr
Copy link
Member

timdorr commented Jan 13, 2016

@gaearon I'm going to merge this in and tweak the message a tad on master. Please flog me appropriately if you disagree or don't like my tweaks :)

timdorr added a commit that referenced this pull request Jan 13, 2016
warn user when using minified code outside of NODE_ENV 'production'
@timdorr timdorr merged commit ee5b52e into reduxjs:master Jan 13, 2016
@gaearon
Copy link
Contributor

gaearon commented Jan 13, 2016

Thanks, please do!

@gaearon
Copy link
Contributor

gaearon commented Jan 25, 2016

Out in 3.0.6, thanks!

@marcelowa
Copy link

Hi
I have other environments like staging that is not production but is suppose to behave like production
this should be configurable somehow

@timdorr
Copy link
Member

timdorr commented Jan 26, 2016

I would use another ENV for swapping your configuration based on environment (CONFIG_ENV, for instance). Even better, I would configure your application based on various ENVs specific to those parts, such as things like DATABASE_URL or IMAGE_HOST or whatever you've got in your app.

NODE_ENV is heavily used by many projects (not just Redux) to check if developer features should be enabled. I wouldn't rely on it for defining a staging context.

@gaearon
Copy link
Contributor

gaearon commented Jan 26, 2016

NODE_ENV is heavily used by many projects (not just Redux) to check if developer features should be enabled. I wouldn't rely on it for defining a staging context.

Yes, this is the reason we went with it. For example, React behaves much slower with NODE_ENV being something other than 'production'. Since it’s often used with Redux it makes sense for us to converge on the same behavior.

@marcelowa
Copy link

Searched the source code and saw this now
It seems to be consistent
Not sure about this convention though

@gaearon
Copy link
Contributor

gaearon commented Jan 26, 2016

Well, it's being used in tons of projects now so we're not changing it anytime soon :-)

@gaearon
Copy link
Contributor

gaearon commented Jan 26, 2016

If you don't want to deal with it you can use UMD builds. The minified build is envified for prod environment.

@marcelowa
Copy link

I will use my own environment as suggested :)
Thanks

* create a function so that we can check if the function name has been altered by minification
* if the function has been minified and NODE_ENV !== 'production', warn the user
*/
function isCrushed() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This line seems to behave differently in ie9 vs. modern browsers. Possible cause of #1311?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, fixed in 3.1.4.

@xhawk
Copy link

xhawk commented Feb 4, 2016

We are using react with Java backend and this same error pops up. Why is that?

@gaearon
Copy link
Contributor

gaearon commented Feb 4, 2016

It means you are minifying the code, but not envifying the CommonJS build. What do you build your JavaScript assets with?

@xhawk
Copy link

xhawk commented Feb 4, 2016

We are using Webpack, is this what we should do new webpack.DefinePlugin({ "process.env": { NODE_ENV: JSON.stringify("production") } })

@gaearon
Copy link
Contributor

gaearon commented Feb 4, 2016

We are using Webpack, is this what we should do new webpack.DefinePlugin({ "process.env": { NODE_ENV: JSON.stringify("production") } })

Yes. JSON.stringify("production") just makes sure the string is quoted. It’s essentially '"production"'.

*/
function isCrushed() {}

if (isCrushed.name !== 'isCrushed' && process.env.NODE_ENV !== 'production') {

Choose a reason for hiding this comment

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

This doesn't seem right, if process.env.NODE_ENV is not defined that doesn't necessarily indicate that I'm using minified code outside of production, or am I missing something? In our codebase we define NODE_ENV without the "process.env" prefix but I can imagine lots of apps have no need to define it.

Choose a reason for hiding this comment

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

From POV it was pretty surprising to see this pop up in production builds. Doesn't seem like something a library should try to enforce IMO.

Copy link
Member

Choose a reason for hiding this comment

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

That comes from the Node universe, where process.env.NODE_ENV is defined by default. process is a global in Node. It is used in many libraries (React included) to define where development code is. Our module bundler (and most common setups) replaces the text process.env.NODE_ENV with "production", which gets eliminated by an optimizer as dead code.

So, it would be a good idea to emulate the same behavior, even if you don't like it. There are some Babel plugins to make this easier (letting you use a global DEV). You're probably getting development builds of many other things for the same reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

The CommonJS build of Redux uses Node conventions. You can explicitly use the UMD builds in dist folder if you’d rather avoid them.

We just went ahead with the same pattern React is using. If you use the CommonJS build of React and don’t envify process.env.NODE_ENV, you are running a slow development version of React in production.

From http://facebook.github.io/react/docs/getting-started.html#using-react-from-npm:

screen shot 2016-02-07 at 01 54 44

We just follow the same convention.

Choose a reason for hiding this comment

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

Fair enough. I suppose it makes sense for libraries to follow a convention for this and being loud about an inefficient build is good service. Thanks to both for your responses!

Choose a reason for hiding this comment

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

Out of curiosity though I would expect to see conditionals with process.env.NODE_ENV or similar in the react codebase, but I don't. If I'm using webpack and requiring react how does defining it make the react build faster?

Copy link
Member

Choose a reason for hiding this comment

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

They use DEV in their codebase, which is converted to the process.env format when they build the CommonJS version for publishing.

Choose a reason for hiding this comment

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

Oh interesting, got it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants