clients(lr): insert assets in lhr for logging purposes #9002
Conversation
connorjclark
requested review from
brendankenny,
patrickhulce and
paulirish
as code owners
|
at some point we were going to delete all of this because the artifacts are returned by the module now. Can lr use that instead of this logging business? |
|
Discussed offline a bit. We considered changing the LR entry to return the entire RunnerResult, but that would involve some annoying parsing changes in LR. Realized we can just put all of the |
paulirish
left a comment
paulirish
left a comment
There was a problem hiding this comment.
Let's put up the LR-side of this for review as well?
It'd be easier to handle both at once.
| * @return {Promise<void>} | ||
| * @return {Promise<Array<{name: string, contents: string}>>} | ||
| */ | ||
| async function logAssets(artifacts, audits) { |
There was a problem hiding this comment.
once we have the LR-side to process your nested artifacts, I think we can delete this logAssets function.
|
|
||
| if (logAssets) { | ||
| await assetSaver.logAssets(results.artifacts, results.lhr.audits); | ||
| const assetsToLog = |
There was a problem hiding this comment.
this refactor doesn't seem to be a win. if anything, logAssets() should call its own private prepare method, but due to other comments, i dont think we'll end up needing this code anyhow.
| // When LR is called with |internal: {keep_raw_response: true, save_lighthouse_assets: true}|, | ||
| // this code will log artifacts to raw_response.artifacts. | ||
| if (logAssets) { | ||
| const reportObj = JSON.parse(runnerResult.report); |
There was a problem hiding this comment.
let's use runnerResult.lhr instead of parsing the string back
3 participants
This enables logging from PSI.
Maybe we should log all the artifacts too?
Tests awaiting feedback.
Note: this PR is building on this (Googlers only).