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

browserify interoperablity #552

Closed
calvinmetcalf opened this issue Mar 11, 2016 · 9 comments
Closed

browserify interoperablity #552

calvinmetcalf opened this issue Mar 11, 2016 · 9 comments

Comments

@calvinmetcalf
Copy link

@calvinmetcalf calvinmetcalf commented Mar 11, 2016

one of the issues I've had with rollup is I can't use all the previous modules I've written with it due to interoperability issues, so after talking to @nolanlawson about it I decided to write some stuff to make browserify modules 'just work'.

@callumlocke
Copy link

@callumlocke callumlocke commented Apr 13, 2016

I know the list isn't all done yet, but would you mind posting your current best browserify-compatible plugins config?

@calvinmetcalf
Copy link
Author

@calvinmetcalf calvinmetcalf commented Apr 13, 2016

import commonjs from 'rollup-plugin-commonjs';
import nodeResolve from 'rollup-plugin-node-resolve';
import globals from 'rollup-plugin-node-globals';
import builtins from 'rollup-plugin-node-builtins';
import json from 'rollup-plugin-json';
import autoTransform from 'rollup-plugin-auto-transform';
rollup({
  entry: 'main.js',
  plugins: [
        autoTransform(),
        globals(),
        builtins(),
        nodeResolve({ jsnext: true, main: true, browser: true }),
        commonjs({
          ignoreGlobal: true
        }),
        json()
]
})

