Skip to content

More accurate side-effect detection - #253

Merged
Rich-Harris merged 32 commits into
masterfrom
side-effects
Nov 14, 2015
Merged

More accurate side-effect detection#253
Rich-Harris merged 32 commits into
masterfrom
side-effects

Conversation

@Rich-Harris

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

3 participants