report: extract independent report-generator types #12940
Conversation
brendankenny
requested review from
connorjclark
and removed request for
a team
There was a problem hiding this comment.
@adamraine this is going to clash with #12929, but it's just directory renames, nothing major. If that's going to land soon I'm happy to fix over here.
But also feel free to weigh in on directory naming/structure since you're adding files to these locations right now :)
| const FLOW_REPORT_TEMPLATE = fs.readFileSync(`${LH_ROOT}/flow-report/assets/standalone-flow-template.html`, 'utf8'); | ||
| const FLOW_REPORT_JAVASCRIPT = fs.readFileSync(`${LH_ROOT}/dist/report/flow.js`, 'utf8'); | ||
| const FLOW_REPORT_TEMPLATE = fs.readFileSync(`${__dirname}/../../flow-report/assets/standalone-flow-template.html`, 'utf8'); | ||
| const FLOW_REPORT_JAVASCRIPT = fs.readFileSync(`${__dirname}/../../dist/report/flow.js`, 'utf8'); |
There was a problem hiding this comment.
@connorjclark sorry, these had to move off using root.js because that would expose generator code to all the dependencies of the root tsconfig.json (really root.js is a leaf file as well for the main tsconfig, but tsc isn't smart enough to allow a simple case like that).
The report assets won't ever have that many paths to look up, so we could use import.meta.url just for these files, we could make a root.js copy for generator code, or we could (eventually?) do the "shared" directory idea from above and put root.js in there. This seemed ok for now since report-assets.js still uses __dirname too.
| @@ -7,6 +7,9 @@ | |||
|
|
|||
| const htmlReportAssets = require('./report-assets.js'); | |||
|
|
|||
| /** @typedef {import('../../types/lhr/lhr').default} LHResult */ | |||
| /** @typedef {import('../../types/lhr/flow').default} FlowResult */ | |||
There was a problem hiding this comment.
we could make a globals d.ts file for the report generator too, but it didn't seem worth it for just these two types. I don't feel strongly either way, though
I don't expect it to land soon, but thanks for the heads up. |
brendankenny
force-pushed
the
report-generator.d.ts
branch
from
0076190 to
9c5d5b4
Compare
report/generator/README.md
Outdated
|
|
||
| Lighthouse's report generator is the entry point for creating reports from an **LHR** (Lighthouse Result object). It returns results as HTML, JSON, and CSV. | ||
|
|
||
| It runs natively in Node.js but can run in the browser after a compile step is applied during our bundling pipeline. That compile step uses a browserify transform that takes any `fs.readFileSync()` calls and replaces them with the stringified file content. |
There was a problem hiding this comment.
| It runs natively in Node.js but can run in the browser after a compile step is applied during our bundling pipeline. That compile step uses a browserify transform that takes any `fs.readFileSync()` calls and replaces them with the stringified file content. | |
| It runs natively in Node.js but can run in the browser after a compile step is applied during our bundling pipeline. That compile step uses `brfs`, which takes any `fs.readFileSync()` calls and replaces them with the stringified file content. |
future proof this :)
brendankenny
force-pushed
the
report-generator.d.ts
branch
from
9c5d5b4 to
b714d16
Compare
This is the last step before isolated report types, for real this time :)
report-generatorand assets are kind of the odd duck in thereport/directory. They're still commonjs and they're intended to be runnable in Node and bundleable and runnable in the browser (for e.g. the viewer). This PR moves them to their own directory and gives them their owntsconfigso they can be used as a project reference, allowingtscto treatreport-generatoras a standalone dep whether being called from core code or report code.This puts them under
report/generator/, but there are other options:report-generator/or whatever). This makes a certain amount of sense to separate from pure browser code like the rest ofreport/, but the name clashing withreportis unfortunatelighthouse-core/report-generator/. It makes a certain amount of sense insidelighthouse-corebecause we sometimes include it directly in core bundles but when we ship it with report code like viewer, it's a separatereport-generatorbundle.util, and likely will have more in the future. Downside is less specificity and it could just become a clearinghouse of unrelated codeFunctionality-wise the directory structure doesn't really seem to matter at this stage, but I wanted to throw out the options in case the current choice seems wrong to anyone.
Also:
file-namer.jsto live with report generator code, because it's only used for generated reports (in node and the browser)report/default"type": "module"and makesreport/generator/andreport/test/generator/the exceptions as"type": "commonjs"