-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Switch to ES Modules with Rollup #5939
Comments
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:
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:
So it looks like our hopeful "what if"
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. |
@anandthakker have you tried to deliver ES6 modules directly to browser? Give a try for those: |
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:What do you think?
cc @jfirebaugh @anandthakker
The text was updated successfully, but these errors were encountered: