core(start-url): reorganize start_url gatherer #4838
Conversation
| // Wait up to 3s to get a matched network request from the fetch() to work | ||
| const timeoutPromise = new Promise(resolve => setTimeout(_ => | ||
| resolve({statusCode: -1, debugString: 'Timed out waiting for fetched start_url'}) | ||
| , 3000)); |
There was a problem hiding this comment.
this is some strange indentation haha is this prettier's handiwork?
There was a problem hiding this comment.
probably, was thinking the same but didn't dare to comment 😛
| driver.on('Network.responseReceived', onResponseReceived); | ||
|
|
||
| // Deliberately not returning, as _attemptManifestFetch resolves when we have a matched network request | ||
| driver.evaluateAsync(`fetch('${startUrl}')`); |
There was a problem hiding this comment.
do we want to have a catch though to prevent unhandled promise rejection?
There was a problem hiding this comment.
can this be pulled out and do
return driver.evaluateAsync(`fetch('${startUrl}')`)
.then(_ => Promise.race([fetchPromise, timeoutPromise]));at the end?
There was a problem hiding this comment.
do we want to have a catch though to prevent unhandled promise rejection?
yes.. like just adding: .catch(reject).
can this be pulled out ...
hm. it's not that i think the fetch() may take forever but that the onResponseRecieved fn may not resolve
There was a problem hiding this comment.
can this be pulled out and do
goooooooood calllllll. sorry didnt understand. yah this is good.
| /** | ||
| * Read the parsed manifest and return failure reasons or the startUrl | ||
| * @param {Manifest} manifest | ||
| * @return {{isReadFailure: !boolean, reason: ?string, startUrl: ?startUrl}} |
There was a problem hiding this comment.
don't need any of the ! anymore 🎉 @brendankenny are we going with closure or ts style ??
There was a problem hiding this comment.
no more !! Assume everything is non-nullable and add ? (or |null) if you really want something to be nullable
There was a problem hiding this comment.
also I think you want reason?: string, startUrl?: startUrl here since they're optional and not null? If you want to get real fancy you can do
@return {{isReadFailure: true, reason: string}|{isReadFailure: false, startUrl: string}}
| */ | ||
| _attemptManifestFetch(options, startUrl) { | ||
| // Wait up to 3s to get a matched network request from the fetch() to work | ||
| const timeoutPromise = new Promise(resolve => setTimeout(_ => |
There was a problem hiding this comment.
why setTimeout(_ => and not setTimeout(() => not sure if we have a rule on this one 🙄
There was a problem hiding this comment.
we've historically allowed a _ as a short param name that's always ignored, but with tsc type checking it's flagged as an implicit any typed variable if the compiler can't figure out what it is, at which point it's usually easier just to switch to (). I'm not sure if there's any way to get typescript to ignore those cases, so we may end up just ditching it. Not sure yet :)
There was a problem hiding this comment.
oo, oo ditch it! ditch it! ditch it! 😃
| , 3000)); | ||
|
|
||
| const fetchPromise = new Promise(resolve => { | ||
| const driver = options.driver; |
There was a problem hiding this comment.
should we do ´attemptManifestFetch({driver}, startUrl) {´ ?
than we could nuke this line
There was a problem hiding this comment.
good idea! much better.
| */ | ||
| _attemptManifestFetch(options, startUrl) { | ||
| // Wait up to 3s to get a matched network request from the fetch() to work | ||
| const timeoutPromise = new Promise(resolve => setTimeout(_ => |
There was a problem hiding this comment.
do we want to hold off on adding this and just have a general "add generalized timeout method to driver commands" issue? Only bring this up because we've never had reports of this timing out, so not sure if it's worth adding in various implementations of timeouts spread out over a few different gatherers
There was a problem hiding this comment.
i'm ok with delaying.
up till now the starturl gatherer uses fetch('${url}').then(response => response.status). That's pretty direct and I wouldn't expect it to hang forever.
Now we're fetching and observing all network requests until we see one with a matching url. it's different and i'm not 100% we'll always have a match. redirects shouldn't time out but ... it probably will mess up.
(This is a merge into #4710)
This is reusing the exact same new logic from 4710, just separated a bit.
Only interesting things:
isReadFailureboolean to indicate if we should bail and fail before trying afetch