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

Switch to ES modules and Rollup #6196

Merged
merged 33 commits into from
Mar 12, 2018
Merged

Switch to ES modules and Rollup #6196

merged 33 commits into from
Mar 12, 2018

Conversation

anandthakker
Copy link
Contributor

@anandthakker anandthakker commented Feb 20, 2018

Closes #5939
Refs #6056

This PR:

  • Refactors the codebase to use ES modules
  • Changes the bundler from browserify to Rollup

Note on building the Worker bundle blob:
There's a substantial change here in our approach to constructing the Blob URL for the worker script, because webworkify depends on specific browserify implementation details. The way we're doing it now:

  • Use rollup's "code splitting" feature to run the build against two entrypoints: src/index.js and src/source/worker.js. The result is three bundles: rollup/build/index.js, rollup/build/worker.js, and rollup/build/chunk1.js. The first two correspond to the two entrypoints, and the third consists of all the code that is shared by the main thread and worker scripts.
  • Use a second pass to assemble these three chunks into a single bundle, with some webworkify-inspired wrapper code that, at runtime, stringifies the worker source and uses it to create our worker blob URL.

Note on running our code in Node:
In order to run our Flow-annotated ES module code in a Node environment, we have to use two require hooks. Previously, we would do this for flow by doing require('flow-remove-types/register') in our common unit test module -- this allowed us to require flow-annotated files from tests, but did not permit flow in the test files themselves. Instead, we're now telling node to require flow-remove-types (and @std/esm, for modules) directly via command-line arguments; you can run node or tap with these args already set using the build/run-{tap,node} scripts.

Benchmarks

Full
Re-run of Paint

Time to first render:

rollup-anandthakker:

screen shot 2018-03-07 at 3 43 55 pm

master

screen shot 2018-03-07 at 3 44 19 pm

Minified bundle size:

-rw-r--r--  1 anand  staff   709K Mar  7 15:37 dist/mapbox-gl--master.js
-rw-r--r--  1 anand  staff   572K Mar  7 15:42 dist/mapbox-gl--rollup-anandthakker.js
-rw-r--r--  1 anand  staff   169K Mar  7 15:37 dist/mapbox-gl--master.js.gz
-rw-r--r--  1 anand  staff   148K Mar  7 15:42 dist/mapbox-gl-rollup-anandthakker.js.gz

Todo


export const {
deserialize
} = exported;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably get rid of all instances of exporting object literals like this — I suppose this is a legacy require/codemod thing? Ideally we would always export symbols directly, and only export default for classes/functions. This makes tree-shaking more effective.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 yeah these cases were either codemod outputs (or just the quickest manual fix for the time being). I'm not sure how much tree-shaking we'll enjoy from within our codebase, since I don't think we have too much dead code within src/... but if we do end up going the code-splitting route, this could make a notable difference in the worker blob size. It'll also just be simpler/clearer code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you ticket this as a followup item?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, already did: #6293

