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

Browser(ify) friendliness #2

Merged
merged 3 commits into from
Jul 12, 2015
Merged

Browser(ify) friendliness #2

merged 3 commits into from
Jul 12, 2015

Conversation

zertosh
Copy link
Contributor

@zertosh zertosh commented Jul 11, 2015

This adds a browserify friendly version of the module. It allows for the package to be used in browserify builds with detectGlobals: false, and for the unused parts to be minified away.

cc: @BerkeleyTrue

@BerkeleyTrue
Copy link
Owner

Hello @zertosh Thanks for the PR!

What is the difference between the browser version and the main module? Is it just moving process.env.NODE_ENV into the if conditional?

Why does envify need to be a dependency?

@zertosh
Copy link
Contributor Author

zertosh commented Jul 11, 2015

What is the difference between the browser version and the main module? Is it just moving process.env.NODE_ENV into the if conditional?

Yup that's the only code difference.

Why does envify need to be a dependency?

envify needs to be a dependency because browserify will try to locate and apply that transform when someone tries to bundle warning. The point of using envify instead of browserify's own process.env wrapper is that the unused code can be eliminated during minification.

Without envify, browserify will produce (comments removed for brevity):

(function (process){

var __DEV__ = process.env.NODE_ENV !== 'production';

var warning = function() {};

if (__DEV__) {
  warning = function(condition, format, args) {
    /* code */
  };
}

module.exports = warning;

}).call(this,require('_process'))

With envify, the value of process.env.NODE_ENV will get inlined, producing something like:

var __DEV__ = "production" !== 'production';

var warning = function() {};

if (__DEV__) {
  warning = function(condition, format, args) {
    /* code */
  };
}

module.exports = warning;

But by moving the code a bit, you can produce:

var warning = function() {};

if ("production" !== 'production') {
  warning = function(condition, format, args) {
    /* code */
  };
}

And then a tool like uglify can remove the entire block of unreachable code. This is the same thing React does and the same thing I do with invariant.

@zertosh
Copy link
Contributor Author

zertosh commented Jul 11, 2015

Also, you want to have two versions of the file because for nodejs use, your original version is more efficient. Accessing process.env is actually a costly operation (see facebook/react#812).

@BerkeleyTrue
Copy link
Owner

I am familiar with envify and the browserify community of packages. What I usually do is globally transform using envify. And uglify usually takes care of dead code. I don't like the idea of tracking two scripts that are identical.

Are you finding uglify dead code elimination does not work as intended with this package?

@zertosh
Copy link
Contributor Author

zertosh commented Jul 11, 2015

Applying global transforms can lead to very unexpected results. As for the dead code elimination, no, uglify (as of v2.4.23) is only able to determine that a conditional branch of code is unreachable if the condition's test is a constant.

I'm bringing this issue up, not because I'm using warning directly, but because it's a dependency of the new react-router. Once it's no longer in beta, this issue is going to affect a lot of people.

@BerkeleyTrue
Copy link
Owner

Fair enough. Merging. I feel this might break expected behavior for some. Will bump as major release.

BerkeleyTrue pushed a commit that referenced this pull request Jul 12, 2015
@BerkeleyTrue BerkeleyTrue merged commit 29b3d03 into BerkeleyTrue:master Jul 12, 2015
@zertosh
Copy link
Contributor Author

zertosh commented Jul 12, 2015

Thanks!

@zertosh zertosh deleted the browser-friendliness branch July 12, 2015 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants