Skip to content

include minified build with npm module? #683

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

Closed
porterjamesj opened this issue Dec 20, 2013 · 16 comments
Closed

include minified build with npm module? #683

porterjamesj opened this issue Dec 20, 2013 · 16 comments

Comments

@porterjamesj
Copy link

Minifying it myself takes forever; would be nice to be able to type var React = require('react/react.min.js'); and be done with it (underscore does this, for example).

@porterjamesj
Copy link
Author

Also related to the npm module: I can var React = require("react-tools").React; just fine, but when I var React = require("react"); I get:
TypeError: ancestorNode is undefined during click handling.

note that this is using browserify, which may have something to do with it. If its not immediately apparent what's wrong I can open a separate issue.

@luigy
Copy link
Contributor

luigy commented Dec 20, 2013

@porterjamesj strange, I'm using browserify as well and the following works fine. Are you doing something similar?

<a onClick={this.myHandler}></a>

@andreypopp
Copy link
Contributor

@porterjamesj I tried to reproduce and it works fine, you can see my setup at https://gist.github.com/andreypopp/a49df77285ae2609a6e2

@andreypopp
Copy link
Contributor

@porterjamesj regarding time of browserifying, in the previous case it takes about 1.8s to produce a bundle on MBA2012, which is a bit long... but during development I use watchify so on changes it doesn't rebuild entire bundle from scratch.

@zpao
Copy link
Member

zpao commented Dec 20, 2013

Right now our minified build just uses browserify on the original package + uglify. As a result, there are still require calls, which breaks browserify if you then wanted to bundle your own application. We talked about "muffinizing" in #627 (where we would replace all the require calls with a random word), but with envify + browserify, we've worked around that.

Underscore doesn't have that problem as it's all 1 file to begin with with no modules.

@porterjamesj
Copy link
Author

@andreypopp It's not browserifying that's slow for me; it's minifying the browserified bundle, which takes on the order of minutes.

Problem with react as opposed to react tools must have something to do with my onClick setup; will investigate. Odd that it works with one but not the other though.

@porterjamesj
Copy link
Author

@andreypopp your example works for me, so clearly the problem lies with my code.

@porterjamesj
Copy link
Author

heh; turns out I was requiring react in one place and react-tools in another; sorry for noise.

I do still think it would be nice to include the minified version in the npm module fwiw.

@andreypopp
Copy link
Contributor

@porterjamesj do you want to serve minified React separately or just prepend it to your own code and not pipe through uglify? Because otherwise I don't see how you would extract performance gains from already minified build. At the same time, you probably would want minification only for production builds, does this happen so frequently to be annoying?

Also

% time browserify ./index.js | uglifyjs > /dev/null 
browserify ./index.js  1.66s user 0.16s system 102% cpu 1.763 total
uglifyjs > /dev/null  0.52s user 0.06s system 26% cpu 2.124 total

so uglifyjs call takes just 0.5s.

@andreypopp
Copy link
Contributor

With -c -m:

% time browserify ./index.js | uglifyjs -c -m > /dev/null
browserify ./index.js  1.63s user 0.15s system 103% cpu 1.726 total
uglifyjs -c -m > /dev/null  1.50s user 0.09s system 50% cpu 3.121 total

@zpao
Copy link
Member

zpao commented Dec 20, 2013

FWIW, just browserifying+minifying react is not quite the same as react.min.js. react.min.js strips out extra logging and error messages. Since we use envify, you'll want to make sure process.env.NODE_ENV === 'production' (haven't looked at how that works with browserify on it's own, probably just NODE_ENV=production browserify ...

@andreypopp
Copy link
Contributor

@zpao yeah, but envify transform is active in either case, just checked with NODE_ENV=production, similar timings but code size reduced even more as expected.

@porterjamesj
Copy link
Author

@andreypopp you're right that minifying doesn't need to happen frequently so it's not that big of an issue.

@zpao thanks for the info!

@petehunt
Copy link
Contributor

Setting NODE_ENV to production should improve runtime perf too.

I would prefer to not check in minified code. Let's instead make sure that we can minify with existing tools in a performant way. Sound good?

@porterjamesj
Copy link
Author

Makes sense; thanks for tips everyone.

@zpao
Copy link
Member

zpao commented Dec 20, 2013

To clarify what @petehunt meant, it would improve runtime perf of React, not the browserification runtime.

Glad we could help @porterjamesj!

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

No branches or pull requests

5 participants