also you'll want to use some specific versions of rollup modules due to the pulls stalling specifically

  • my nowrap branch of the commonjs module (calvinmetcalf/rollup-plugin-commonjs#nowrap) which has a more nuanced check for commonjs modules avoiding false positives and avoids wrapping the entire module in a function so that parts of it can get tree-shaken (without that they will always be side effects)
  • my skipmodule branch of rollup (calvinmetcalf/rollup#skipmodule) this makes sure modules that are only required by code that has been shaken out never get included even if they have side effects, important for being able to do things like get randomBytes from crypto without getting all the public key stuff
@FredyC
Copy link
Contributor

@FredyC FredyC commented Jun 1, 2016

@calvinmetcalf Um, how can I actually install those forked modules? I don't know if this is something different for NPM 3 or what. I do remember it was possible just like npm install calvinmetcalf/rollup-plugin-commonjs#nowrap. However in this case it seems to be failing. The dist folder is missing and apparently it's not even completely cloned (eg. missing rollup.config.js). Trying to run npm run build just fails with strange errors. I guess I could just clone repo and build it myself, but that's hardly a solution for everyone on team.

@calvinmetcalf
Copy link
Author

@calvinmetcalf calvinmetcalf commented Jun 1, 2016

Woops yeah the rollup pre publish build thing doesn't work with installing
a fork I'll either need to commit the build or change it to post install

On Wed, Jun 1, 2016, 4:05 PM Daniel K. notifications@github.com wrote:

Um, how can I actually install those forked modules? I don't know if this
is something different for NPM 3 or what. I do remember it was possible
just like npm install calvinmetcalf/rollup-plugin-commonjs#nowrap.
However in this case it seems to be failing. The dist folder is missing and
apparently it's not even completely cloned (eg. missing rollup.config.js).
Trying to run npm run build just fails with strange errors. I guess I
could just clone repo and build it myself, but that's hardly a solution for
everyone on team.


You are receiving this because you authored the thread.

Reply to this email directly, view it on GitHub
#552 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABE4n7EEnrqR63zfLsqAr6t3AxT3Ox8lks5qHeYXgaJpZM4HvItu
.

@calvinmetcalf
Copy link
Author

@calvinmetcalf calvinmetcalf commented Jun 1, 2016

You can clone it, build it and manually link it if you want it right now

On Wed, Jun 1, 2016, 6:03 PM Calvin Metcalf calvin.metcalf@gmail.com
wrote:

Woops yeah the rollup pre publish build thing doesn't work with installing
a fork I'll either need to commit the build or change it to post install

On Wed, Jun 1, 2016, 4:05 PM Daniel K. notifications@github.com wrote:

Um, how can I actually install those forked modules? I don't know if this
is something different for NPM 3 or what. I do remember it was possible
just like npm install calvinmetcalf/rollup-plugin-commonjs#nowrap.
However in this case it seems to be failing. The dist folder is missing and
apparently it's not even completely cloned (eg. missing rollup.config.js).
Trying to run npm run build just fails with strange errors. I guess I
could just clone repo and build it myself, but that's hardly a solution for
everyone on team.


You are receiving this because you authored the thread.

Reply to this email directly, view it on GitHub
#552 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABE4n7EEnrqR63zfLsqAr6t3AxT3Ox8lks5qHeYXgaJpZM4HvItu
.

@calvinmetcalf
Copy link
Author

@calvinmetcalf calvinmetcalf commented Aug 20, 2016

Since Nolan keeps on linking to this I thought I'd give an update/overview

Can you use arbitrary node modules and they "just work" like in browserify?

No, there are valid browserifable structures that do not work with rollup so it doesn't just work all the time.

But if you are using commonjs modules with rollup you are likely missing out on rollups benefits as they will never be tree shaken out ever (as the current commonjs plugin is set up) and they have a function called on at runtime (like Webpack or browserify) which imposes a startup cost that traditionally rollup omits.

Is this fixable?

Yes but probably not by me alone. The mandatory and blocking one is circular dependencies. Basically rollup assumes es6 module rules where depended on modules are always evaluated before the script that imports them is. Commonjs evaluate modules at the exact moment it's required the first time.

It would seem to me the straight forward method would be for rollup to simply inline the module being required at the place it was required (instead of before it) but rollup has no ability to do that at the moment. If I both have time and can figure out how I'll open a pull, I don't know how likely that is to happen though (both the time and me being able to figure it out) but even if I did I'd probably need to discuss with the rollup folks about how to do it.

But then we're great right?

As I said before commonjs modules don't tree shake out right now, this is because of how they are defined right now which rollup sees as a side effect so never removes it, I have a fix to the commonjs plugin but it doesn't currently handle top level returns, so I need to fix that.

But then right?

So there is one issue I have with rollup that makes me hesitant to use it as a browserify replacement and that is its strict inclusion of all code that produces side effects or might always. This means if a imports b which imports c which imports d which imports e which imports f, g, h, and i then if e has side effects but the part of b that imports c is tree shaken out and thus c and d are removed, e and thus f, g, h, and i will still be included. This basically breaks tree shaking in practice but fixing it is a spec divergence so we'll see if my pull to fix it gets merged or if others have better ideas.

So rollup is pretty much broken and I shouldn't use it?

Most definitely not, it is a great tool written by great people and it is totally works for certain work flows such how pouchdb is using it to bundle it's self, it's just not a browserify replacement... Yet.

(sent from my phone so apologies for any autocratic typos)

@wearhere
Copy link

@wearhere wearhere commented Aug 20, 2016

@calvinmetcalf by "my pull to fix [strict inclusion of all code that produces side effects or might always]" you mean #555 right?

@calvinmetcalf
Copy link
Author

@calvinmetcalf calvinmetcalf commented Aug 21, 2016

@wearhere yes sorry should have linked

@shellscape
Copy link
Contributor

@shellscape shellscape commented Aug 7, 2019

Hey folks. This is a saved-form message, but rest assured we mean every word. The Rollup team is attempting to clean up the Issues backlog in the hopes that the active and still-needed, still-relevant issues bubble up to the surface. With that, we're closing issues that have been open for an eon or two, and have gone stale like pirate hard-tack without activity.

We really appreciate the folks have taken the time to open and comment on this issue. Please don't confuse this closure with us not caring or dismissing your issue, feature request, discussion, or report. The issue will still be here, just in a closed state. If the issue pertains to a bug, please re-test for the bug on the latest version of Rollup and if present, please tag @shellscape and request a re-open, and we'll be happy to oblige.

@shellscape shellscape closed this Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.