-
Notifications
You must be signed in to change notification settings - Fork 9.5k
core(asset-saver): fix handling of undefined trace #15451
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
fs.mkdirSync(tmpDir, {recursive: true}); | ||
const artifacts = { | ||
DevtoolsLog: [{message: 'first'}, {message: 'second'}], | ||
Trace: {traceEvents}, | ||
}; | ||
|
||
return assetSaver.saveAssets(artifacts, dbwResults.audits, `${tmpDir}/the_file`); | ||
await assetSaver.saveAssets(artifacts, dbwResults.audits, `${tmpDir}/the_file`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ope, previous attempt had multiple calls to saveAssets
, I'll change it back if someone wants
const saveAll = allAssets.map(async (assets, index) => { | ||
if (assets.devtoolsLog) { | ||
const devtoolsLogFilename = `${pathWithBasename}-${index}${devtoolsLogSuffix}`; | ||
await saveDevtoolsLog(assets.devtoolsLog, devtoolsLogFilename); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick callout, using saveDevtoolsLog
instead of a generic JSON.stringify
and fs.writeFileSync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a readability change, but in what direction is subjective. seems fine.
Fixes #15450