Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 13, 2016
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,22 @@ import bindActionCreators from './utils/bindActionCreators'
import applyMiddleware from './utils/applyMiddleware'
import compose from './utils/compose'

/*
* 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.


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

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.

Copy link

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.

Copy link

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!

Copy link

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.

Copy link

Choose a reason for hiding this comment

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

Oh interesting, got it!

/*eslint-disable no-console */
console.error('You are currently using minified code outside of NODE_ENV === \'production\'. ' +
'This means that you are running a slower development only build of Redux. ' +
'Consult tools such as loose-envify (https://github.com/zertosh/loose-envify) for browserify ' +
'and DefinePlugin for webpack (http://stackoverflow.com/questions/30030031) ' +
'to build with proper NODE_ENV')
/*eslint-enable */
}

export {
createStore,
combineReducers,
Expand Down