cli(asset-saver): print one devtoolsLog event per line #12348
Conversation
| const writeStream = fs.createWriteStream(traceFilename); | ||
| writeStream.on('finish', resolve); | ||
| writeStream.on('error', reject); | ||
| return pipeline(stream.Readable.from(traceIter), writeStream); |
There was a problem hiding this comment.
this was able to get way nicer than the old version. Node 13+ will let us drop the stream.Readable.from() part (pipeline will take iterables directly) and node 15+ will let us drop the promisify(stream.pipeline) call
There was a problem hiding this comment.
add a compat TODO comment for the latter bit?
| {"args":{"data":{"encodedDataLength":20,"requestId":"1.22"}},"pid":1,"ts":6} | ||
| ]} | ||
| `); | ||
| }); |
There was a problem hiding this comment.
we didn't have any tests of the printing style, just that all the correct content was being printed in a JSON.parse()able way.
| version "10.14.0" | ||
| resolved "https://registry.yarnpkg.com/@types/node/-/node-10.14.0.tgz#1c297530428c6f4e0a0a3222f5b44745669aa9f7" | ||
| integrity sha512-1UhSMMDix7bVdUeqtZERQQyJr3QuFoN5X5APtpIooGkumE3crPaeq7UgFeJNjGD8yCQ8od8PzRkgptR5+x327Q== | ||
| version "12.20.7" |
There was a problem hiding this comment.
needed up to date (for Node 12) types to be able to use stream.Readable.from().
(@adamraine these outdated node types were also the thing preventing this suggestion from working: #12145 (comment))
|
test failure is browserify not supplying It is included in browserify 17 (up from the current 16.2.3`). I'm looking at how easy the update is vs just doing a workaround. |
brendankenny
force-pushed
the
pretty-devtoolslog
branch
from
0ec7203
to
8ba00ce
Compare
it's not bad at all |
lighthouse-core/lib/asset-saver.js
Outdated
| const firstElement = elementsIterator.next().value; | ||
| yield ` ${JSON.stringify(firstElement)}`; | ||
|
|
||
| let elementsRemaining = ELEMENTS_PER_ITERATION; |
There was a problem hiding this comment.
"element" => "item"? or "value"?
lighthouse-core/lib/asset-saver.js
Outdated
|
|
||
| yield '{\n'; | ||
| function* arrayOfObjectsJsonGenerator(arrayOfObjects) { | ||
| const ELEMENTS_PER_ITERATION = 500; | ||
|
|
||
| // Stringify and emit trace events separately to avoid a giant string in memory. |
| * @return {IterableIterator<string>} | ||
| */ | ||
| function* traceJsonGenerator(traceData) { | ||
| const {traceEvents, ...rest} = traceData; |
| const writeStream = fs.createWriteStream(traceFilename); | ||
| writeStream.on('finish', resolve); | ||
| writeStream.on('error', reject); | ||
| return pipeline(stream.Readable.from(traceIter), writeStream); |
There was a problem hiding this comment.
add a compat TODO comment for the latter bit?
brendankenny
force-pushed
the
pretty-devtoolslog
branch
from
aaac433
to
557dcf6
Compare
It's very nice that the trace that comes out of
-GAis saved with a trace event per line. Doing some recent work with devtoolsLogs made me really want the same printing style for those.