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

new_audit: offline-start-url #6397

Merged
merged 6 commits into from
Oct 30, 2018
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
10 changes: 9 additions & 1 deletion lighthouse-cli/test/cli/__snapshots__/index-test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,9 @@ Object {
Object {
"path": "metrics",
},
Object {
"path": "offline-start-url",
},
Object {
"path": "manual/pwa-cross-browser",
},
Expand Down Expand Up @@ -820,6 +823,11 @@ Object {
"id": "works-offline",
"weight": 5,
},
Object {
"group": "pwa-fast-reliable",
"id": "offline-start-url",
"weight": 1,
},
Object {
"group": "pwa-installable",
"id": "is-on-https",
Expand All @@ -833,7 +841,7 @@ Object {
Object {
"group": "pwa-installable",
"id": "webapp-install-banner",
"weight": 3,
"weight": 2,
},
Object {
"group": "pwa-engaging",
Expand Down
6 changes: 6 additions & 0 deletions lighthouse-cli/test/smokehouse/pwa-expectations.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ module.exports = [
'works-offline': {
score: 1,
},
'offline-start-url': {
score: 1,
},
'viewport': {
score: 1,
},
Expand Down Expand Up @@ -95,6 +98,9 @@ module.exports = [
'works-offline': {
score: 0,
},
'offline-start-url': {
score: 1,
},
'viewport': {
score: 1,
},
Expand Down
6 changes: 6 additions & 0 deletions lighthouse-cli/test/smokehouse/pwa2-expectations.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ module.exports = [
'works-offline': {
score: 1,
},
'offline-start-url': {
score: 1,
},
'viewport': {
score: 1,
},
Expand Down Expand Up @@ -89,6 +92,9 @@ module.exports = [
'works-offline': {
score: 1,
},
'offline-start-url': {
score: 1,
},
'viewport': {
score: 1,
},
Expand Down
3 changes: 3 additions & 0 deletions lighthouse-cli/test/smokehouse/pwa3-expectations.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ module.exports = [
'works-offline': {
score: 1,
},
'offline-start-url': {
score: 1,
},
'viewport': {
score: 1,
},
Expand Down
5 changes: 2 additions & 3 deletions lighthouse-core/audits/multi-check-audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class MultiCheckAudit extends Audit {
}

/**
* @param {{failures: Array<string>, warnings?: Array<string>, manifestValues?: LH.Artifacts.ManifestValues}} result
* @param {{failures: Array<string>, manifestValues?: LH.Artifacts.ManifestValues}} result
* @return {LH.Audit.Product}
*/
static createAuditProduct(result) {
Expand Down Expand Up @@ -57,7 +57,6 @@ class MultiCheckAudit extends Audit {
return {
rawValue: true,
details,
warnings: result.warnings,
};
}

Expand All @@ -66,7 +65,7 @@ class MultiCheckAudit extends Audit {
/**
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} context
* @return {Promise<{failures: Array<string>, warnings?: Array<string>, manifestValues?: LH.Artifacts.ManifestValues}>}
* @return {Promise<{failures: Array<string>, manifestValues?: LH.Artifacts.ManifestValues}>}
Copy link
Member Author

Choose a reason for hiding this comment

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

webapp-install-banner was the only one using warnings for start_url warnings, so no longer needed

*/
static audit_(artifacts, context) {
throw new Error('audit_ unimplemented');
Expand Down
49 changes: 49 additions & 0 deletions lighthouse-core/audits/offline-start-url.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/**
* @license Copyright 2018 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

const Audit = require('./audit.js');

class OfflineStartUrl extends Audit {
/**
* @return {LH.Audit.Meta}
*/
static get meta() {
return {
id: 'offline-start-url',
title: 'start_url responds with a 200 when offline',
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we be trying to i18n new strings in general? feels like we should

unrelated: start_url feels so weird in the title like that haha

Copy link
Member Author

Choose a reason for hiding this comment

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

should we be trying to i18n new strings in general? feels like we should

I like doing them with everyone all at once so I can avoid responsibility for them, but should we do them as we go?

unrelated: start_url feels so weird in the title like that haha

haha, I felt the same, but it felt worse to just have them be words. "Start URL responds with a 200..."

Copy link
Member

Choose a reason for hiding this comment

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

Maybe "Service Worker's start_url responds with a 200"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe "Service Worker's start_url responds with a 200"?

yeah, it's not the service worker's start_url, but the service worker needs to respond to the request for the start_url...

Copy link
Member

Choose a reason for hiding this comment

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

There's a text update on the works-offline audit. Something like "Current page... responds with a 200"

dunno if you want to do that here but we shouldn't drop it.

Copy link
Member Author

Choose a reason for hiding this comment

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

here's a text update on the works-offline audit. Something like "Current page... responds with a 200"

dunno if you want to do that here but we shouldn't drop it.

The current proposed update is "Current page responds with a 200 when offline". I modeled this after that...not sure what you mean by dropping it, though.

Page at start_url responds with a 200 when offline?

failureTitle: 'start_url does not respond with a 200 when offline',
description: 'A service worker enables your web app to be reliable in unpredictable network conditions. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/http-200-when-offline).',
Copy link
Member

Choose a reason for hiding this comment

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

nit: I feel like this maybe isn't specific enough? Someone might have a service worker but have a bad start_url that would be flagged by this audit.

"A service worker with a valid start_url enables your web app.."

Copy link
Member Author

Choose a reason for hiding this comment

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

Someone might have a service worker but have a bad start_url that would be flagged by this audit.

yeah, I'm just not sure how to word it. It's not the service worker that has the start_url, it's the manifest, though the service worker needs to serve it. Maybe we can tweak

requiredArtifacts: ['Manifest', 'StartUrl'],
};
}

/**
* @param {LH.Artifacts} artifacts
* @return {LH.Audit.Product}
*/
static audit(artifacts) {
// StartUrl gatherer will give explanations for failures, but need to take manifest parsing
// warnings from the manifest itself (e.g. invalid `start_url`, so fell back to document URL).
const warnings = [];
const manifest = artifacts.Manifest;
if (manifest && manifest.value && manifest.value.start_url.warning) {
const manifestWarning = manifest.value.start_url.warning;
warnings.push('We couldn\'t read the start_url from the manifest. As a result, the ' +
`start_url was assumed to be the document's URL. Error message: '${manifestWarning}'.`);
}

const hasOfflineStartUrl = artifacts.StartUrl.statusCode === 200;

return {
rawValue: hasOfflineStartUrl,
explanation: artifacts.StartUrl.explanation,
warnings,
};
}
}

module.exports = OfflineStartUrl;
39 changes: 2 additions & 37 deletions lighthouse-core/audits/webapp-install-banner.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class WebappInstallBanner extends MultiCheckAudit {
description: 'Browsers can proactively prompt users to add your app to their homescreen, ' +
'which can lead to higher engagement. ' +
'[Learn more](https://developers.google.com/web/tools/lighthouse/audits/install-prompt).',
requiredArtifacts: ['URL', 'ServiceWorker', 'Manifest', 'StartUrl'],
requiredArtifacts: ['URL', 'ServiceWorker', 'Manifest'],
};
}

Expand Down Expand Up @@ -94,55 +94,20 @@ class WebappInstallBanner extends MultiCheckAudit {
return failures;
}

/**
* @param {LH.Artifacts} artifacts
* @return {{failures: Array<string>, warnings: Array<string>}}
*/
static assessOfflineStartUrl(artifacts) {
const failures = [];
const warnings = [];
const hasOfflineStartUrl = artifacts.StartUrl.statusCode === 200;

if (!hasOfflineStartUrl) {
failures.push('Service worker does not successfully serve the manifest\'s start_url');
if (artifacts.StartUrl.explanation) {
failures.push(artifacts.StartUrl.explanation);
}
}

if (artifacts.StartUrl.explanation) {
warnings.push(artifacts.StartUrl.explanation);
}

return {failures, warnings};
}

/**
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} context
* @return {Promise<{failures: Array<string>, warnings: Array<string>, manifestValues: LH.Artifacts.ManifestValues}>}
* @return {Promise<{failures: Array<string>, manifestValues: LH.Artifacts.ManifestValues}>}
*/
static async audit_(artifacts, context) {
/** @type {Array<string>} */
let offlineFailures = [];
/** @type {Array<string>} */
let offlineWarnings = [];

const manifestValues = await ManifestValues.request(artifacts.Manifest, context);
const manifestFailures = WebappInstallBanner.assessManifest(manifestValues);
const swFailures = WebappInstallBanner.assessServiceWorker(artifacts);
if (!swFailures.length) {
const {failures, warnings} = WebappInstallBanner.assessOfflineStartUrl(artifacts);
offlineFailures = failures;
offlineWarnings = warnings;
}

return {
warnings: offlineWarnings,
failures: [
...manifestFailures,
...swFailures,
...offlineFailures,
],
manifestValues,
};
Expand Down
4 changes: 3 additions & 1 deletion lighthouse-core/config/default-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ const defaultConfig = {
'font-display',
'network-requests',
'metrics',
'offline-start-url',
'manual/pwa-cross-browser',
'manual/pwa-page-transitions',
'manual/pwa-each-page-has-url',
Expand Down Expand Up @@ -334,10 +335,11 @@ const defaultConfig = {
// Fast and Reliable
{id: 'load-fast-enough-for-pwa', weight: 7, group: 'pwa-fast-reliable'},
{id: 'works-offline', weight: 5, group: 'pwa-fast-reliable'},
{id: 'offline-start-url', weight: 1, group: 'pwa-fast-reliable'},
Copy link
Member Author

@brendankenny brendankenny Oct 26, 2018

Choose a reason for hiding this comment

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

keeps the scores the same as before for now (webapp-install-banner split in two)

// Installable
{id: 'is-on-https', weight: 2, group: 'pwa-installable'},
{id: 'service-worker', weight: 1, group: 'pwa-installable'},
{id: 'webapp-install-banner', weight: 3, group: 'pwa-installable'},
{id: 'webapp-install-banner', weight: 2, group: 'pwa-installable'},
// Engaging
{id: 'redirects-http', weight: 2, group: 'pwa-engaging'},
{id: 'splash-screen', weight: 1, group: 'pwa-engaging'},
Expand Down
23 changes: 9 additions & 14 deletions lighthouse-core/gather/gatherers/start-url.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@

const Gatherer = require('./gatherer');
const manifestParser = require('../../lib/manifest-parser');
const Driver = require('../driver.js'); // eslint-disable-line no-unused-vars

/** @typedef {import('../driver.js')} Driver */

class StartUrl extends Gatherer {
/**
Expand All @@ -29,33 +30,27 @@ class StartUrl extends Gatherer {

return this._attemptManifestFetch(passContext.driver, startUrlInfo.startUrl);
}).catch(() => {
return {statusCode: -1, explanation: 'Unable to fetch start URL via service worker'};
return {statusCode: -1, explanation: 'Unable to fetch start URL via service worker.'};
Copy link
Member Author

Choose a reason for hiding this comment

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

periods added to look better in the report :) (this is currently the only audit using this gatherer)

});
}

/**
* Read the parsed manifest and return failure reasons or the startUrl
* @param {?{value?: {start_url: {value?: string, warning?: string}}, warning?: string}} manifest
* @param {?{value?: {start_url: {value: string, warning?: string}}, warning?: string}} manifest
* @return {{isReadFailure: true, reason: string}|{isReadFailure: false, startUrl: string}}
*/
_readManifestStartUrl(manifest) {
if (!manifest || !manifest.value) {
const detailedMsg = manifest && manifest.warning;

if (detailedMsg) {
return {isReadFailure: true, reason: `Error fetching web app manifest: ${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 {isReadFailure: true, reason: `No usable web app manifest found on page.`};
}
}

// Even if the start URL had an error, the browser will still supply a fallback URL.
// Therefore, we only set the warning here and continue with the fetch.
if (manifest.value.start_url.warning) {
return {isReadFailure: true, reason: manifest.value.start_url.warning};
}

// @ts-ignore - TODO(bckenny): should actually be testing value above, not warning
// Even if the start URL had a parser warning, the browser will still supply a fallback URL.
Copy link
Member Author

Choose a reason for hiding this comment

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

this was wrong before. A manifest parsing warning doesn't mean there was a read failure, since the parser falls back to the document URL

return {isReadFailure: false, startUrl: manifest.value.start_url.value};
}

Expand All @@ -70,7 +65,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 fetched start_url.'}),
3000
)
);
Expand All @@ -88,7 +83,7 @@ class StartUrl extends Gatherer {
if (!response.fromServiceWorker) {
return resolve({
statusCode: -1,
explanation: 'Unable to fetch start URL via service worker',
explanation: 'Unable to fetch start URL via service worker.',
});
}
// Successful SW-served fetch of the start_URL
Expand Down
17 changes: 13 additions & 4 deletions lighthouse-core/lib/manifest-parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ function checkSameOrigin(url1, url2) {
* @param {*} jsonInput
* @param {string} manifestUrl
* @param {string} documentUrl
* @return {{raw: any, value: string, warning?: string}}
*/
function parseStartUrl(jsonInput, manifestUrl, documentUrl) {
const raw = jsonInput.start_url;
Expand All @@ -127,10 +128,18 @@ function parseStartUrl(jsonInput, manifestUrl, documentUrl) {
warning: 'ERROR: start_url string empty',
};
}
const parsedAsString = parseString(raw);
if (!parsedAsString.value) {
parsedAsString.value = documentUrl;
return parsedAsString;
if (raw === undefined) {
return {
raw,
value: documentUrl,
};
}
if (typeof raw !== 'string') {
return {
raw,
value: documentUrl,
warning: 'ERROR: expected a string.',
};
Copy link
Member Author

@brendankenny brendankenny Oct 26, 2018

Choose a reason for hiding this comment

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

equivalent to before, but a lot easier to understand what happens to undefined and non strings (edit prompted by tsc thinking the value property was optional, when it actually always falls back to documentUrl no matter how bad the start_url mess up is)

}

// 8.10(4) - construct URL with raw as input and manifestUrl as the base.
Expand Down
Loading