mapbox/mapbox-gl-js

Switch to ES Modules with Rollup

mourner opened this issue · 10 comments

I've been looking into this today, and to sum up, I think we should do it.

Up till now, the main blocker for this has been our desire to keep time-to-first-render low. According to research in #3153 by @jfirebaugh, the amount of code evaluated on the worker side is the biggest factor in that, so we've been assuming we need to automatically create slim worker bundles from the main bundle, stripping out all the unnecessary code. So far, this has only been possible with Browserify because it keeps all the code in a dependency tree which we can traverse at runtime. There's no way to do that in Rollup — see rollup/rollup#1029.

Note that when we implemented slimming down the worker bundle (browserify/webworkify#30), the overall effect on TTFR was not that big — only ~200ms despite the bundle becoming ~2x smaller.

Today, I decided to take a small part of GL JS and convert it to Rollup to see how it would look — specifically, the expression code. When uglified/gzipped, the ES modules based build is approximately 10% smaller than the browserify-based one. Additionally, I wrapped the whole code with a simple console.time/timeEnd — the browserify-based build executed in 5.5ms in Node but the rollup one in just 1.2ms. Then I also remembered this article by Nolan Lawson that benchmarked the runtime cost of different bundlers (and showed Browserify having a huge hit compared to Rollup).

All this made me realize that maybe we should have looked at Browserify itself as the culprit of long evaluation times, and not just the size of the bundle. What if the switch from Browserify to ES Modules / Rollup reduces TTFR so dramatically that we no longer have to worry about worker bundle size much? Then, we could just make a custom rollup prelude that would launch the whole bundle as workers without traversing the deps at runtime.

My hunch is that we should take the plunge with ES Modules. The only big drawback I see is that we'll have to complicate the unit test setup (possibly relying on something like @std/esm), but advantages are great:

  • smaller bundle
  • possibly better time-to-first-render
  • modern code base without the awkward mix of ES-like flow type imports and CommonJS
  • more strict compile-time checking of dependencies

What do you think?

cc @jfirebaugh @anandthakker

If the worker overhead is not too big, this would be ideal. Is there any way to check that short of doing the entire conversion?

@jfirebaugh by my estimates, the evaluated worker bundle will be 40% bigger, but is likely to be fully offset by faster evaluation (due to linear structure without lots of closures).

Started a branch to see how this would look, converted most of style-spec code so far: https://github.com/mapbox/mapbox-gl-js/compare/rollup

Seems pretty straightforward, and satisfying too. I also suspect that converting the whole codebase will lead to bigger savings than that small sample because of all the require interdependencies that will be eliminated by having just one reference to each used thing in one big closure.

You can also try to reuse flow type information to get more advanced minification with Closure compiler sindresorhus/project-ideas#79

@stereobooster interesting! How does Closure Compiler take advantage of types to make smaller builds?

(Finally) got a fully working proof of concept going up at https://github.com/mapbox/mapbox-gl-js/tree/rollup-anandthakker

Promising initial results:

master:

screen shot 2018-02-16 at 9 03 50 pm

682K Feb 16 21:03 mapbox-gl.js

rollup-anandthakker:
screen shot 2018-02-16 at 9 03 18 pm

601K Feb 16 20:59 mapboxgl-rollup.js // (minified)

Haven't yet measured the time to workers-ready yet, but will check that out soon

Looks like the time-to-first-render is about 100ms worse for the rollup build, due to the significantly larger worker bundle taking longer to compile/evaluate:

branch compile script evaluate script
master 28ms 66ms
rollup-anandthakker 53ms 98ms

So it looks like our hopeful "what if"

What if the switch from Browserify to ES Modules / Rollup reduces TTFR so dramatically that we no longer have to worry about worker bundle size much?

didn't pan out.

Rollup recently got code splitting, so next I'm going to see if we can use that to get the best of both worlds -- indeed, thanks to rollups es-module-based tree shaking, this could mean an even smaller worker bundle than we had with webworkify.

Update: using code splitting gets us back to parity with master:

  • compile script: 41ms
  • evaluate script: 70ms

Note that according to the performance tab, "compile script" and "evaluate script" happen in parallel, so this really is pretty close to master.

screen shot 2018-02-21 at 2 20 03 pm