nolanlawson / rollupify

Browserify transform to apply Rollup (UNMAINTAINED)
Apache License 2.0
386 stars 16 forks source link

parse commonjs packages out of the box #20

Closed yoshuawuyts closed 8 years ago

yoshuawuyts commented 8 years ago

if I'm not mistaken https://github.com/rollup/rollup-plugin-commonjs makes it so rollup can parse require() packages from start to finish. Might be cool to load it by default so all the browserify can toss in rollupify into their existing workflow without having to worry about import, ever :sparkles: What do you think?

nolanlawson commented 8 years ago

Unfortunately doing so would basically negate any of the benefits of Rollup, because the rollup-plugin-commonjs doesn't do any tree-shaking or variable-hoisting but rather loads it exactly like Browserify would. The point of rollupify is to basically use import/export for the local project while keeping third-party dependencies in the old-school Browserify/Webpack CJS style, i.e. squeeze out bytes without having to deal with subtle bugs in how Rollup handles third-party npm. (Long story short, Rollup would need like 5 extra plugins to really "just work" like Browserify, @calvinmetcalf is basically writing all those plugins right now but I don't know how mature they are.)

Closing because I consider this out-of-scope for rollupify and anyway you can use rollup-plugin-commonjs via the plugin hook.

yoshuawuyts commented 8 years ago

doesn't do any tree-shaking or variable-hoisting but rather loads it exactly like Browserify would

oh, that's super unfortunate. Maybe writing dead code elimination for commonJS might actually be easier than backporting things into rollup hah. Ah well, thanks for replying! :grin:

nolanlawson commented 8 years ago

Yeah I did consider that, but it's tough because it actually goes against the CJS spec (which expects per-module function scoping). Google Closure Compiler apparently does this flattening for CJS, but I'm not sure how robust it is.

yoshuawuyts commented 8 years ago

which expects per-module function scoping

Hm, for a first version couldn't the single scope thing be left out? It probably depends on the project, but isn't the biggest win the dead dep pruning? What I was thinking shouldn't be too hard to implement, e.g.

Haven't thought about the single scope stuff tho. How does it break the spec / what makes it tricky?

nolanlawson commented 8 years ago

The single scope issue means that you need to rename all the variables within the function so they don't collide when hoisted to a global scope. Closure Compiler does this by prefixing all of them: http://www.nonblocking.io/2011/12/experimental-support-for-common-js-and.html

For the dependency graph, I'm not sure how you would extract unused methods, since in CommonJS module.exports = {foo: 'bar'} is equivalent to exports.foo = 'bar', i.e. you're exporting one big object with key/value pairs where the key is the non-default export. In ES6 land you're not actually exporting an object when you do export var foo = 'bar', you're literally just exporting the variable. I guess you could strip the keys from the exported CJS object, but it may be a complex object with dependencies between the exported keys, which would then break.

I'm not really very good at explaining this, but there's a page on the Rollup wiki that basically answers the question "why does Rollup need ES6 modules in order to do its magic?" https://github.com/rollup/rollup/wiki/ES6-modules#the-killer-feature

As for tree-shaking vs scope-hoisting, in the case of PouchDB we had 100% code coverage (i.e. no dead code) and yet we still switched to Rollup because the scope-hoisting alone got us like 6% bundle size savings. I posted some resources on this here: https://gist.github.com/nolanlawson/744034415dae7bc8c50d

calvinmetcalf commented 8 years ago

hey so I did do a bit of work on alternative methods of compiling es6 modules to commmonjs modules.