-
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
core(start-url): reorganize start_url gatherer #4838
Changes from 3 commits
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 |
---|---|---|
|
@@ -9,76 +9,90 @@ const Gatherer = require('./gatherer'); | |
const manifestParser = require('../../lib/manifest-parser'); | ||
|
||
class StartUrl extends Gatherer { | ||
executeFetchRequest(driver, url) { | ||
return driver.evaluateAsync( | ||
`fetch('${url}')` | ||
); | ||
} | ||
|
||
/** | ||
* Grab the manifest, extract it's start_url, attempt to `fetch()` it while offline | ||
* @param {*} options | ||
* @return {{statusCode: number, debugString: ?string}} | ||
*/ | ||
afterPass(options) { | ||
return options.driver.goOnline(options) | ||
.then(() => options.driver.getAppManifest()) | ||
const driver = options.driver; | ||
return driver.goOnline(options) | ||
.then(() => driver.getAppManifest()) | ||
.then(response => driver.goOffline(options).then(() => response)) | ||
.then(response => response && manifestParser(response.data, response.url, options.url)) | ||
.then(manifest => { | ||
if (!manifest || !manifest.value) { | ||
const detailedMsg = manifest && manifest.debugString; | ||
|
||
if (detailedMsg) { | ||
const debugString = `Error fetching web app manifest: ${detailedMsg}`; | ||
return { | ||
statusCode: -1, | ||
debugString, | ||
}; | ||
} else { | ||
const debugString = `No usable web app manifest found on page ${options.url}`; | ||
return { | ||
statusCode: -1, | ||
debugString, | ||
}; | ||
} | ||
const {isReadFailure, reason, startUrl} = this._readManifestStartUrl(manifest); | ||
if (isReadFailure) { | ||
return {statusCode: -1, debugString: reason}; | ||
} | ||
|
||
if (manifest.value.start_url.debugString) { | ||
// Even if the start URL had an error, the browser will still supply a fallback URL. | ||
// Therefore, we only set the debugString here and continue with the fetch. | ||
return { | ||
statusCode: -1, | ||
debugString: manifest.value.start_url.debugString, | ||
}; | ||
} | ||
return this._attemptManifestFetch(options, startUrl); | ||
}).catch(() => { | ||
return {statusCode: -1, debugString: 'Unable to fetch start URL via service worker'}; | ||
}); | ||
} | ||
|
||
/** | ||
* Read the parsed manifest and return failure reasons or the startUrl | ||
* @param {Manifest} manifest | ||
* @return {{isReadFailure: !boolean, reason: ?string, startUrl: ?startUrl}} | ||
*/ | ||
_readManifestStartUrl(manifest) { | ||
if (!manifest || !manifest.value) { | ||
const detailedMsg = manifest && manifest.debugString; | ||
|
||
const startUrl = manifest.value.start_url.value; | ||
if (detailedMsg) { | ||
return {isReadFailure: true, reason: `Error fetching web app manifest: ${detailedMsg}`}; | ||
} else { | ||
return {isReadFailure: true, reason: `No usable web app manifest found on page`}; | ||
} | ||
} | ||
|
||
return (new Promise(resolve => { | ||
options.driver.on('Network.responseReceived', function responseReceived({response}) { | ||
if (response.url === startUrl) { | ||
options.driver.off('Network.responseReceived', responseReceived); | ||
// Even if the start URL had an error, the browser will still supply a fallback URL. | ||
// Therefore, we only set the debugString here and continue with the fetch. | ||
if (manifest.value.start_url.debugString) { | ||
return {isReadFailure: true, reason: manifest.value.start_url.debugString}; | ||
} | ||
|
||
if (!response.fromServiceWorker) { | ||
return resolve({ | ||
statusCode: -1, | ||
debugString: 'Unable to fetch start URL via service worker', | ||
}); | ||
} | ||
return {isReadFailure: false, startUrl: manifest.value.start_url.value}; | ||
} | ||
|
||
/** | ||
* Try to `fetch(start_url)`, return true if fetched by SW | ||
* @param {*} options | ||
* @param {!string} startUrl | ||
* @return {Promise<{statusCode: ?number, debugString: ?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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why 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. we've historically allowed a 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. oo, oo ditch it! ditch it! ditch it! 😃 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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'm ok with delaying. up till now the starturl gatherer uses 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. |
||
resolve({statusCode: -1, debugString: 'Timed out waiting for fetched start_url'}) | ||
, 3000)); | ||
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 is some strange indentation haha is this prettier's handiwork? 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. probably, was thinking the same but didn't dare to comment 😛 |
||
|
||
const fetchPromise = new Promise(resolve => { | ||
const driver = options.driver; | ||
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. should we do ´attemptManifestFetch({driver}, startUrl) {´ ? 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. good idea! much better. |
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we want to have a catch though to prevent unhandled promise rejection? 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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yes.. like just adding:
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
goooooooood calllllll. sorry didnt understand. yah this is good. |
||
|
||
return resolve({ | ||
statusCode: response.status, | ||
debugString: '', | ||
}); | ||
} | ||
function onResponseReceived({response}) { | ||
// ignore mismatched URLs | ||
if (response.url !== startUrl) return; | ||
driver.off('Network.responseReceived', onResponseReceived); | ||
|
||
if (!response.fromServiceWorker) { | ||
return resolve({ | ||
statusCode: -1, | ||
debugString: 'Unable to fetch start URL via service worker', | ||
}); | ||
} | ||
// Successful SW-served fetch of the start_URL | ||
return resolve({statusCode: response.status}); | ||
} | ||
}); | ||
|
||
options.driver.goOffline(options) | ||
.then(() => this.executeFetchRequest(options.driver, startUrl)) | ||
.then(() => options.driver.goOnline(options)) | ||
.catch(() => { | ||
resolve({ | ||
statusCode: -1, | ||
debugString: 'Unable to fetch start URL via service worker', | ||
}); | ||
}); | ||
})); | ||
}); | ||
return Promise.race([fetchPromise, timeoutPromise]); | ||
} | ||
} | ||
|
||
|
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.
don't need any of the
!
anymore 🎉 @brendankenny are we going with closure or ts style?
?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.
no more
!
! Assume everything is non-nullable and add?
(or|null
) if you really want something to be nullableThere 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 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