deps: remove details-element-polyfill and rimraf #12369
Conversation
| @@ -36,8 +35,8 @@ async function runLighthouse(url, configJson, testRunnerOptions = {}) { | |||
|
|
|||
| const {isDebug} = testRunnerOptions; | |||
| return internalRun(url, tmpPath, configJson, isDebug) | |||
| // Wait for internalRun() before rimraffing scratch directory. | |||
| .finally(() => !isDebug && rimraf(tmpPath)); | |||
| // Wait for internalRun() before rmdiring scratch directory. | |||
There was a problem hiding this comment.
| // Wait for internalRun() before rmdiring scratch directory. | |
| // Wait for internalRun() before removing scratch directory. |
lighthouse-core/lib/asset-saver.js
Outdated
| @@ -102,8 +101,8 @@ async function saveArtifacts(artifacts, basePath) { | |||
| const status = {msg: 'Saving artifacts', id: 'lh:assetSaver:saveArtifacts'}; | |||
| log.time(status); | |||
| fs.mkdirSync(basePath, {recursive: true}); | |||
| rimraf.sync(`${basePath}/*${traceSuffix}`); | |||
| rimraf.sync(`${basePath}/${artifactsFilename}`); | |||
| fs.rmdirSync(`${basePath}/*${traceSuffix}`, {recursive: true}); | |||
There was a problem hiding this comment.
glob works?
no, and in fact doing this with fs.rmdir also makes no sense. Thanks for catching :)
There was a problem hiding this comment.
in fact doing this with
fs.rmdiralso makes no sense
fs.rm() will let us handle files and directories (with and without recursive: true, respectively), but not until node 14, unfortunately
There was a problem hiding this comment.
fixed and added a test to catch this next time
| @@ -26,7 +26,7 @@ | |||
| "build-viewer": "node ./build/build-viewer.js", | |||
| "reset-link": "(yarn unlink || true) && yarn link && yarn link lighthouse", | |||
| "c8": "bash lighthouse-core/scripts/c8.sh", | |||
| "clean": "rimraf dist proto/scripts/*.json proto/scripts/*_pb2.* proto/scripts/*_pb.* proto/scripts/__pycache__ proto/scripts/*.pyc *.report.html *.report.dom.html *.report.json *.devtoolslog.json *.trace.json lighthouse-core/lib/i18n/locales/*.ctc.json || true", | |||
| "clean": "rm -r dist proto/scripts/*.json proto/scripts/*_pb2.* proto/scripts/*_pb.* proto/scripts/__pycache__ proto/scripts/*.pyc *.report.html *.report.dom.html *.report.json *.devtoolslog.json *.trace.json lighthouse-core/lib/i18n/locales/*.ctc.json || true", | |||
There was a problem hiding this comment.
breaks for windows, doesn't it?
There was a problem hiding this comment.
dont really care if it does. seems reasonable to support only bash-in-windows and not the windows command line.
There was a problem hiding this comment.
dont really care if it does. seems reasonable to support only bash-in-windows and not the windows command line.
Yeah, I also don't think it matters. I honestly never run this because it also deletes *.report.html *.report.json *.devtoolslog.json *.trace.json and I usually have at least one of those lying around I don't want deleted right now, but I'm sure someone out there likes a tidy checkout.
| // Delete any previous artifacts in this directory. | ||
| const filenames = fs.readdirSync(basePath); | ||
| for (const filename of filenames) { | ||
| if (filename.endsWith(traceSuffix) || filename.endsWith(devtoolsLogSuffix) || |
There was a problem hiding this comment.
It's not clear why we weren't deleting latest-run/*.devtoolslog.json before. Maybe we just wrote this before devtoolslogs as files existed and didn't update?
It leads to weird situations since loadArtifacts() will happily load any file in the directory ending with devtoolslog.json, so you can easily get devtoolsLogs from previous runs (like pageLoadError-defaultPass.devtoolslog.json) in the artifacts of a brand new -A run. I don't think we currently have any code that would automatically do anything with logs with non-default names, but still weird for them to be mixed in there
details-element-polyfill hasn't been necessary since Edgium was released. This was just tweaked in #12267, but seems like there's no reason to keep it?
rimrafis no longer necessary with node 12+. Together with GoogleChrome/chrome-launcher#237, it means it's out of our deps 馃帀