misc(build): refactor viewer bundler into reusable GhPagesApp

[connorjclark]

ref https://github.com/GoogleChrome/lighthouse/pull/11545/files#r503644206

• Provides interface for bundling an app for gh pages
• Uses simpler synchronous methods (instead of promisfying everything)
• removed service worker from viewer (doesn't do anything, no plan to add functionality, and I think it messed with my local dev every now and then...)

Next immediate use will be treemap app. If we ever port the scorecalc to this repo, it'd use this code too.

[patrickhulce]

this will be great for sourcemap app thanks :)

LGTM % few Qs

[patrickhulce]

we had a serviceworker? 😆

[patrickhulce]

LGTM

[patrickhulce]

Vercel doesn't seem to like this though, and I can't pull up the details. Did something break there with our viewer deployment?

[brendankenny]

Can you add some documentation here? It's a definite loss readability-wise otherwise. The current file is simple and more or less self contained. This new version drops any internal comments because the viewer-specific gotchas don't apply, but as a result it's less clear what's going on to build viewer and if you're writing a new "GhPagesApp", there are a bunch of implicit things going on. e.g. where do you put the main html file? (implicit vs the explicit css and js). Why are some js files included by default (and browserified) and not others? etc

[brendankenny]

(to answer the earlier DRY question, I'm a big fan of the rule of three, personally (master is currently at one gh-pages build file), and if Patrick wants to move to rollup he should just say so :P, but this is fine with some documentation so I don't have to read two build files and open devtools just to figure out what files are going to be included in the bundle :)

[patrickhulce]

If we ever port the scorecalc to this repo, it'd use this code too.

and CPU throttling calc :)

if Patrick wants to move to rollup he should just say so

Yes, for the record again, I think it would be better if we used any modern bundler that's maintained (rollup, webpack, parcel) instead of rolling our own in multiple places. Asking @connorjclark to pickup that effort for just treemap though seemed unreasonable. In my judgement I don't view this to be a loss to readability (it actually seemed to be an improvement IMO even if we have no additional copies at all. Viewer now calls something resembling a bundler API without needing to understand the implementation of a bundler just to make sense of the script). I'm sorry if everyone else on the team views this as wasted effort, but I do appreciate it greatly and think it's a win :)

[brendankenny]

Viewer now calls something resembling a bundler API without needing to understand the implementation of a bundler just to make sense of the script

But that's the point I was making, you do still have to understand the implementation, just now it's in two different places and there are fewer comments :) Some scripts are manually added, some are pulled automatically, some are browserified, some are concatted, html just appears, etc.

[connorjclark]

Instead of automatically/magically adding sources (like adding app/src/*.js to the js), could instead do more explicit:

const app = new GhPagesApp({
    name: 'viewer',
    appDir: `${__dirname}/../lighthouse-viewer/app`,
    htmlReplacements: {
      '%%LIGHTHOUSE_TEMPLATES%%': htmlReportAssets.REPORT_TEMPLATES,
    },
    html: 'index.html', // ----------- NEW CODE ----------------
    javascripts: [
      await generatorJsPromise,
      htmlReportAssets.REPORT_JAVASCRIPT,
      fs.readFileSync(require.resolve('idb-keyval/dist/idb-keyval-min.js'), 'utf8'),
      {path: 'src/*'}, // ----------- NEW CODE ----------------
    ],
    stylesheets: [
      htmlReportAssets.REPORT_CSS,
      {path: 'styles/*'}, // ----------- NEW CODE ----------------
    ],
    assetPaths: [
      'images/**/*',
      'manifest.json',
    ],
  });

paths would use appDir as root. and be glob-able. Better?

[brendankenny]

nice, yeah, that does seem like a good change

[connorjclark]

cool, that works out a lot better.

[brendankenny]

it makes more sense to me to make all four of these Array<{path: string} | string> if some of them are going to go this way, but it's not a huge deal

[connorjclark]

Can't do it for assetPaths without having some way to interpret a literal string as an asset. {path: string}[] might work?

Concating an array of html files doesn't make sense, but perhaps {path: string} | string could be allowed?

[brendankenny]

Oh right, cause assets are just copied over, nothing new is created.

Yeah, maybe having them be be {path: string} helps self document by making them consistent with the others, but it also makes sense how you have it here.

[brendankenny]

jsdocs for these (especially the ones with the options like this) please? :)