Replace Browserify with Rollup #3810

robertknight · 2021-10-06T17:46:08Z

Replace Browserify with Rollup as a module bundler. This brings several
advantages:

It is a more modern bundler that has better support in the ecosystem,
more active maintenence, fewer dependencies and is easier to write plugins for.
The core team also maintain most of the plugins that we need.
It has native support for modern platform features like ES modules,
including dynamic import for lazy-loading of functionality.
It generates smaller bundles due to its ability to do tree shaking and scope hoisting.

The new bundles are generated as ES modules rather than UMD / IIFE bundles.
Our target browsers now all support ES modules and this will enable us to use
native static/dynamic imports for code splitting or lazy-loading of
functionality in future.

The initial Rollup build generates one bundle per application (boot script,
annotator, sidebar). This simplifies the build process and minimizes production
bundle size, at the cost of making incremental updates during development
slightly slower.

Build performance is currently similar to or a little slower than Browserify.
We can optimize this by adding caching of transforms (mainly Babel) and
pre-building of a vendor bundle containing large dependencies.

As part of changing the bundler this also changes how the code is packaged
for tests. We now do the bundling ourselves rather than using a Karma
integration. This makes it much easier to inspect the processed code and fix
problems. It also makes it easier to optimize the bundle building in future.

Summary of changes:

Add Rollup and required plugins to dependencies
Remove Browserify dependencies
Add rollup.config.js config file that specifies how to build the app bundle, and rollup-tests.config.js that specifies how to build the test bundle.
Change gulpfile.js to use Rollup's JS API to build bundles instead of Browserify
Remove the Browserify-related sections of karma.config.js. The equivalent functionality is provided by buildAndRunTests in gulpfile.js and rollup-tests.config.js.
Add karma-source-map-support Karma plugin to enable error messages to show original source locations for any errors.
Change the boot script to load the annotator / sidebar bundles as ES modules (<script type="module" ...>) rather than classic scripts
Remove files from gulp/scripts/* that were used to generate the Browserify bundle
Move a couple of JSX expressions that were not inside a component to be inside a named function or component. This works around an issue with the way Babel processes JSX in development builds

robertknight · 2021-10-06T17:57:34Z

Prod bundle size before:

[I]  ~/h/client > exa --long --no-permissions --no-user --no-time build/scripts/ | grep '\.js$'
153k annotator.bundle.js
5.8k boot.bundle.js
260k katex.bundle.js
113k sentry.bundle.js
 76k showdown.bundle.js
340k sidebar.bundle.js

Where the sidebar app consists of the sidebar, sentry, katex and showdown bundles (340 + 113 + 260 + 76 = 789k)

Prod bundle size after:

[I]  ~/h/client > exa --long --no-permissions --no-user --no-time build/scripts/ | grep '\.js$'
114k annotator.bundle.js (-25%)
4.3k boot.bundle.js
651k sidebar.bundle.js (-17% of total sidebar app size)

Where the sidebar app is contained in only one bundle.

robertknight · 2021-10-06T18:04:54Z

I've seen this error sometimes happen on a clean build:

[19:02:50] Error: File not found with singular glob: /Users/robert/hypothesis/client/build/scripts/boot.bundle.js (if this was purposeful, use `allowEmpty` option)
    at Glob.<anonymous> (/Users/robert/hypothesis/client/node_modules/glob-stream/readable.js:84:17)
    at Object.onceWrapper (node:events:510:26)
    at Glob.emit (node:events:390:28)
    at Glob.emit (node:domain:475:12)

This is harmless because the task gets re-run automatically after the file is generated. But we should still fix it.

robertknight · 2021-10-06T18:08:45Z

One small quality of life improvement in this PR is that the make dev output is now clearer about when the JS build is completed, as opposed to just writing a log entry after individual bundles are created and relying on the developer to recognize when they are all done.

$ make dev
...
[19:05:25] Finished 'build-css' after 1.86 s
[19:05:25] Starting 'watchCSS'...
[19:05:26] JS build starting...
[19:05:28] Finished 'build-fonts' after 4.12 s
[19:05:28] Starting 'watchFonts'...
[19:05:29] Starting 'updateManifest'...
[19:05:32] Sidebar app URL: {current_scheme}://{current_host}:5000/app.html
[19:05:32] Notebook app URL: {current_scheme}://{current_host}:5000/notebook
[19:05:32] Client asset root URL: {current_scheme}://{current_host}:3001/hypothesis/1.0.0-dummy-version/
[19:05:32] Finished 'updateManifest' after 2.77 s
[19:05:35] Starting 'updateManifest'...
[19:05:36] JS build completed.
[19:05:36] Finished 'watch-js' after 12 s

It would still be good to further prune the output in future and reduce the amount of noise.

codecov · 2021-10-06T18:10:13Z

Codecov Report

Merging #3810 (ee6fbf8) into master (910b3da) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3810   +/-   ##
=======================================
  Coverage   99.02%   99.02%           
=======================================
  Files         211      211           
  Lines        7797     7797           
  Branches     1757     1757           
=======================================
  Hits         7721     7721           
  Misses         76       76

Impacted Files	Coverage Δ
src/boot/boot.js	`100.00% <100.00%> (ø)`
src/sidebar/components/Menu.js	`100.00% <100.00%> (ø)`
src/annotator/selection-observer.js	`97.29% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 910b3da...ee6fbf8. Read the comment docs.

robertknight · 2021-10-11T08:37:50Z

gulpfile.js

@@ -215,23 +157,8 @@ gulp.task(
  })
 );

-const imageFiles = 'src/images/**/*';


The build-images task has been removed because all of the images are SVGs embedded in the JS bundles as strings. This was already the case with the Browserify build. I'm just removing obsolete logic.

robertknight · 2021-10-11T08:39:09Z

gulpfile.js

+  // should always have been generated first.
+  if (usingDevServer && !existsSync(bootBundle)) {
+    return;
+  }


It might have been technically possible for this issue to have occurred previously with Browserify, but I never observed it in practice when running make clean; make dev. I did however see it sometimes with Rollup.

robertknight · 2021-10-11T08:40:28Z

package.json

  "prettier": {
    "arrowParens": "avoid",
    "singleQuote": true
  },
-  "browser": {
-    "fetch-mock": "./node_modules/fetch-mock/cjs/client.js"


We don't need this any more because Rollup will use the ES module version of fetch-mock.

robertknight · 2021-10-11T09:10:19Z

I did some experiments with adding caching at various points in the build process. What I learned is that there are worthwhile wins to be had from minimizing or caching:

Babel transformation by @rollup/plugin-babel
The JS parsing that Rollup does using Acorn
The work that @rollup/plugin-commonjs does to support non-ES module dependencies

Of the above, the Babel transform looks like it will be simplest to do. Not sure about the others yet.

Also we could look at what we did with Browserify where large vendor dependencies were bundled separately in development. In production building vendor dependenceis + app code together maximizes the effectiveness of tree-shaking.

I will look at these after the initial conversion lands.

robertknight · 2021-10-11T09:35:21Z

This is almost ready for review / testing now. Remaining things to do:

Build the boot script as a classic (non-module) script, since there are many existing embeds out there that load it as a classic script (ie. not using type="module" on the <script> tag) [DONE]
Test that dev and prod builds work in our oldest supported browsers (Safari 11, Chrome 70, Firefox 70) [DONE]

robertknight · 2021-10-11T10:24:32Z

I haven't yet added the polyfill bundles to the Rollup config. It turns out that we don't currently need them, as none of our supported browsers need any polyfills. The most recent thing we use is Promise.prototype.finally, and all of our target browsers support that. However I do think we want to keep the basic infrastructure around so that we can add new polyfills easily in future.

lyzadanger · 2021-10-11T11:57:00Z

@robertknight I've read the description and thread here but have not yet reviewed code or tested (will do shortly). One thing that jumped out at me:

The new bundles are generated as ES modules rather than UMD / IIFE bundles.
Our target browsers now all support ES modules...

A related comment:

Test that dev and prod builds work in our oldest supported browsers (Safari 11, Chrome 70, Firefox 70) [DONE]

Our current failure mode in unsupported browsers could be described as "soft." IIRC, we actually render a nice message telling the user that their browser is out of date and provide a link to help docs.

What's the failure mode going forward for unsupported browsers?

robertknight · 2021-10-11T11:59:08Z

What's the failure mode going forward for unsupported browsers?

That's the same as before. The only code that runs in unsupported browsers is the boot script, and that's compiled to a classic (non ES-module) script. The current behavior in the client is just to log a console warning. The LMS app does display a UI message and we should be able to keep that.

lyzadanger

At this stage, mostly lots of questions! I tried to do some background reading where I could.

Main observations:

In general, rollup config is straightforward to read and understand, and their docs seem solid. I appreciate that they manage their own core plugins, as that makes documentation consistent.
My biggest challenge with this set of changes is the test-build configuration. I'm having trouble following it (more specific questions inline).

lyzadanger · 2021-10-11T11:58:13Z

.babelrc

@@ -17,7 +17,6 @@
    ]
  ],
  "plugins": ["inject-args"],
