core(robots): use new fetcher to get robots.txt #12423
Conversation
| return { | ||
| stream: networkResponse.resource.success ? (networkResponse.resource.stream || null) : null, | ||
| status: networkResponse.resource.httpStatusCode || null, | ||
| }; |
There was a problem hiding this comment.
I used null to match the expectations of the RobotsTxt artifact. I think null makes more sense than undefined but it leads to some ugly code like this.
There was a problem hiding this comment.
I have had 3 different versions of a comment written here in various tabs that I've lost with varying levels of concern, but at this point it just SGTM 😆
There was a problem hiding this comment.
what are the failure cases? ERR_BLOCKED_BY_CLIENT? Is success set by netError and error status codes, or just netError? If it's just netError, maybe we should mimic real fetch() and throw in that case?
There was a problem hiding this comment.
If it's just netError, maybe we should mimic real fetch() and throw in that case?
In one iteration of my comment, I discuss with my lost tab that it seems odd that we would throw an exception rather than a warning on notApplicable. It's not a bug, but rather a known edge case, so the new null behavior makes more sense. That does suggest we actually make it a warning in RobotsTxt audit.
I'm fine with maintaining the status quo as well and throwing if others prefer that.
There was a problem hiding this comment.
Yeah, I was thinking in terms of fetcher on its own. The gatherer could then do as it saw fit with the response
There was a problem hiding this comment.
success is set to false for error status codes, and there is no stream handle to use if content is served wit an error status code.
There was a problem hiding this comment.
there is no stream handle to use if content is served wit an error status code.
that seems like a CDP bug but probably not one worth pursuing until we have a reason to want to look at response bodies from requests that 404 or whatever :)
I think it still could make sense for fetcher to return
- good statusCode:
Promise.resolve({status: number, content: string}) - bad statusCode:
Promise.resolve({status: number, content: null}) - netError:
Promise.reject(new Error(netErrorName))
(and e.g. the RobotsTxt gatherer could catch the error and augment the artifact with a failure reason to add to the audit explanation string)
but I don't think this is critical today, so I'm fine with this version as well.
There was a problem hiding this comment.
nothing blocking here, LGTM nice work! :)
does this method trigger the download UI if the mime type isn't set like the previous fetching approach? I bet that's much more common on .txt than js/json files and something to be aware of and/or prioritize on the devtools side to fix
| return { | ||
| stream: networkResponse.resource.success ? (networkResponse.resource.stream || null) : null, | ||
| status: networkResponse.resource.httpStatusCode || null, | ||
| }; |
There was a problem hiding this comment.
I have had 3 different versions of a comment written here in various tabs that I've lost with varying levels of concern, but at this point it just SGTM 😆
No |
|
https://github.com/GoogleChrome/lighthouse/pull/12423/checks?check_run_id=2470752493 Getting a diagnostic message "Getting browser version" in the DevTools integration tests. Does that happen for every call to |
Fixes #10225
Use new fetcher to get
robots.txt. The CSP of the page should not interfere this way.