cli(asset-saver): end devtoolsLog with a newline #13566
Conversation
| // don't need anything in this file except for `stringifyReplacer`, so a check for | ||
| // truthiness before using is enough. | ||
| // TODO: Can remove promisify(pipeline) in Node 15. | ||
| // https://nodejs.org/api/stream.html#streams-promises-api |
There was a problem hiding this comment.
just moving the TODO to see it more easily with the other workaround that can eventually be removed
|
|
||
| let newArtifacts = assetSaver.loadArtifacts(artifactPath); | ||
|
|
||
| // Normalize some data so it doesn't change on every update. |
There was a problem hiding this comment.
this file was making new artifacts, saving them with saveArtifacts(), loading them again to normalize churn-y Timing, then doing its own fs.writeFileSync(). Normalization should just happen before the first (and only) save, which also means the concluding newline added by saveArtifacts() will be in the resulting file.
| {args: {IsMainFrame: true}, cat: 'v8', pid: 1, ts: 5}, | ||
| {args: {data: {encodedDataLength: 20, requestId: '1.22'}}, pid: 1, ts: 6}, | ||
| ], | ||
| metadata: {'cpu-model': 9001, 'network-type': 'Unknown'}, |
There was a problem hiding this comment.
not directly important to Lighthouse, but overlooked in previous testing of saveTrace() and very relevant to where the various methods are putting newlines within their output :)
| yield* arrayOfObjectsJsonGenerator(devtoolsLog); | ||
| yield '\n'; |
There was a problem hiding this comment.
First time seeing yield in JS, lemme just google that quick
3 participants
Inspired by recent discussion about churn in newlines at the end of files updated by
yarn update:sample-artifactsvs.editorconfig'sinsert_final_newline = truewhen editing one of the artifact files manually. The scripted update should match the repo style, just like we do withyarn update:sample-json.Turns out
saveArtifacts()already saved the main artifacts and trace files with trailing newlines, so only the devtoolsLog files needed updating.update-report-fixtures.jsalso needed tweaking to not second guesssaveArtifacts().