report: create faux psi report#12815
Conversation
|
I noticed clicking on an audit filter in the desktop tab doesn't do anything on the desktop audits, but does change the filter for the mobile tab. Edit: ope this is a PSI bug |
heh. well. thats exactly what this lil stupid report is for. :) update: PR to fix up in #12817 |
connorjclark
left a comment
There was a problem hiding this comment.
I wanted a test page where there are two LHRs, like in PSI. This helps surface the peculiar behavior we get with tabs and element-screenshot-overlay and how it's gotta live outside the tab body.
Can you put that as a comment in one of the PSI test "asset" files?
|
|
||
| (async function() { | ||
| // Must build before importing report-generator (or indirectly through lighthouse) | ||
| await buildreport.buildStandaloneReport(); |
There was a problem hiding this comment.
why remove yarn build-report && from yarn now-build ? it's simpler than doing the imports here in the right order.
the CLI options could be expanded to build psi report on a flag
There was a problem hiding this comment.
hmm.. i don't know what you do, but if i'm working on the report this is my watch equivalent:
find report | entr node lighthouse-core/scripts/build-report-for-autodeployment.jsi heart entr. (but i can't && in that command or whatever so... the combo of the two commands is a problem).
i guess an alternative would be a new yarn build-dist-reports script that does build-report followed by this. that'd be fine.
wdyt?
There was a problem hiding this comment.
but i can't && in that command or whatever so.
Do it like this:
find report | entr bash -c "ls && echo hi"
wdyt?
the extra commands make it hard to follow. go back to yarn build-report && node lighthouse-core/scripts/build-report-for-autodeployment.js && yarn build-viewer && yarn build-treemap && cp -r dist/gh-pages dist/now/gh-pages and just use the bash trick above ?
I assume you don't just do find report | yarn now-build because of all the extra stuff going on there that it takes a bit longer... 7s (vs 2s) for a watch is rough
| "cli-unit": "yarn unit-cli", | ||
| "viewer-unit": "yarn unit-viewer", | ||
| "watch": "yarn unit-core --watch", | ||
| "watch:report": "find report | entr yarn build-dist-reports", |
| /** | ||
| * @return {string} | ||
| */ | ||
| function generatePsiReportHtml() { |
There was a problem hiding this comment.
this definitely seems kind of weird in build-report-for-autodeployment.js and possibly hard to keep up to date when changes happen to report-generator, but I also like keeping it simple like this while we iterate on report/overall build story
| * Drops the LHR to only one, solo category (performance), and removes budgets. | ||
| * @param {LH.Result} lhr | ||
| */ | ||
| function tweakLhrForPsi(lhr) { |
There was a problem hiding this comment.
should this matter? Shouldn't it just be able to take a regular LHR and just ignore the stuff it doesn't want? Or is this more a matter of giving it the data it will actually get in practice for a more realistic deployment?
There was a problem hiding this comment.
It shouldnt matter much, but I just wanted to the data to match what PSI would expect to see. I can't think of any real reasons why it'd matter (like full-page-screenshot being in another category or something).
| "cli-unit": "yarn unit-cli", | ||
| "viewer-unit": "yarn unit-viewer", | ||
| "watch": "yarn unit-core --watch", | ||
| "watch:report": "find report | entr yarn build-dist-reports", |
There was a problem hiding this comment.
what happened to
sure lets just leave the entr stuff out of our scripts for now
?
| "build-lr": "yarn reset-link && node ./build/build-lightrider-bundles.js", | ||
| "build-pack": "bash build/build-pack.sh", | ||
| "build-report": "node build/build-report.js", | ||
| "build-dist-reports": "yarn build-report && node lighthouse-core/scripts/build-report-for-autodeployment.js", |
There was a problem hiding this comment.
bike shed: what is build-dist-reports referring to? Just build reports that will be in dist/? Because it's also sample reports in particular. build-sample-reports?
There was a problem hiding this comment.
hehe me and paul were talking about the naming here...
build-dist-reports: Builds the report renderer and the reports for the Vercel deploymentbuild-report-for-autodeployment.js: Builds the reports for the Vercel deploymentnow-build: The Vercel deployment used to be called "now". not anymore
some renaming is in order all around.
|
|
||
| /** | ||
| * Returns the report HTML as a string with the report JSON and renderer JS inlined. | ||
| * @param {Object} object |
There was a problem hiding this comment.
| * @param {Object} object | |
| * @param {unknown} object |
| } | ||
|
|
||
| /** | ||
| * Returns the standaloe report HTML as a string with the report JSON and renderer JS inlined. |
There was a problem hiding this comment.
| * Returns the standaloe report HTML as a string with the report JSON and renderer JS inlined. | |
| * Returns the standalone report HTML as a string with the report JSON and renderer JS inlined. |
| * @param {LH.Result} lhr | ||
| * @param {string} tabId | ||
| */ | ||
| async function distinguishLHR(lhr, tabId) { |
There was a problem hiding this comment.
can this be combined with what happens in tweakLhrForPsi?
There was a problem hiding this comment.
i could but then i wouldn't have the single-category dist-report. but i really want that for local report dev. :)
I wanted a test page where there are two LHRs, like in PSI. This helps surface the peculiar behavior we get with tabs and element-screenshot-overlay and how it's gotta live outside the tab body.
here 'tis: https://lighthouse-git-faux-psi-googlechrome.vercel.app/%E2%8C%A3.psi.english