@@ -1,5 +1,5 @@
/* eslint-disable */
(function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw f.code="MODULE_NOT_FOUND",f}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({1:[function(require,module,exports){
(function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw (f.code="MODULE_NOT_FOUND", f)}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({1:[function(require,module,exports){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Committed accidentally?

}).on('cycle', (event) => {
console.log(String(event.target));

}).run();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove those benchmarks as a part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can reinstate them or remove them in a separate PR -- I was just getting distracted by the error messages coming from these files :)

const fs = require('fs');

require = require('@std/esm')(module, {"cjs":true, "esm":"js"});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand on why flow-remove-types and @std/esm don't play well together? It would be great if we could avoid generating the source to a separate src_untyped folder...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some context here: standard-things/esm#119

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if we could avoid generating the source to a separate src_untyped folder...

I agree, I'm really hoping we can avoid a build step for tests 🤞

@mourner
Copy link
Member

mourner commented Feb 22, 2018

BTW how's the total bundle size (minified/gzipped) looking?

@anandthakker
Copy link
Contributor Author

@mourner pretty good!

master

-rw-r--r--   1 anand  staff   682K Dec 12 23:23 mapbox-gl.js
-rw-r--r--   1 anand  staff   168K Dec 12 23:23 mapbox-gl.js.gz

rollup-anandthakker

-rw-r--r--   1 anand  staff   555K Feb 22 11:13 mapbox-gl.js
-rw-r--r--   1 anand  staff   144K Feb 22 11:13 mapbox-gl.js.gz

@anandthakker
Copy link
Contributor Author

anandthakker commented Feb 24, 2018

Alright, using Rollup's relatively new code splitting feature, we're now at a point where we have a smaller bundle that loads as fast as our browserify build. Before I embark on the longish tail of tasks that would be needed to finish this and merge it into master, it's time to ask: should I?

The advantages:

  • A smaller bundle, with the potential for (possibly significant?) further optimizations via "tree shaking"
  • Interesting build possibilities: with rollup & es modules, we could experiment with a build of GL JS that uses dynamic imports to fetch modules on demand.
  • Modernized codebase:
    • language-level module declarations (rather than runtime ones via require()/module.exports)
    • better handling of cyclic dependencies
    • cleaner code (IMO, anyway)
    • consistent with our ubiquitous Flow import type / export type declarations

The disadvantages:

  • Browserify has been around a long time, is well hardened, we know it works.
  • Requires the use of Rollup's new "code splitting" functionality, which is still new and behind a feature flag (experimentalCodeSplitting). (On the other hand, there are a couple of active core maintainers who have already been very responsive.)
  • Might require something like a build step to handle the combination of ES modules + Flow before we can run code in Node (for unit tests, integration tests, codegen)

@mapbox/gl-core what do you think? Please weigh in! It's definitely not too late to abandon this effort if we don't feel that the time is right.

@ChrisLoer
Copy link
Contributor

I can't really comment on the overall approach although it sounds like progress! Two questions:

  • How would "dynamic imports to fetch modules on demand" work? I'm wondering if we'd end up building out infrastructure that would make it convenient to have mapbox-gl-rtl-text auto-load instead of requiring additional configuration. Even closer to the dream would be to dynamically choose whether to load a WASM or ASM.js version of the plugin based on browser capability.
  • You've been doing a lot of bechmarking of time-to-first-render, right? Can you document any of those numbers here?

@jfirebaugh
Copy link
Contributor

Might require something like a build step to handle the combination of ES modules + Flow before we can run code in Node (for unit tests, integration tests, codegen)

I think this is the most important unanswered question. Way back in the day we reverted an early attempt to introduce ES6 syntax because tooling wasn't ready -- specifically debuggers and code coverage. I rely on running individual test files pretty frequently, often attaching a node debugger to track down test failures. If switching to ES6 modules would mean I can no longer do that, I'd prefer to hold off on it.

@anandthakker
Copy link
Contributor Author

🎉 looks like flow-remove-types and @std/esm can cooperate after all! @jfirebaugh I confirmed that I'm able to attach a debugger with node --inspect and step through original sources.

@anandthakker
Copy link
Contributor Author

anandthakker commented Feb 27, 2018

Another open question: how should we build the benchmarks? Options:

  1. Reproduce the code-splitting trick we're using for the main bundle. Pro: we continue to have a single benchmarks.js bundle that gets published on each master CI build and for each release. Con: more complicated build setup.
  2. Generate a separate worker.js script, and, in benchmarks.js, set mapboxgl.workerUrl to point to it. This would mean that the web workers would be loaded from that worker.js file rather than a blob URL. Pro: less complicated build setup. Con: we would have to publish two files--benchmarks.js and worker.js.

I was initially thinking that another downside to option 2 is that it would preclude being able to benchmark anything that included worker load time, because we'd be loading the worker in a different manner than in the shipped library. But in fact, I don't think it would really be practical to measure this within our benchmarking system regardless of how we load the workers, so I don't think this should be a factor.

@anandthakker anandthakker force-pushed the rollup-anandthakker branch 5 times, most recently from f2912dc to 28880a9 Compare March 1, 2018 21:13
@anandthakker
Copy link
Contributor Author

How would "dynamic imports to fetch modules on demand" work? I'm wondering if we'd end up building out infrastructure that would make it convenient to have mapbox-gl-rtl-text auto-load instead of requiring additional configuration. Even closer to the dream would be to dynamically choose whether to load a WASM or ASM.js version of the plugin based on browser capability.

@ChrisLoer I haven't dug into the details here, but I think dynamic imports might indeed make this a possibility. (But we'd likely have to also have to keep supporting the existing approach for the foreseeable future.)

You've been doing a lot of bechmarking of time-to-first-render, right? Can you document any of those numbers here?

Not much more than what's documented in #5939 -- I'll do another pass at this again soon, along with running our normal benchmarks, and post results here.

yarn.lock Outdated
dependencies:
chalk "~0.4.0"
underscore "~1.6.0"
version "2.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert this to fix the build (zaach/jsonlint#103 (comment)).

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally I think this is great, and we should move forward. It's a bit scary for sure, but ultimately feels like the right choice for future.

Apart from the remaining bullet points and making the build green, I'd love to add a bunch of eslint-plugin-import rules to our ESLint config — used this plugin and it is fantastic. Can help catch a lot of subtle bugs.

Also, would be great to fix a few remaining export const exported = { ... } cases, but I'm fine with leaving it for a follow-up PR if it makes it easier to ship.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants