core(fetcher): remove iframe fetcher #13923
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
| return { | ||
| content, | ||
| status: response.status, | ||
| }; |
There was a problem hiding this comment.
iframe fetcher suppressed the response content if the status code was not 2xx. (note: this was not a mistake, even if cached 304 response was received JS fetch sees the cached 2xx response code. and by default redirects are followed and the status code is the final response)
Should we consider suppressing the response content here if the status code is not 2xx (like iframe fetcher did)? And also for Network.loadNetworkResponse?
There was a problem hiding this comment.
Does the browser still work with robots.txt or source maps that have a non-2xx status code?
There was a problem hiding this comment.
I don't think the status code matters for source maps.
for robots.txt yes it needs to be 2xx https://developers.google.com/search/docs/advanced/robots/robots_txt#http-status-codes and the audit already checks this
There was a problem hiding this comment.
I think we should just return anything we get + the status code, and leave it to each use case to check the status if they care. we should be good as it is.
There was a problem hiding this comment.
I think we should just return anything we get + the status code, and leave it to each use case to check the status if they care. we should be good as it is.
Yeah SGTM
|
|
||
| /** @typedef {{content: string|null, status: number|null}} FetchResponse */ | ||
|
|
||
| const log = require('lighthouse-logger'); | ||
| const {getBrowserVersion} = require('./driver/environment.js'); | ||
|
|
||
| class Fetcher { |
There was a problem hiding this comment.
We don't need to use a class anymore, the state is just whatever session we pass into the constructor. We could refactor this to be more function like the rest of our helper files in lighthouse-core/gather/driver. This way we don't need to pass around a fetcher object on the gather context.
There was a problem hiding this comment.
I think we should do this in a different PR.
|
confirmed working in LR. |
| @@ -48,10 +48,20 @@ describe('SourceMaps gatherer', () => { | |||
| const sendCommandMock = createMockSendCommandFn() | |||
| .mockResponse('Debugger.enable', {}) | |||
| .mockResponse('Debugger.disable', {}) | |||
| <<<<<<< HEAD | |||
There was a problem hiding this comment.
Why is it always merge conflict
| /** @type {{stream: LH.Crdp.IO.StreamHandle|null, status: number|null}} */ | ||
| const response = await Promise.race([responsePromise, timeoutPromise]) | ||
| .finally(() => clearTimeout(timeoutHandle)); | ||
| const response = await this._wrapWithTimeout(this._loadNetworkResource(url), options.timeout); |
There was a problem hiding this comment.
_readIOStream has its own weird timeout system, would it be possible to remove that and use _wrapWithTimeout over _fetchResourceOverProtocol?
There was a problem hiding this comment.
it wouldn't be the same, unless we made the work _readIOStream does use a cancel-able promise. even if the wrapped timer timed out the work would keep going
| return { | ||
| content, | ||
| status: response.status, | ||
| }; |
There was a problem hiding this comment.
Does the browser still work with robots.txt or source maps that have a non-2xx status code?
The iframe fetcher was being used only in LR (see #13006), but now we have protocol support (originsWithUniversalNetworkAccess) to use
fetchdirectly in LR.Draft because we're waiting on some changes to WRS.