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

core: remove recoverOrThrow / err.fatal #6343

Merged
merged 4 commits into from
Oct 19, 2018

Conversation

connorjclark
Copy link
Collaborator

Fixes #6298

paulirish
paulirish previously approved these changes Oct 19, 2018
Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

lg though i want @brendankenny's 👍 for sure

@connorjclark
Copy link
Collaborator Author

Hmm.

@brendankenny For some reason, some tests fail now. Related to the gather lifecycle. For example: 'runs gatherer lifecycle methods strictly in sequence'. Functionally, the only difference I see it one less await for a Promise that immediately resolves. But, I tried adding a dumb await Promise.resolve(); where await GatherRunner.recoverOrThrow(artifactPromise) was, tests still fail.

@connorjclark connorjclark force-pushed the issue-6298-remove-recoverOrThrow branch from 8d3bef6 to b7add1f Compare October 19, 2018 01:49
@connorjclark
Copy link
Collaborator Author

connorjclark commented Oct 19, 2018

that promise still needs to be awaited :) updated

@@ -197,7 +183,7 @@ class GatherRunner {
passContext.options = gathererDefn.options || {};
const artifactPromise = Promise.resolve().then(_ => gatherer.beforePass(passContext));
gathererResults[gatherer.name] = [artifactPromise];
await GatherRunner.recoverOrThrow(artifactPromise);
await artifactPromise;
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I looked too quickly :) I should have said it'll have to be something like await artifactPromise.catch(() => {}); (basically the equivalent of deleting just the if (err.fatal) {throw err;} part of recoverOrThrow).

gathererResults[gatherer.name] needs to be set to the raw promise, which may resolve or reject, which will become either the artifact or an artifact error in collectArtifacts(). But the await needs to never throw so the failure of one gatherer won't prevent moving on to the next

lighthouse-core/gather/gather-runner.js Outdated Show resolved Hide resolved
connorjclark and others added 2 commits October 19, 2018 10:12
@connorjclark
Copy link
Collaborator Author

^That happened when I used GH's "apply change" feature ...

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

nice!! one codebase wart removed, only 9000 to go :)

lighthouse-core/test/runner-test.js Outdated Show resolved Hide resolved
Co-Authored-By: Hoten <cjamcl@gmail.com>
@GoogleChrome GoogleChrome deleted a comment from googlebot Oct 19, 2018
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

nice! ✂️ ✂️

@brendankenny brendankenny merged commit fd7bc18 into master Oct 19, 2018
@brendankenny brendankenny deleted the issue-6298-remove-recoverOrThrow branch October 19, 2018 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants