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

Bundling pre-ES6 modules #45

Closed
callumlocke opened this issue Jul 16, 2015 · 36 comments
Closed

Bundling pre-ES6 modules #45

callumlocke opened this issue Jul 16, 2015 · 36 comments

Comments

@callumlocke
Copy link

Hi Rich. This project looks very exciting. I'm sold.

I have a question about pre-ES6 modules... I tried importing lodash as an experiment (today's lodash), and got this error:

Package lodash does not have a jsnext:main field, and so cannot be included in your rollup. Try adding it as an external module instead (e.g. options.external = ['lodash']

Is this a permanent design decision or is it just early days? Will you ever add functionality that can read an old-style main script and traverse require() calls...or do you recommend another strategy for people who want to mix ES6 and old school modules in one bundle (e.g. using Browserify as another build step after rollup)?

@Rich-Harris
Copy link
Contributor

I've gone back and forth on this. For the time being I think it's better to focus solely on ES6 modules, at least until we iron out all the kinks, but long term we definitely need a sane way to handle legacy module formats.

In theory, it can be done by overriding the resolveId function to accept CommonJS modules, and adding a transform step that wraps or rewrites modules so that Rollup can analyse them. To what extent you can convert CommonJS etc, I don't know - in a lot of cases it'd be straightforward, but I've no idea how you'd handle things like this (it's not quite as contrived as it looks)...

var myLib = exports;
myLib.foo = 'foo';

augment(myLib);
function augment(lib) { lib.bar = 'bar'; }

...so a strategy of wrapping rather than rewriting is probably better.

Luckily @guybedford has spent a lot of time thinking about these problems, and has done some initial research on incorporating Rollup's strategy inside JSPM: systemjs/builder#199. This seems like the best route forward, since JSPM is a soup-to-nuts solution (repository, bundler, module loader polyfill etc all talking the same language) that already handles legacy formats.

For right now though, my workaround is to have a two-phase bundling process, exactly as you suggest. First, I bundle my app code (plus any npm depedencies that expose jsnext:main, i.e. anything I wrote myself 😆) as CommonJS, and then I pass that bundle over to Browserify (but could be Webpack or whatever), which takes it from there. It works perfectly well, it's just a PITA to have to juggle all these different tools which is why I'm excited to see where JSPM goes.

@Victorystick
Copy link
Contributor

Actually, @callumlocke, if you want an to import lodash into your ES6 project, you can just include the ES6 build of lodash. On npm it's called lodash-es. It has a "jsnext:main" field defined, so it should be ready to go. I've been able import it in my bundles, and once #47 is approved, the only hiccup I've encountered with it so far should be resolved.

@ha404
Copy link

ha404 commented Sep 3, 2015

I'm pretty confused, does this replace Webpack? Thanks!

@Victorystick
Copy link
Contributor

The wiki says it better than I can.

@callumlocke
Copy link
Author

@Rich-Harris:

First, I bundle my app code (plus any npm depedencies that expose jsnext:main, i.e. anything I wrote myself 😆) as CommonJS, and then I pass that bundle over to Browserify (but could be Webpack or whatever), which takes it from there.

That sounds good! Just to clarify, with this rollup-then-browserify approach, would your source code contain contain a mixture of import statements and require calls? ...i.e. use import for your own local ES6 modules AND for third party modules that expose a jsnext:main, but use require for everything else)?

@Rich-Harris
Copy link
Contributor

@callumlocke you could, though more likely you'd write your app code all as ES6...

import external from 'third-party';
import foo from './my-utils';

/* ... *.

...and create a CommonJS bundle with Rollup, specifying which modules are external, then hand it over to Browserify:

rollup --input main.js\
       --format cjs\
       --external third-party,other-third-party\
       --output tmp/bundle.js

browserify tmp/bundle.js > dist/bundle.js

That's what I do, anyway!

@callumlocke
Copy link
Author

Ahh ok, I did try that at first, but rollup errored out when it failed to resolve a third party import (which didn't have a jsnext:main), because I wasn't using the cjs format.

So basically the cjs format means: rollup will bundle whatever it can into the file, but for unresolvable imports it will just convert the import statement into a require call. Is that a good summary?

@Victorystick
Copy link
Contributor

@callumlocke Yes, that's correct. What Rollup can't pull into the bundle will be "imported" using the idioms of the target module format.

// ES2015
import * as bar from './bar.js';

// CommonJS
var bar = require( './bar.js' );

// AMD
define(['./bar.js'], function (bar) {
  ...
});

// IIFE, use the `globals` option to tweak global names
(function (bar) {
  ...
)(bar);

And UMD being a combination of the last three.

@Hypercubed
Copy link

Hello @Rich-Harris (and others), Thank you for rollup. I'm using it in two of my new project.

Related to this issue. I'd really like to use rollup to pull in a few small helper utilities when building for the browser (for example a Object.assign shim and node.js util's isUndefined). Rollup seems ideal for this. Since rollup currently does not support importing from CJS modules what's the suggested temporary fix? The easy thing to do is copy and paste. We could also fork and convert a few core CJS modules to ES6 (https://github.com/defunctzombie/node-util/blob/master/util.js for example). Or wait for something else (e.g. future version of SystemJS builder using rollup).

I should add while I love JSPM for building apps I don't want to use JSPM (or Browserify) for building modules at this point. Rollup is much cleaner.

Edit: I also saw what you were doing with the *-jsnext repos.

@callumlocke
Copy link
Author

...and create a CommonJS bundle with Rollup, specifying which modules are external, then hand it over to Browserify:

rollup --input main.js\
       --format cjs\
       --external third-party,other-third-party\
       --output tmp/bundle.js

browserify tmp/bundle.js > dist/bundle.js

Could there be a new option like --auto-externals, which would tell rollup to assume I want to treat any unresolvable imports as external?

@callumlocke
Copy link
Author

@Victorystick:

Actually, @callumlocke, if you want an to import lodash into your ES6 project, you can just include the ES6 build of lodash. On npm it's called lodash-es. It has a "jsnext:main" field defined, so it should be ready to go. I've been able import it in my bundles, and once #47 is approved, the only hiccup I've encountered with it so far should be resolved.

Importing an individual method using e.g. import {zipObject} from 'lodash-es'; doesn't work (error: Module ~/foo/node_modules/lodash-es/lodash.js does not export zipObject).

Looks like this is because the module doesn't export zipObject from './array/zipObject'. Instead it just exports one big default (the entire lodash library). Is this going to be changed in lodash-es? Or is this a limitation of rollup, that it doesn't yet know how to grab a single property off a big default export in the absence of a named export?

@timshannon
Copy link

@callumlocke If you look, the main entry point for the lib just imports a bunch of smaller modules. What I've been doing (if I don't want to import the entire gigantic library) is just import the modules I want:

import findIndex from "../lib/lodash/array/findIndex";

It's been working great for me.

@Rich-Harris
Copy link
Contributor

@Hypercubed

Since rollup currently does not support importing from CJS modules what's the suggested temporary fix?

Depending on how many helpers we're talking about, copy and paste might be the best option, if it's the difference between being able to keep the bundle entirely inside Rollup and having to pass it off to Browserify for a second step. Though I'm still wondering about the possibility of somehow transforming CommonJS modules into ES6 during the transformation phase.

Long-term though, the best option is to start creating ES6 modules and open sourcing them! 😀

@callumlocke

Could there be a new option like --auto-externals

Maybe... with Esperanto (Rollup's predecessor) I found that being permissive caused more problems than it solved (e.g. if you got the wrong number of ../s when importing ../../utils/foo, it would assume you were referencing an external module), though if it was an option perhaps that would be okay. I typically use the JS API rather than the CLI, and do this sort of thing:

{
  entry: 'app.js',
  format: 'cjs',
  external: (function () {
    var dependencies = require( './package.json' ).dependencies;

    return Object.keys( dependencies ).filter( function ( name ) {
      return !require( name + '/package.json' )[ 'jsnext:main' ];
    });
  }())
}

An option would be simpler.

Importing an individual method using e.g. import {zipObject} from 'lodash-es'; doesn't work

That's a shame. It would be great to be able to use lodash as a 'true' ES6 module and get the tree-shaking benefits without having to remember (or lookup) the fact that zipObject lives in the array folder. There would be some complicating factors, e.g. chaining would only work if you included the whole library (because everything needs to live on lodash.prototype), so it would actually be a different library. You'd have to ask @jdalton if he has any plans in that direction!

Or is this a limitation of rollup, that it doesn't yet know how to grab a single property off a big default export

It's more a limitation of static analysis in as dynamic a language as JavaScript, unfortunately.

@timshannon FYI in more recent versions of Rollup, if you have lodash installed as an npm module, you can do this if it's easier:

import findIndex from "lodash/array/findIndex";

@Victorystick
Copy link
Contributor

@timshannon FYI in more recent versions of Rollup, if you have lodash installed as an npm module, you can do this if it's easier:

import findIndex from "lodash/array/findIndex";

This is how I've been doing it. Until lodash handles is exports properly.

@jdalton
Copy link
Contributor

jdalton commented Oct 12, 2015

Totally open to getting this supported. I've been kicking around a work-in-progress with @megawac in the es branch. I think he ran into rollup issues so we kind of hit a wall. If you have pointers or suggestions to get it straightened out that'd be great.

@megawac
Copy link

megawac commented Oct 12, 2015

For those interested, here is the test case I whipped up to test rollup with
lodash-es https://github.com/megawac/rollup-lodash-es-test

On Mon, Oct 12, 2015 at 11:33 AM, John-David Dalton <
notifications@github.com> wrote:

Totally open to getting this working. I've been working with @megawac
https://github.com/megawac in the es branch
https://github.com/lodash/lodash/tree/es. I think he ran into rollup
issues so we kind of hit a wall. If you have pointers or suggestions to get
it working that'd be great.


Reply to this email directly or view it on GitHub
#45 (comment).

@Rich-Harris
Copy link
Contributor

@jdalton Awesome! It might be as straightforward as creating a similar build that cuts everything except the export block from lodash.js, though that might be based on a superficial understanding.

I could try throwing a lodash-jsnext project together (similar to d3-jsnext and three-jsnext) to test out this hypothesis, if that's helpful?

@megawac Thanks. It's likely that the example-main.js version is pulling in all the extraneous clutter because of everything that needs to get added to lodash.prototype, and there are probably a few things we're including out of an abundance of caution (#179 is designed to eliminate some of those cases). A non-chainable version of the library (i.e. where there's no lodash function exported as default) would probably go a long way towards leaner builds.

@callumlocke
Copy link
Author

@Rich-Harris nice snippet, I'll use that at some point. I can see the problems with doing it magically. An alternative would be for rollup not to error out straight away, but carry on building the import tree, and then throw one big error at the end with a message listing all of the imports that failed due to a lack of jsnext:main. Then I could just copy and paste this list into my own --externals list (after skimming it and making sure I'm happy with it). It might sound like a small thing, but anything that makes it easier to try out rollup on a large existing codebase could be good for adoption!

@jdalton
Copy link
Contributor

jdalton commented Oct 13, 2015

I'll try adding extra foo.default module versions of foo modules to better isolate them.

@Victorystick
Copy link
Contributor

We're making some progress here as of the 0.20.0 release and the rollup-plugin-commonjs!

@jdalton
Copy link
Contributor

jdalton commented Oct 26, 2015

@Rich-Harris @megawac

Awesome! It might be as straightforward as creating a similar build that cuts everything except the export block from lodash.js, though that might be based on a superficial understanding.

I've updated the es branch.

@Victorystick
Copy link
Contributor

I've updated the es branch.

That's beautiful! 😄

@callumlocke
Copy link
Author

@jdalton nice commit!

I've done some experiments, and it is now possible to import individual functions using the nicer syntax: import {foo, bar} from 'lodash-es';

But it looks like rollup's tree-shaking doesn't work with this at all – you still need to import from individual function files if you want an optimal bundle. Not sure if this is a problem with rollup or a problem with the way lodash-es is organised. @Rich-Harris?


The following experiments were done with rollup --format=iife using rollup v0.19.2.

Importing from lodash-es main

import {chunk, zipObject} from 'lodash-es';

console.log(zipObject(chunk(['a', 'b', 'c', 'd'], 2)));

Result: ~429kB

Importing from categories

import {chunk, zipObject} from 'lodash-es/array';

console.log(zipObject(chunk(['a', 'b', 'c', 'd'], 2)));

Bundle size: ~92kB

Importing individual functions

import chunk from 'lodash-es/array/chunk';
import zipObject from 'lodash-es/array/zipObject';

console.log(zipObject(chunk(['a', 'b', 'c', 'd'], 2)));

Bundle size: ~4kB

@Rich-Harris
Copy link
Contributor

Excited about the new Lodash build. I haven't tried it yet, but my hunch is that unnecessary code is being included because Rollup is being overly cautious about side-effects (see #179). If that's the case, this gives us a powerful incentive to get that issue resolved...

@jdalton
Copy link
Contributor

jdalton commented Oct 26, 2015

I haven't tried it yet, but my hunch is that unnecessary code is being included because Rollup is being overly cautious about side-effects

Maybe further down the dep chain.
The lodash module and category modules, as they are in edge, have no function calls.

@jdalton
Copy link
Contributor

jdalton commented Nov 9, 2015

I've updated the es branch to work with Babel 6.

@jdalton
Copy link
Contributor

jdalton commented Dec 27, 2015

Is there something I can do with how I generate lodash-es modules to make tree-shaking easier?

@Rich-Harris
Copy link
Contributor

@jdalton Finally had a chance to play around with this a bit more. The tl;dr is that for the time being, Lodash users should continue to import individual files:

import zipObject from 'lodash-es/array/zipObject.js';

There definitely isn't a silver bullet in terms of small changes to Lodash that would no longer trigger Rollup's over-eager side-effect detection. There are lots of small things like this...

/** Used to resolve the decompiled source of functions. */
var fnToString = Function.prototype.toString;

/** Used to check objects for own properties. */
var hasOwnProperty = objectProto.hasOwnProperty;

/** Used to detect if a method is native. */
var reIsNative = RegExp('^' +
  fnToString.call(hasOwnProperty).replace(reRegExpChar, '\\$&')
  .replace(/hasOwnProperty|(function).*?(?=\\\()| for .+?(?=\\\])/g, '$1.*?') + '$'
);

...that add up to a lot of 'unnecessarily' included code. (In this case, even though Rollup is smart enough to realise that calling RegExp won't have any side-effects other than to assign a value to reIsNative, it panics around fnToString.call(...) and the .replace(...) calls, because it doesn't know what methods it's calling – it could be mutating an object called fnToString, for example.)

Beyond that, side-effects multiply – once Rollup thinks it needs to include one chunk of code, that has cascading effects in terms of what other bits of code it thinks it need to include. It's possible to be a lot more ruthless, but not without risking breaking things.

So while there are things that Lodash could do to make things marginally easier static-analysis wise, I'm hesitant to suggest them – they would probably involve rather large changes and it still wouldn't be as efficient as importing directly from individual files.

Having said that, most of the false positives appear to fall into two or three categories. I'm very hopeful that we'll be able to introduce some much more sophisticated static analysis that would make Rollup much less over-cautious:

  • we could track values as they're assigned to variables. While there are lots of cases where we obviously can't reliably do this statically, there are lots of places where we can – for example, if we know that the value of the isArray declaration is Array.isArray, we can be satisfied that any calls to isArray don't have side-effects
  • by the same token, in the example above, if we know that the value of fnToString is Function.prototype.toString, and that no-one has assigned a call (or computed) property to it, then we also know that fnToString.call(...) has no side-effects and returns a string. We can also know that calling .replace(...) on that string has no side-effects (certainly if the second argument is not a function) and returns another string. In aggregate, we could determine statically that the code in the sample above does not have side-effects, and therefore needn't be included unless we're using reIsNative
  • Rollup is also very cautious about functions that potentially mutate their arguments. In many cases it needn't be, especially if we're able to track values

The good news is that Lodash is an absolute goldmine in terms of battle-testing some of these theories.

@jdalton
Copy link
Contributor

jdalton commented Dec 31, 2015

@Rich-Harris

Finally had a chance to play around with this a bit more. The tl;dr is that for the time being, Lodash users should continue to import individual files:

import zipObject from 'lodash-es/array/zipObject.js';

Thanks for digging in! All of this side-effects detection is way more complicated than I thought the rollup detection would be. Is there a way to just detect imports of each module to build a tree of used modules and then know which to include in the bundle?

In the case of lodash-es import {zipObject} from 'lodash-es' looks to an
export {default as zipObject} from './array/zipObject' of lodash.js which looks to array/zipObject which has import baseSet from '../internal/baseSet' which then looks to internal/baseSet, and so on.

@guybedford
Copy link
Contributor

Perhaps custom compiler metadata could be provided with an option or hook
input to indicate which modules are side effect free to enable more
agressive optimization here?

On Thu, 31 Dec 2015 08:51 John-David Dalton notifications@github.com
wrote:

In the case of lodash-es import { zipObject } from 'lodash-es' ties to an export
{ default as zipObject } from './array/zipObject' of lodash.js which
looks to array/zipObject which has import baseSet from
'../internal/baseSet'; which then looks to internal/baseSet, and so on.


Reply to this email directly or view it on GitHub
#45 (comment).

@eventualbuddha
Copy link
Contributor

I like Guy's idea here. While we can try to statically analyze to determine
the answer, it eventually boils down to needing to run the code (which, if
you want to be reeeaaally crazy we could do in a sandbox 😉).
On Thu, Dec 31, 2015 at 1:18 AM Guy Bedford notifications@github.com
wrote:

Perhaps custom compiler metadata could be provided with an option or hook
input to indicate which modules are side effect free to enable more
agressive optimization here?

On Thu, 31 Dec 2015 08:51 John-David Dalton notifications@github.com
wrote:

In the case of lodash-es import { zipObject } from 'lodash-es' ties to
an export
{ default as zipObject } from './array/zipObject' of lodash.js which
looks to array/zipObject which has import baseSet from
'../internal/baseSet'; which then looks to internal/baseSet, and so on.


Reply to this email directly or view it on GitHub
#45 (comment).


Reply to this email directly or view it on GitHub
#45 (comment).

@Rich-Harris
Copy link
Contributor

All of this side-effects detection is way more complicated than I thought the rollup detection would be. Is there a way to just detect imports of each module to build a tree of used modules and then know which to include in the bundle?

That's actually how early versions of Rollup worked – it wouldn't even load a module until one of its exports was referenced by code that was directly reachable from the entry point. Unfortunately that turns out to be problematic in cases like this – the config.asap = async line gets dropped unless you analyse that code and determine that it affects the config object, which changes how defer behaves. We see similar things in D3.

Perhaps custom compiler metadata could be provided with an option or hook input

It's certainly an idea worth exploring. For a brief moment there was an aggressive option that didn't worry about side-effects, but it proved too failure-prone. The more we can do via static analysis the better, but a combination of approaches might be better still.

@jdalton
Copy link
Contributor

jdalton commented Dec 31, 2015

For a brief moment there was an aggressive option that didn't worry about side-effects, but it proved too failure-prone.

The lodash-es case is probably as safe as you're going to get.
All method dependencies are tracked to generate builds (AMD, ES, Node, etc) based on it.

In the meantime there's always babel-plugin-lodash to assist.
It's also great combo'ed with lodash-webpack-plugin.

@callumlocke
Copy link
Author

callumlocke commented Apr 14, 2016

hey @jdalton, what's the latest status of lodash-es and babel-plugin-lodash? What's the best practice at the moment for bundling lodash with rollup for clientside code (if you want to make sure only a minimal set of lodash functions make it into the bundle)?

@jdalton
Copy link
Contributor

jdalton commented Apr 14, 2016

@callumlocke lodash-es and babel-plugin-lodash are trucking along. The FAQ covers es6 to es6 transforms but if you'd like to contribute an example working with rollup that would be great.

@Rich-Harris
Copy link
Contributor

Going to close this as it's digressed a long way from where it started – bundling CommonJS now works reasonably well in most cases and there are other issues relating to tricky tree-shaking cases.

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

10 participants