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

UNKNOWN_ERROR LHRs cause Rendering Failure #9510

Closed
exterkamp opened this issue Aug 5, 2019 · 2 comments
Closed

UNKNOWN_ERROR LHRs cause Rendering Failure #9510

exterkamp opened this issue Aug 5, 2019 · 2 comments
Labels
PSI/LR PageSpeed Insights and Lightrider

Comments

@exterkamp
Copy link
Member

exterkamp commented Aug 5, 2019

Provide the steps to reproduce

Try to render an UNKNOWN ERROR LHR from PSI. Seems like we set configSettings and other things to null instead of filling them in (requestedUrl is null even though we know what url was requested). This causes renderer problems since it expects some things to exist.

What is the current behavior?

Uncaught TypeError: Cannot read property 'channel' of null

What is the expected behavior?

Rendering a failed report.

@patrickhulce ideas for this? I was thinking we populate most of the LHR in lr-entry before bailing with an UNKNOWN ERROR.

@exterkamp exterkamp added bug PSI/LR PageSpeed Insights and Lightrider labels Aug 5, 2019
@brendankenny
Copy link
Member

Try to render an UNKNOWN ERROR LHR from PSI. Seems like we set configSettings and other things to null instead of filling them in (requestedUrl is null even though we know what url was requested). This causes renderer problems since it expects some things to exist.

This was by design, though (#6014). The result was never supposed to be passed into the report renderer (it's nothing close to an LHR), it was only to satisfy the API needing an object with a top-level runtimeError.

} catch (err) {
// If an error ruined the entire lighthouse run, attempt to return a meaningful error.
let runtimeError;
if (!(err instanceof LHError) || !err.lhrRuntimeError) {
runtimeError = {
code: LHError.UNKNOWN_ERROR,
message: `Unknown error encountered with message '${err.message}'`,
};
} else {
runtimeError = {
code: err.code,
message: err.friendlyMessage ?
`${err.friendlyMessage} (${err.message})` :
err.message,
};
}

I was thinking we populate most of the LHR in lr-entry before bailing with an UNKNOWN ERROR.

I think this is the real phrasing of the issue. Current behavior is/was WAI. If the intent should instead be to always get an LHR back, lightrider-entry.js will need to recreate an LHR at that point.

@exterkamp exterkamp removed the bug label Aug 5, 2019
@exterkamp
Copy link
Member Author

Brendan has convinced me that this is WAI for Lighthouse, and needs to be handled client side. I will move this to track internally for PSI/LR.
Untitled-1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PSI/LR PageSpeed Insights and Lightrider
Projects
None yet
Development

No branches or pull requests

2 participants