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

More accurate side-effect detection #253

Merged
merged 32 commits into from
Nov 14, 2015
Merged

More accurate side-effect detection #253

merged 32 commits into from
Nov 14, 2015

Conversation

Rich-Harris
Copy link
Contributor

This is quite a big PR; bear with me. Among other things it needs some battle testing and a tidy up before it gets merged (some of the names are a bit weird, etc).

Currently, any top-level CallExpression (i.e. not inside a function declaration/expression) is assumed to contain side-effects, and is therefore included in the bundle. This is often wasteful:

// main.js
import { random } from './maths.js'

export const r = random( 0, 100 );

// maths.js
export function power ( n ) {
  return function ( base ) {
    return Math.pow( base, n );
  };
}

export const square = power( 2 );
export const cube = power( 3 );

export function random ( min, max ) {
  return min + ( max - min ) * Math.random();
}

In this example there's no reason for square, cube and power to be included. As of this PR, Rollup is smart enough to recognise that power refers to a pure function (i.e. calling it has no side-effects).

A few other changes that fall out of this way of doing things:

  • Classes transpiled with Babel, which currently get included whether or not they're used because they get wrapped in an IIFE, are now only included if appropriate
  • We no longer need to treat IIFEs differently – they're just the same as any other CallExpression.
  • Detecting strong dependencies is slightly more robust – if a module calls foo, and foo refers to bar, the module depends strongly on the module where bar is defined. Previously, the module had to have a bar reference at the top level (or inside an IIFE, thanks to some special case logic introduced to accommodate a pattern used a lot inside Three.js)
  • Global functions that are known to be pure (e.g. new Array(foo) or Object.keys(bar)) can be treated the same as local functions that are known to be pure

Unfortunately this only goes some way towards solving the problem identified here – that apps using individual Lodash functions end up bundling a lot of unnecessary stuff (though with this PR it's nowhere near as bad as @callumlocke found). A lot of it could be eliminated if the lodash-es build were to forgo a default export altogether, but that's certainly not a perfect solution.

The reason the extra stuff gets included is because Rollup digs up the entire dependency graph and scans it for side-effects – and discovers statements that could have side-effects, but in modules we're not really interested in.

To be completely safe, we do need to include those side-effects because of things like this, where module a happens to muck about with the contents of module b, which we're using. But in a lot of cases – Lodash being a fine example – that's overly cautious. So this PR also introduces aggressive mode, which only looks for side-effects in the entry module:

rollup.rollup({
  entry: 'main.js',
  aggressive: true
}).then(...)

With aggressive: true, these result in almost exactly the same sized bundle:

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

// ...or this
import chunk from 'lodash-es/array/chunk';
import zipObject from 'lodash-es/array/zipObject';

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

Oh, and I moved a couple of files around to make things a bit easier.

Thoughts welcome!

@Victorystick
Copy link
Contributor

Woo! Comments! I haven't had a chance to read through the PR on a computer, but I like the look of it.

@eventualbuddha
Copy link
Contributor

I don't really have the time to look at this properly, but I looked over the tests quickly and they seem sane. This is exciting!

@Rich-Harris
Copy link
Contributor Author

I think I have some changes on this branch that aren't checked in (on another machine) so I won't merge this just yet

Rich-Harris added a commit that referenced this pull request Nov 14, 2015
More accurate side-effect detection
@Rich-Harris Rich-Harris merged commit fe0394a into master Nov 14, 2015
@Rich-Harris Rich-Harris deleted the side-effects branch November 14, 2015 15:28
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.

None yet

3 participants