Skip to content
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): stay offline for entirety of offlinePass #9451

Merged
merged 8 commits into from
Aug 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions lighthouse-core/gather/gatherers/offline.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ class Offline extends Gatherer {
* @param {LH.Gatherer.PassContext} passContext
*/
beforePass(passContext) {
// This call sets up the offline state for the page navigation of the `offlinePass` in gather-runner.
// gather-runner will automatically go back online before the `afterPass` phase, so no additional
// cleanup is necessary.
return passContext.driver.goOffline();
}

Expand All @@ -21,14 +24,13 @@ class Offline extends Gatherer {
* @param {LH.Gatherer.LoadData} loadData
* @return {Promise<LH.Artifacts['Offline']>}
*/
afterPass(passContext, loadData) {
async afterPass(passContext, loadData) {
const navigationRecord = loadData.networkRecords.filter(record => {
return URL.equalWithExcludedFragments(record.url, passContext.url) &&
record.fetchedViaServiceWorker;
}).pop(); // Take the last record that matches.

return passContext.driver.goOnline(passContext)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment around L16 that the goOnline now happens in start-url which sequentially follows this gatherer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually it seems like the goOffline in L16 isnt needed anymore since starturl does it on its own. right

Copy link
Collaborator Author

@patrickhulce patrickhulce Jul 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment around L16 that the goOnline now happens in start-url which sequentially follows this gatherer?

well the goOnline that cleans this up actually happens in gather-runner that was the problem. each afterPass happens while online and each gatherer has to handle its own online/offline cycle. but yes comment is great idea 👍

actually it seems like the goOffline in L16 isnt needed anymore since starturl does it on its own. right

it's still needed to setup the offline for the load of the page, we do the offline check for both the page and the start url separately

.then(_ => navigationRecord ? navigationRecord.statusCode : -1);
return navigationRecord ? navigationRecord.statusCode : -1;
}
}

Expand Down
28 changes: 22 additions & 6 deletions lighthouse-core/gather/gatherers/start-url.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,36 @@ const Gatherer = require('./gatherer.js');

class StartUrl extends Gatherer {
/**
* Grab the manifest, extract it's start_url, attempt to `fetch()` it while offline
* Go offline, assess the start url, go back online.
* @param {LH.Gatherer.PassContext} passContext
* @return {Promise<LH.Artifacts['StartUrl']>}
*/
async afterPass(passContext) {
// `afterPass` is always online, so manually go offline to check start_url.
await passContext.driver.goOffline();
const result = await this._determineStartUrlAvailability(passContext);
await passContext.driver.goOnline(passContext);

return result;
}

/**
* Grab the manifest, extract its start_url, attempt to `fetch()` it while offline
* @param {LH.Gatherer.PassContext} passContext
* @return {Promise<LH.Artifacts['StartUrl']>}
*/
async _determineStartUrlAvailability(passContext) {
const manifest = passContext.baseArtifacts.WebAppManifest;
const startUrlInfo = this._readManifestStartUrl(manifest);
if (startUrlInfo.isReadFailure) {
return {statusCode: -1, explanation: startUrlInfo.reason};
}

return this._attemptStartURLFetch(passContext.driver, startUrlInfo.startUrl).catch(() => {
return {statusCode: -1, explanation: 'Unable to fetch start URL via service worker.'};
});
try {
return await this._attemptStartURLFetch(passContext.driver, startUrlInfo.startUrl);
} catch (err) {
return {statusCode: -1, explanation: 'Error while fetching start_url via service worker.'};
}
}

/**
Expand Down Expand Up @@ -59,7 +75,7 @@ class StartUrl extends Gatherer {
// Wait up to 3s to get a matched network request from the fetch() to work
const timeoutPromise = new Promise(resolve =>
setTimeout(
() => resolve({statusCode: -1, explanation: 'Timed out waiting for fetched start_url.'}),
() => resolve({statusCode: -1, explanation: 'Timed out waiting for start_url to respond.'}),
3000
)
);
Expand All @@ -77,7 +93,7 @@ class StartUrl extends Gatherer {
if (!response.fromServiceWorker) {
return resolve({
statusCode: -1,
explanation: 'Unable to fetch start URL via service worker.',
explanation: 'The start_url did respond, but not via a service worker.',
});
}
// Successful SW-served fetch of the start_URL
Expand Down
Loading