cli(asset-saver): end devtoolsLog with a newline #13566
Conversation
| @@ -19,6 +19,8 @@ const LHError = require('../lib/lh-error.js'); | |||
| // TODO(esmodules): Rollup does not support `promisfy` or `stream.pipeline`. Bundled files | |||
| // 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
| @@ -37,32 +34,30 @@ async function update(artifactName) { | |||
| await cli.runLighthouse(url, flags, budgetedConfig); | |||
| await server.close(); | |||
|
|
|||
| 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.
| @@ -96,6 +96,7 @@ describe('asset-saver helper', () => { | |||
| {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
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().