-
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
refactor(StartUrl): switch from error to debugString object #2549
Changes from all commits
fbd724e
d3c8dfd
f98d4b9
6589cdd
1cf9b18
369058d
6cefcd6
5c9b84b
4a0e7d4
c552cab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,7 +103,7 @@ URL.elideDataURI = function elideDataURI(url) { | |
// Why? Special handling was added by Chrome team to allow a pushState transition between chrome:// pages. | ||
// As a result, the network URL (chrome://chrome/settings/) doesn't match the final document URL (chrome://settings/). | ||
function rewriteChromeInternalUrl(url) { | ||
if (!url.startsWith('chrome://')) return url; | ||
if (!url || !url.startsWith('chrome://')) return url; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what new path needs this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was a drive-by fix of something I encountered on another branch, can't remember what site now though it was over a month ago :P There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but to answer your question the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
ha, I had forgotten we had done that. I guess that's fine vs moving the |
||
return url.replace(/^chrome:\/\/chrome\//, 'chrome://'); | ||
} | ||
|
||
|
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.
why drop the
response.url
check? If the manifest 404s, for instance,response.data
will beundefined
but that should probably be considered an error state, not that there is no manifestThere 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.
IMO, anything that's not a bug should not say 'GathererError: blah', if the manifest 404s that's not a bug so it shouldn't be an error, at best we can give them the slightly different debug strings, is that what you're pushing back against?
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.
Saying "No web app manifest found on page" is definitely wrong in that case, so a better error string is needed. But I'm not sure I agree about Gatherer errors. I have no problem with a method fetching a manifest throwing on a manifest URL that doesn't exist.
But I assume the issue is us conflating Lighthouse bugs with exceptional states? We may need to get back into that conversation to get the functionality we need. We don't want to slide back into -1 land where every audit has to handle all possible gatherer error states, but agreed we probably need a better separation between the classes of errors.
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've got no problem with this throwing either just the overall pass throwing instead of returning debugString. There should be some distinguishing from a user's perspective between a bug they have no control over and something they need to take action on.
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.
Also re:
I'd don't actually agree that "Unable to retrieve manifest" is better than "No web app manifest found on page", as a user receiving the former I may believe that it's LH problem not being able to retrieve it and there's nothing for me to do whereas the latter gets me to check my manifest. There's probably room for improvement on both, but calling it "definitely wrong" seems overly critical. It's not wrong, there was no usable manifest found.
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.
maybe we need to start laying out the failure states for an audit ahead of time so we can decide on them once (in theory) and then it's obvious when they're implemented or not :)
If there's no
<link rel="manifest">
in the page, it's obviously not an error, the audits simply just won't pass. However, to me, if there is a<link rel="manifest">
and the url 404s, that is an error worth reporting to the user, as much as a manifest made up of invalid JSON would be. They tried to include one and it's failing for some reason.Agreed "Unable to retrieve manifest" is a bad error string for that and we should be as specific as possible to help with those situations. Also totally on board with exploring non-exception error returns, expected errors, etc after this so we can separate the two (or more?) clear classes of errors we're dealing with here.
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.
One downside of the current
Page.getAppManifest
protocol call is that it gives no network status information (as far as I can tell). So e.g. if the server returns a 404 with aCannot GET /manifest.json
response string, the protocol will just return aJSON.parse
error from trying to parse that string (with no other indication of what happened) and we just have to infer that there was a 404. Maybe we could see about getting more detail there.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.
(the devtools Application panel suffers from the same problem, only showing "Errors and warnings:
Line: 1, column: 1, Unexpected token.
" and you have to notice the 404 in the console/network panel to see what happened)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 feels like it should return something similar to an artifact
{value: null, debugString: ''}
or whatever