-  "ignore": ["**/vendor/*"],


Can you help me understand this change?

We used to have some third-party dependencies in a local vendor directory in the source tree. This no longer exists.

lyzadanger · 2021-10-11T11:59:29Z

.babelrc

-            // Use `preact/compat/jsx-dev-runtime` which is an alias for `preact/jsx-runtime`.
-            // See https://github.com/preactjs/preact/issues/2974.
-            "importSource": "preact/compat"


Reading the linked bug description, sounds like this workaround was because of a shortcoming in Browserify so it makes sense we wouldn't need it anymore...

That's correct, it is not needed anymore.

lyzadanger · 2021-10-11T12:01:30Z

.eslintrc

@@ -16,5 +16,12 @@

    // Replaced by TypeScript's static checking.
    "react/prop-types": "off"
-  }
+  },
+  "overrides": [


TIL: ESLINT overrides. For when you need to provide different (potentially conflicting) settings to a particular glob (subset) of files.

Is this set of overrides potentially common/reusable for modules that run in a Node.js context vs. the browser? Or is there something particularly special about rollup configs?

Rollup configs are written as ES modules, but currently ESLint expects is configured to expect files outside of the src/ dir to be non-ES modules (eg. CommonJS). If we make everything an ES module we could remove this.

lyzadanger · 2021-10-11T12:02:59Z

gulpfile.js

 const gulp = require('gulp');
 const replace = require('gulp-replace');
 const rename = require('gulp-rename');
+const rollup = require('rollup');
+const loadConfigFile = require('rollup/dist/loadConfigFile');


Huh. Is this a typical pattern with rollup/part of the documented API?

Yes. See https://rollupjs.org/guide/en/#programmatically-loading-a-config-file.

gulpfile.js

lyzadanger · 2021-10-11T12:51:07Z

rollup-tests.config.js

+    string({
+      include: '**/*.{html,svg}',
+    }),


Blurb from this plugin is that it "converts text files to modules." Can you help me grok what it does for us in terms of building bundles for test?

This enables import someIcon from 'path/to/icon.svg' statements to work. Same for .html files.

lyzadanger · 2021-10-11T12:51:40Z

rollup.config.js

+  // Eliminate debug-only imports.
+  prodPlugins.push(
+    virtual({
+      'preact/debug': '',
+    })
+  );
+}


How would be become aware of (additional) debug-only imports in the future?

updated: I see a helpful comment in annotator/index related to this.

lyzadanger · 2021-10-11T12:52:59Z

rollup.config.js

