-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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: add top-level runtimeError #6014
Conversation
lol no |
This comment has been minimized.
This comment has been minimized.
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.
awesome. this matches what we need. :)
CI needs golden lhr update
|
||
return results.report; | ||
return JSON.stringify(runtimeError, null, 2); |
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.
I was thinking we're returning a psuedo LHR so one more level on top of this:
return JSON.stringify({runtimeError}, null, 2);
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.
yeah I missed that.
ok, ready for review @paulirish :) |
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.
looks so great. all lgtm except for one question on throwing in ext-bg
} catch (err) { | ||
// If an error ruined the entire lighthouse run, attempt to return a meaningful error. | ||
if (!(err instanceof LHError) || !err.lhrRuntimeError) { | ||
throw err; |
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.
In this situation I think we should not throw but instead return an "UNKNOWN_ERROR" i guess.
tbh i dont know what throwing does in this context. my guess is its not caught and triggers a timeout?
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.
tbh i dont know what throwing does in this context. my guess is its not caught and triggers a timeout?
well it would throw and be up to the caller to deal with, but we can do UNKNOWN_ERROR
instead :)
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.
done
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.
so so good. \o/
🍳🍳🍳
@paulirish
Here's one simple approach to #5881. All artifact errors are caught, and any deemed
runtimeError
worthy are floated to the top-level property in the LHR. From that bug, this handlesIt sort of handles
but if that pageload error affects enough audits (which it usually does) we throw from the whole Lighthouse run and don't ever return an LHR.
Like the idea mentioned in #5881 (comment), I'd prefer to continue throwing exceptions for these cases and then checking outside core in
runLighthouseForLR
that the thrown error haslhrRuntimeError
set on it. Most of our clients just want a thrown error in those cases, not a pseudo-LHR, and dealing with a pseudo-LHR just makes tracking the LHR type and what's on it messier when we really just want an exception floated up.So I don't think it's worth doing that inside core. In
runLighthouseForLR
we can create that pseudo-LHR with just the top-levelruntimeError
property.Doing it that way will completely handle (1) above and
If this is 👍 from you I can add that and tests.