misc(build): use rollup to build lighthouse-core bundles

[connorjclark]

ref #12689

Motivation: we need a build system that supports ES modules

Upfront: an alternative to this is to introduce Babel to our Browserify pipeline. However, not only would we then have babel, we'd also still have Browserify.

• Replace browserify with rollup in build-bundle.js
• Remove a handful of browserify plugin/transform dependencies, and duplicate their behavior w/ Rollup
• ~30KB reduction in bundle size (2%). I think it's less than expected because the node builtins included by Rollup plugin is more than what browserify does

Dynamic modules

This is probably the most complex bit

Since Rollup's whole concept doesn't support dynamic modules (it does not create a module system like Browserify bundling commonjs does; instead everything goes into one scope and module exports/imports are transformed into local scope references), we need a workaround for our config/audit/gatherer loading (config-helpers.js).

The solution is to replace dynamic require calls with requireWrapper, which will first look in a map of string -> module for a match before falling back to the real require.

/** @type {Map<string, any>} */
const bundledModules = new Map(/* BUILD_REPLACE_BUNDLED_MODULES */);

/**
 * Wraps `require` with an entrypoint for bundled dynamic modules.
 * See build-bundle.js
 * @param {string} requirePath
 */
function requireWrapper(requirePath) {
  return bundledModules.get(requirePath) || require(requirePath);
}

When not bundled, the map is empty, and this just defers to require.

In the Rollup config, rollup-plugin-replace is used to replace * BUILD_REPLACE_BUNDLED_MODULES */ with some programmatically code. Rollup treats this the same as if we wrote it in the original source file. The result is that bundled code will load dynamic modules from this map. It will still fallback to require if not found, which will throw an Error (no change in behavior there).

Resultant code:

Aliases

A couple of modules don't play nice with rollup-plugin-commonjs: debug and url.

For debug, I suspect the issue is the very complex entry module that jumps through a dozen hoops to set module.exports.

For url, IIRC the issue was something like Rollup was creating URL when we needed Url (or maybe vice verse). I sidestepped the issue by just directly aliasing url to lighthouse-core/lib/url-shim.js. I don't quite recall why url is being included in the bundle...

Shims

To ignore a module, rollup-plugin-shim (made w/ <3 by an amazing developer) can be used to replace the text of the module with export default {};

There was an issue with how Rollup resolved lighthouse within the pubads code–it was using Audit (to extend from) way before being defined. My hunch is that Rollup treated lighthouse-core/index.js as different from lighthouse in pubads (aka node_modules/lighthouse-plugin-publisher-ads/node_modules/lighthouse/...), even though it is yarn linked. It was resolved by shimming lighthouse with code that just re-exports Audit.

brfs

there is a rollup plugin for brfs but it is used a deprecated rollup api. I had to copy the code, un-TS it, and update the api hook it used.

brfs uses an old acorn that doesn't know esmodules, which means it must run before the commonjs plugin turns modules to esm. so ... even with all this work we are still blocked from using esmodules in our source code, although a single plugin is a much smaller roadblock. I didn't put any effort into figuring our workarounds for this (replacing brfs / updating brfs's parser).

EDIT: #13231 has fixed this part

[patrickhulce]

My initial impression is that there appear to be several more gotchas and issues with global scope than with our browserify system today.

I'm definitely a bit nervous since we typically don't discover bundled specific bugs until after LH makes its way to DevTools + PSI. The importance of our bundled smokes will be much greater after this :)

[patrickhulce]

psh, what a loser maintainer 😄

[patrickhulce]

yikes, this is going to be really annoying to remember. maybe we should tweak our evaluate API to support a keyed-object so we don't have to think about it?

or better yet always force deps to be function parameters? there are just too many gotchas these days with the .toString name reliance

[connorjclark]

I'm for it. link should do the trick.

[brendankenny]

IMO if we're going to keep the magically-interspersed Node and evaluate() browser code (which #10816 was a culmination of us deciding it was a lot nicer to go that way) we should embrace explicit import of functions and not require everything to live in page-functions (a lot of times it's a lot nicer just to have support functions defined locally).

This would allow type checking of functions we currently @ts-expect-error and make evaluated code more robust all without added ceremony. The downside is minifiers are still kind of dumb about keeping consistent minified functions names across modules even after bundling, but there are ways around that or we could just live with somewhat worse minification.

I have an old branch for this I can put up for discussion since I know there's a variety of opinions on each of these points :)

edit: up at #12795

[connorjclark]

(...expanding on the fact that custom page functions outside page-functions.js exists ...)

My linked idea (having a pageFunctions namespace) can support custom functions like this:

const pageFunctions = {
  ...require('../../lib/page-functions.js'),
  myFnDeclaredHere,
};

function myFnDeclaredHere() { }

// ...

function getMetaElements() {
  return pageFunctions.getElementsInDocument('head meta').map(el => {
    const meta = /** @type {HTMLMetaElement} */ (el);
    pageFunctions.myFnDeclaredHere(); // also i exist.
    return {
      name: meta.name.toLowerCase(),
      content: meta.content,
      property: meta.attributes.property ? meta.attributes.property.value : undefined,
      httpEquiv: meta.httpEquiv ? meta.httpEquiv.toLowerCase() : undefined,
      charset: meta.attributes.charset ? meta.attributes.charset.value : undefined,
    };
  });
}

const code = pageFunctions.createEvalCode(getMetaElements, {
      deps: [
        pageFunctions.getElementsInDocument,
		pageFunctions.myFnDeclaredHere,
      ],
    });

// `pageFunctions.createEvalCode` would inject a var `pageFunctions` with all values passed to deps, using the `.name` as keys

in theory (plus removing the *String usages), that should keep the mangled names the same

[connorjclark]

#12795 has not been resolved–is this blocking for this PR?

[patrickhulce]

aw, look who's pipeing now :)

[connorjclark]

don't be fooled, i copy/pasted this.

[patrickhulce]

seems like we're not using all of these features? was this copied from somewhere or did you need them in a previous version?

[connorjclark]

I copy/pasted it. I'll add a link to the original source.

[patrickhulce]

was this fixed already?

[connorjclark]

for computed artifacts the exact name doesn't matter, so I didn't bother. only impacts the log message https://github.com/GoogleChrome/lighthouse/blob/f90efbf/lighthouse-core/computed/computed-artifact.js#L30 . Could do the same name trimming here if you like.

[connorjclark]

I'm definitely a bit nervous since we typically don't discover bundled specific bugs until after LH makes its way to DevTools + PSI. The importance of our bundled smokes will be much greater after this :)

yarn test-bundle and yarn test-devtools (plus manual testing with yarn open-devtools) caught a handful of issues, mostly page function mangling issues. As long as we keep up our practice of putting audits in smoke tests, I feel good about it.

[brendankenny]

it looks like there's some stuff that might need another pass to make sure it'll work ok. We don't want to include the locales in the dt bundle like this, for instance

[connorjclark]

Tried it out in LR, oddly I get Buffer not defined errors. Odd because I'd expect the same in the bundled smoke tests yet those are fine.

[paulirish]

whats up with these dolla bills?

[connorjclark]

#12771 (comment)