+      nodeResolve(),
+      commonjs(),
+      string({
+        include: '**/*.svg',


No .html here vs. in test config...curious.

Only some HTML fixtures in the tests need to import .html files as strings.

lyzadanger · 2021-10-11T12:53:30Z

src/annotator/index.js

-if (process.env.NODE_ENV !== 'production') {
-  require('preact/debug');
-}
+// Enable debug checks for Preact. Removed in prod builds by Rollup config.


Helpful comment for future, thanks!

lyzadanger · 2021-10-11T12:55:57Z

src/sidebar/components/Menu.js

+    <svg className={classnames('Menu__arrow', className)} width={15} height={8}>
+      <path d="M0 8 L7 0 L15 8" stroke="currentColor" strokeWidth="2" />
+    </svg>


Orthogonal, but is there a counter-argument to eventually replacing this SVG with an SVGIcon component?

This is a bit simpler and faster, but not so easy to update as an SVGIcon. We can change if needed in future.

esanzgar · 2021-10-11T14:37:35Z

gulpfile.js

-);
+//
+// Some (eg. a11y) tests rely on CSS bundles. We assume that JS will always take
+// londer to build than CSS, so build in parallel.


Suggested change

// londer to build than CSS, so build in parallel.

// longer to build than CSS, so build in parallel.

Replace Browserify with Rollup as a module bundler. This brings several advantages: - It is a more modern bundler that has better support in the ecosystem, more active maintenence, fewer dependencies and is easier to write plugins for. The core team also maintain most of the plugins that we need. - It has native support for modern platform features like ES modules, including dynamic import for lazy-loading of functionality. - It generates smaller bundles due to its ability to do tree shaking and scope hoisting. The new bundles are generated as ES modules rather than UMD / IIFE bundles. Our target browsers now all support ES modules and this will enable us to use native static/dynamic imports for code splitting or lazy-loading of functionality in future. The initial Rollup build generates one bundle per application (boot script, annotator, sidebar). This simplifies the build process and minimizes production bundle size, at the cost of making incremental updates during development slightly slower. Build performance is currently similar to or a little slower than Browserify. We can optimize this by adding caching of transforms (mainly Babel) and pre-building of a vendor bundle containing large dependencies. As part of changing the bundler this also changes how the code is packaged for tests. We now do the bundling ourselves rather than using a Karma integration. This makes it much easier to inspect the processed code and fix problems. It also makes it easier to optimize the bundle building in future.

See hypothesis/browser-extension#681

Fix an occasional error from Gulp about a missing `boot.bundle.js` file when running `make dev` for the first time after `make clean`. This can happen if `updateManifest` runs before Rollup has generated `boot.bundle.js`. The error can be safely ignored as `updateManifest` will be re-run after `boot.bundle.js` is generated.

Make conditional exclusion of modules work the same way in the app bundle as in the tests bundle.

Since the exclusion is no longer triggered by logic in the source code, add a note about where this happens.

This prevents a warning from Rollup about use of `this` outside a class or (non-arrow) function in development builds, due to the way Babel compiles JSX expressions [1]. Moving top-level code into a function is also useful in case we ever want to write tests for it. [1] babel/babel#9149

Due to pre-existing `<script>` tags on third-party websites that load the client's boot script, via https://hypothes.is/embed.js, we need to keep generating it as a classic rather than ES module script for now.

Add comments to clarify a few things about the way the tests are built and run.

robertknight · 2021-10-11T15:59:37Z

I have rebased this on master and added a commit with some comments to clarify aspects of how the test bundle is built and run.

`sinon` is exposed as a global in the test environment so it doesn't need to be imported. Additionally some of the dependencies of Sinon (samsam, nise) cause warnings about circular dependencies and use of `eval` when bundled by Rollup.

robertknight · 2021-10-12T11:13:47Z

There is a test that is running slowly on this branch:

Chrome Headless 93.0.4577.0 (Mac OS 10.15.7) SLOW 1.158 secs: PortRPC-Bridge integration errors and timeouts no longer sen
ds RPC messages to a Bridge channel that has received an error

It turns out to be caused by attempting to return an Error from a Bridge method via the callback. If the Error in that test is replaced with a { message: <string value> } object, the test runs quickly.

Update: The slow execution of this test is something to do with the karma-source-map-support plugin that we use to map Error stack traces back to their original locations in this branch. Removing the source-map-support plugin from the frameworks key in karma.config.js makes the problem go away. That functionality is important though for test debugging. Given that serializing errors is not supported across browsers, we'll need to make changes here anyway. On this branch I think I'll change the error value to be Error-like (ie. { message: string }) rather than an actual Error.

Looking into this further, I found that attempting to send an Error via a MessagePort (using MessagePort.postMessage) succeeds in Chrome but fails in Firefox and Safari. According to https://bugzilla.mozilla.org/show_bug.cgi?id=1556604 the native Error types are supposed to support structured cloning, but only Chrome currently implements this. The method for serializing an Error is described in Step 17 of https://html.spec.whatwg.org/multipage/structured-data.html#structuredserializeinternal. Basically it checks if the Error is one of the native Error types and if it is, preserves the constructor, name and message properties. Otherwise it throws an error.

Since passing an Error the Bridge callback is such an obvious thing to try to do, I think it would make sense for us to handle this in some way. We could either implement the native Error-cloning behavior, or implement some variation (eg. one that copies the message property but nothing else).

This fixes a slow test on this branch, which appears to be caused by an interaction between using `postMessage` to serialize native Error objects and the `source-map-support` Karma plugin that we use. Sending native Errors via `PortRPC` currently doesn't work in all browsers so I expect we'll want to implement our own serialization of errors in `PortRPC` to work around that. That can also work around the `source-map-support`/`postMessage` issue in Chrome in future. To move the Browserify => Rollup conversion forwards, this commit just changes the test to use an Error-like rather than actual Error object.

The CommonJS plugin is the second most expensive transform after Babel. Since all of our own code uses ES modules, limit it to npm dependencies to speed up the build. In tests the Babel transform has also been moved last so that additional code it generates is not unnecessarily processed by subsequent plugins. Combined these changes speed up a build of the full test bundle by ~4s.

robertknight · 2021-10-12T12:32:52Z

Some performance metrics for building the test bundle:

Current full test bundle build time: 22s
... + pre-bundling npm dependencies: 16s (Building a separate bundle containing all dependencies and then marking them as external in the main bundle)
... + disabling code coverage plugin: 10s (Caching the Babel transform would have a similar or greater effect)

This is not especially surprising since 90% of the test bundle is dependencies. In modules that have code coverage enabled, the inserted code increases the total volume of code a lot and that has consequences for subsequent steps.

This should make it easier to speed up test runs in future by pre-building npm dependencies into a separate bundle which the main test bundle can import from.

Change the LMS frontend to use Rollup for bundling JS. This change was done for the same reasons and using a similar approach to the corresponding change in the Hypothesis client. See hypothesis/client#3810.

robertknight · 2021-10-14T05:20:11Z

One remaining warning I am aware of is this message when running make dev:

(node:21339) [DEP0148] DeprecationWarning: Use of deprecated folder mapping "./dist/" in the "exports" field module resolution of the package at /Users/robert/hyp
othesis/client/node_modules/rollup/package.json.
Update this package.json to use a subpath pattern like "./dist/*".
(Use `node --trace-deprecation ...` to show where the warning was created)

A fix for this issue was proposed upstream but later reverted. I tried to restart the discussion with a proposed fix at rollup/rollup#3896 (comment).

lyzadanger

I think we're ready to move forward on this! One thing that would be helpful is if you could summarize all the need-to-know stuff for devs (e.g. the slow test, the remaining warning) so we have a quick reference of what to expect.

robertknight · 2021-10-15T13:19:27Z

One thing that would be helpful is if you could summarize all the need-to-know stuff for devs (e.g. the slow test, the remaining warning) so we have a quick reference of what to expect.

Sure. The slow test was fixed, so I think the most relevant remaining things are:

You will see a warning ([DEP0148] DeprecationWarning: ...) when running make dev in the most recent version of Node. I'm trying to talk with upstream to get this resolved.
When running make dev you will see JS build starting... and JS build completed messages in the log output. The "JS build completed" message is the signal that the client is built and ready to use.
You'll see the same messages as (2) whenever you make changes.
Running tests works the same as before, but the sequence of messages is a bit different. You'll see Building test bundle... ($N files) at the start, where $N is either the total number of files or the number matching the --grep pattern, if specified.

Change the LMS frontend to use Rollup for bundling JS. This change was done for the same reasons and using a similar approach to the corresponding change in the Hypothesis client. See hypothesis/client#3810.

robertknight commented Oct 11, 2021

View reviewed changes

robertknight marked this pull request as ready for review October 11, 2021 09:57

lyzadanger reviewed Oct 11, 2021

View reviewed changes

esanzgar reviewed Oct 11, 2021

View reviewed changes

robertknight added 10 commits October 11, 2021 16:59

Create build/scripts dir if necessary before writing to it

349d861

Enable source-mapped error stacktraces in tests

04a679f

See hypothesis/browser-extension#681

Use @rollup/plugin-virtual to exclude modules in production

6f69553

Make conditional exclusion of modules work the same way in the app bundle as in the tests bundle.

Add note about preact/debug exclusion in prod builds

6bd24b5

Since the exclusion is no longer triggered by logic in the source code, add a note about where this happens.

Ensure warnings during build are displayed in console

756c74b

Build boot script as classic rather than module script

28283ff

Due to pre-existing `<script>` tags on third-party websites that load the client's boot script, via https://hypothes.is/embed.js, we need to keep generating it as a classic rather than ES module script for now.

Add some clarifying comments

89910bc

Add comments to clarify a few things about the way the tests are built and run.

robertknight force-pushed the rollup branch from 4321810 to 89910bc Compare October 11, 2021 15:59

Remove unnecessary sinon import

51d5aed

`sinon` is exposed as a global in the test environment so it doesn't need to be imported. Additionally some of the dependencies of Sinon (samsam, nise) cause warnings about circular dependencies and use of `eval` when bundled by Rollup.

Convert test bundle to be an ES module

ee6fbf8

This should make it easier to speed up test runs in future by pre-building npm dependencies into a separate bundle which the main test bundle can import from.

robertknight mentioned this pull request Oct 12, 2021

Replace Browserify with Rollup hypothesis/lms#3221

Merged

lyzadanger approved these changes Oct 15, 2021

View reviewed changes

robertknight merged commit 53da403 into master Oct 15, 2021

robertknight deleted the rollup branch October 15, 2021 13:19

	// londer to build than CSS, so build in parallel.
	// longer to build than CSS, so build in parallel.

Replace Browserify with Rollup #3810

Replace Browserify with Rollup #3810

Conversation

robertknight commented Oct 6, 2021 • edited

robertknight commented Oct 6, 2021 • edited

robertknight commented Oct 6, 2021

robertknight commented Oct 6, 2021

codecov bot commented Oct 6, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight commented Oct 11, 2021

robertknight commented Oct 11, 2021 • edited

robertknight commented Oct 11, 2021

lyzadanger commented Oct 11, 2021

robertknight commented Oct 11, 2021 • edited

lyzadanger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight commented Oct 11, 2021

robertknight commented Oct 12, 2021 • edited

robertknight commented Oct 12, 2021 • edited

robertknight commented Oct 14, 2021

lyzadanger left a comment

Choose a reason for hiding this comment

robertknight commented Oct 15, 2021

robertknight commented Oct 6, 2021 •

edited

robertknight commented Oct 6, 2021 •

edited

codecov bot commented Oct 6, 2021 •

edited

robertknight commented Oct 11, 2021 •

edited

robertknight commented Oct 11, 2021 •

edited

robertknight commented Oct 12, 2021 •

edited

robertknight commented Oct 12, 2021 •

edited