report: extract independent report types

[brendankenny]

Isolates report/ as a typescript "project" allowing it to live off in its own type world. This allows it to be browser specific and not bring in the entire lighthouse-cli/lighthouse-core/etc world just to type check the report.

Dependencies are simpler and (because of the expanded -b mode) more compilation caching occurs which means faster follow-up type checks (e.g. if you're working on core and haven't touched the report, the yarn tsc -b report/ part will finish in 200ms because the cached .tmp/tsbuildinfo file will be newer than all the files in report/).

The bulk of this PR is splitting up a few more d.ts files for e.g. the parts of html-renderer.d.ts that treemap and viewer need vs the parts they don't.

I'll follow this PR with changes to make treemap/, viewer/, and flow-report/ to do the same to them. This will be especially helpful for treemap and viewer since they recompile large parts of core due to transitive type deps. There will also probably need to be a final clean up pass to better use tsconfig inheritance and some documentation.

[brendankenny]

added a new eslintrc for report/ to make it env browser and remove the ad hoc /* global document window */ and /* eslint-env browser */s sprinkled throughout some of the files.

Also, turns out eslintrc files automatically extend any other eslintrc files up through to the root directory, though I added a root: true in the root file just to make it clear

[brendankenny]

the use of Buffer in text-encoding.js prevents getting rid of node types for these files. I was tempted to write a little type shim just for Buffer to keep out @types/node, but it seemed a bit too fragile. Maybe worth it at some point

[brendankenny]

the use of Buffer in text-encoding.js prevents getting rid of node types for these files. I was tempted to write a little type shim just for Buffer to keep out @types/node, but it seemed a bit too fragile. Maybe worth it at some point

trying out viewer types on top of this, I think we actually do want a little Buffer shim. The node types infect everything (we end up with errors on setTimeout return types, for instance, if viewer doesn't also import @types/node, which is just getting silly by that point)

[brendankenny]

I didn't realize generator/* files would be included even though generator/ is in references and has its own tsconfig. Having to explicitly exclude these is one more argument not to nest generator under report, I think (see other options in #12940 (comment)).

[patrickhulce]

what's the force module situation these days such that this isn't necessary? I feel like I remember reading a PR description or comment about this recently, but couldn't find it quickly.

[brendankenny]

what's the force module situation these days such that this isn't necessary? I feel like I remember reading a PR description or comment about this recently, but couldn't find it quickly.

it's kind of dumb: if there's any import or export statements, the file is a module (which is why we had the empty ones in there before when everything was in one or two files and we didn't have any imports), and if you want an explicit declare global {} block in there it has to be a module, otherwise it's ambient and everything is a global type.

Fortunately(?) we almost always have import statements these days, and the error message is a lot better, basically saying you can't declare global unless this file is a module.

I vaguely recall a few years back in the es modules transition wars the idea of using that same heuristic to automatically detect if a file was an .mjs vs .cjs file without a file extension/package.json type entry, and then everyone realized that would be super fragile and annoying and decided not to go that way. But here it is anyways :)

[patrickhulce]

LGTM

[patrickhulce]

Oh interesting I wouldn't have expected export import to be needed since we're reassigning.

does this differ from export type ReportResult = _ReportResult; or because it's a module we can't do that and export import is the only valid way?

[brendankenny]

Oh interesting I wouldn't have expected export import to be needed since we're reassigning.

does this differ from export type ReportResult = _ReportResult; or because it's a module we can't do that and export import is the only valid way?

yeah, you get the interface but not the module, so e.g. LH.ReportResult.AuditRef won't resolve

[patrickhulce]

I think this starts to illustrate how the boundaries are hard to see here (glancing at this without context, I wouldn't expect LH.Audit to be within the self-contained report context

Long-term do you plan to renamespace the report to LHReport or LH.Report. or similar?

[brendankenny]

I think this starts to illustrate how the boundaries are hard to see here (glancing at this without context, I wouldn't expect LH.Audit to be within the self-contained report context

Long-term do you plan to renamespace the report to LHReport or LH.Report. or similar?

Yeah, it's a really good question. I'm not sure. I defaulted to maintaining the familiar LH.* hierarchy, but in the report files, the LH.Audit namespace is empty except for Details:

module LH {
  module Audit {
     export import Details = AuditDetails;
  }
}

https://github.com/GoogleChrome/lighthouse/pull/12946/files#diff-2a34e64f4e5c4c358613feaff0fbc2da80d47238fb89ca37fdc07c96ce8e1c2cR22-R24

so seems doubly weird to maintain it? Not that useful in the report if considered in isolation, and if the goal is consistency across files, it's actually misleading because you might think e.g. LH.Audit.ScoreDisplayMode is going to be in there too.

So long term it may make sense to re work the names, but then it's asking everyone to keep track of multiple names for the same thing...

[brendankenny]

Maybe if microsoft/TypeScript#41825 is fixed, core could keep the global types and the other areas with fewer files and fewer type needs could just do

/** @typedef {import('../../lhr/audit-details')} AuditDetails */

export class DetailsRenderer {
  /**
   * @param {DOM} dom
   * @param {{fullPageScreenshot?: AuditDetails.FullPageScreenshot}} [options]
   */
  constructor(dom, options = {}) {
    this._dom = dom;
    this._fullPageScreenshot = options.fullPageScreenshot;
  }
}

Without that issue fixed, though, we'd be stuck with

/** @typedef {import('../../lhr/audit-details').FullPageScreenshot} FullPageScreenshot */
/** @typedef {import('../../lhr/audit-details').OpportunityColumnHeading} OpportunityColumnHeading */
/** @typedef {import('../../lhr/audit-details').TableColumnHeading} TableColumnHeading */
/** @typedef {import('../../lhr/audit-details').List} List */
/** @typedef {import('../../lhr/audit-details').NodeValue} NodeValue */
// ...

:(