clients(lightrider): serialize errors in artifacts #9410
Conversation
| return JSON.stringify(preprocessor.processForProto(runnerResult.lhr)); | ||
| const preprocessedLhr = preprocessor.processForProto(runnerResult.lhr); | ||
| // Properly serialize artifact errors for logging. | ||
| return JSON.stringify(preprocessedLhr, logAssets ? assetSaver.stringifyReplacer : undefined); |
There was a problem hiding this comment.
hmm, I don't think we want to structure it this way. assetSaver should be able to change stringifyReplacer at will, which will be a whole lot easier if it's only ever run over artifacts, not a whole LHR as well.
Maybe we should expose a thing on assetSaver to stringifyArtifacts (but not save them to disk like saveArtifacts() does)? My only concern is replacer performance against traces, since it runs against every single trace event (and trace event properties, recursively). That's not an issue in saveArtifacts because traces are removed before stringifying
There was a problem hiding this comment.
hmm, I don't think we want to structure it this way. assetSaver should be able to change stringifyReplacer at will, which will be a whole lot easier if it's only ever run over artifacts, not a whole LHR as well.
how would the resulting artifacts JSON be put back to the larger LHR json?
My only concern is replacer performance against traces, since it runs against every single trace event (and trace event properties, recursively). That's not an issue in saveArtifacts because traces are removed before stringifying
performance can also be ignored here, as asset logging is only done via the LR demo app (production would never use the replacer).
There was a problem hiding this comment.
how would the resulting artifacts JSON be put back to the larger LHR json?
parse again? It's not pretty but I don't think this replacer should be responsible for preserving artifacts and LHRs
There was a problem hiding this comment.
assetSaver should be able to change stringifyReplacer at will, which will be a whole lot easier if it's only ever run over artifacts, not a whole LHR as well.
what changes are you anticipating such that this would break when you pass in an LHR? Maybe we could wait until then?
There was a problem hiding this comment.
what changes are you anticipating such that this would break when you pass in an LHR? Maybe we could wait until then?
i18n strings (#9269)? Other kinds of objects? etc etc
It's been a huge help in a few areas to have a definitive single place to save/load artifacts from disk (e.g. recent changes to report-update-fixtures.js), which allowed specialization like saving traces to separate files without worry that they'd be lost when loading them at a later point. If we want to open that to stringifying (without writing to disk), we should apply the same principles and let asset-saver do whatever it has to do internally without dictating an implementation.
|
(let's move against master) |
| // @ts-ignore - piggyback the artifacts on the LHR. | ||
| runnerResult.lhr.artifacts = runnerResult.artifacts; | ||
| } | ||
| // When LR is called with |internal: {keep_raw_response: true, save_lighthouse_assets: true}|, |
There was a problem hiding this comment.
Move comment to be above if (logAssets) {
| ...preprocessedLhr, | ||
| artifacts: JSON.parse(artifactsJson), | ||
| }); | ||
| } else { |
There was a problem hiding this comment.
Can't decide if the else is necessary or not. I'm flip flopping on keeping it in/removing it is more readable.
There was a problem hiding this comment.
Can't decide if the else is necessary or not. I'm flip flopping on keeping it in/removing it is more readable.
I'd fall on the side of no else, but really up to @connorjclark, it's fine either way
There was a problem hiding this comment.
can you be more specific about what you are suggesting instead
There was a problem hiding this comment.
You're returning in both sides of the if/else. You can just remove the else here to always return JSON.stringify(preprocessedLhr).
There was a problem hiding this comment.
do you see what i'm doing with artifacts tho.
| ...preprocessedLhr, | ||
| artifacts: JSON.parse(artifactsJson), | ||
| }); | ||
| } else { |
There was a problem hiding this comment.
Can't decide if the else is necessary or not. I'm flip flopping on keeping it in/removing it is more readable.
I'd fall on the side of no else, but really up to @connorjclark, it's fine either way
LR specific changes for #9397