Skip to content
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

deps: remove details-element-polyfill and rimraf #12369

Merged
merged 4 commits into from
Apr 23, 2021
Merged

deps: remove details-element-polyfill and rimraf #12369

merged 4 commits into from
Apr 23, 2021

Conversation

brendankenny
Copy link
Member

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?

rimraf is no longer necessary with node 12+. Together with GoogleChrome/chrome-launcher#237, it means it's out of our deps 🎉

@brendankenny brendankenny requested a review from a team as a code owner April 15, 2021 22:30
@brendankenny brendankenny requested review from connorjclark and removed request for a team April 15, 2021 22:30
@google-cla google-cla bot added the cla: yes label Apr 15, 2021
@@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Wait for internalRun() before rmdiring scratch directory.
// Wait for internalRun() before removing scratch directory.

@@ -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});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glob works?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glob works?

no, and in fact doing this with fs.rmdir also makes no sense. Thanks for catching :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in fact doing this with fs.rmdir also makes no sense

fs.rm() will let us handle files and directories (with and without recursive: true, respectively), but not until node 14, unfortunately

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

breaks for windows, doesn't it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont really care if it does. seems reasonable to support only bash-in-windows and not the windows command line.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) ||
Copy link
Member Author

@brendankenny brendankenny Apr 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@brendankenny brendankenny merged commit 2e9c3c9 into master Apr 23, 2021
@brendankenny brendankenny deleted the olddeps branch April 23, 2021 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants