new_audit: offline-start-url #6397

brendankenny · 2018-10-26T03:35:18Z

Part of reorganization of the PWA category (#6395).

This splits the start_url offline check out of webapp-install-banner.js and into its own audit.

Based on top of #6396, so you may want to look at the diff between the two: pwacat...start_url

brendankenny · 2018-10-26T19:03:59Z

rebased off master

brendankenny · 2018-10-26T19:04:43Z

lighthouse-core/audits/multi-check-audit.js

@@ -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}>}


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

brendankenny · 2018-10-26T19:05:22Z

lighthouse-core/config/default-config.js

@@ -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'},


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

brendankenny · 2018-10-26T19:06:06Z

lighthouse-core/gather/gatherers/start-url.js

@@ -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.'};


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

brendankenny · 2018-10-26T19:06:49Z

lighthouse-core/gather/gatherers/start-url.js

-    }
-
-    // @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.


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

brendankenny · 2018-10-26T19:07:29Z

lighthouse-core/lib/manifest-parser.js

+      raw,
+      value: documentUrl,
+      warning: 'ERROR: expected a string.',
+    };


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)

brendankenny · 2018-10-26T19:08:56Z

lighthouse-core/test/gather/gatherers/start-url-test.js

-      });
-  });
-
-  it('returns an error when origin is not the same', () => {


not an error, a warning

is it easy to convert to a test for the warning?

is it easy to convert to a test for the warning?

the gatherer was set up to either fail and return an explanation or succeed and have no explanation, so making it not fail in that case means no more warning from the manifest parser. But both webapp-install-banner and offline-start-url both check the warning straight from the Manifest artifact (and both have tests)

patrickhulce

looks good to me outside i18n question! boy it is going to be so hard to remember all the various states of checking across LH versions 😩

patrickhulce · 2018-10-26T19:29:41Z

lighthouse-core/audits/offline-start-url.js

+  static get meta() {
+    return {
+      id: 'offline-start-url',
+      title: 'start_url responds with a 200 when offline',


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

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..."

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

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...

patrickhulce · 2018-10-26T19:30:22Z

lighthouse-core/audits/offline-start-url.js

+      id: 'offline-start-url',
+      title: 'start_url responds with a 200 when offline',
+      failureTitle: 'start_url does not respond with a 200 when offline',
+      description: 'If you\'re building a Progressive Web App, consider using a service worker ' +


feels a bit out of the ordinary for the PWA, but I like the qualification. Maybe we should prefix the other audit descriptions too?

how about

A service worker enables your web app to be reliable in unpredictable network conditions [Learn more]

how about

Sure.

It's also a little weird that both this and the offline check (checking the current page) have very similar descriptions, but I'm sure we can work on tweaking that.

patrickhulce · 2018-10-26T19:33:11Z

lighthouse-core/test/gather/gatherers/start-url-test.js

-      });
-  });
-
-  it('returns an error when origin is not the same', () => {


is it easy to convert to a test for the warning?

paulirish · 2018-10-26T21:58:26Z

lighthouse-core/audits/offline-start-url.js

+      id: 'offline-start-url',
+      title: 'start_url responds with a 200 when offline',
+      failureTitle: 'start_url does not respond with a 200 when offline',
+      description: 'If you\'re building a Progressive Web App, consider using a service worker ' +


how about

A service worker enables your web app to be reliable in unpredictable network conditions [Learn more]

paulirish · 2018-10-26T22:01:33Z

lighthouse-core/audits/offline-start-url.js

+    const warnings = [];
+    const manifest = artifacts.Manifest;
+    if (manifest && manifest.value && manifest.value.start_url.warning) {
+      warnings.push('There was an error in the manifest and so the start_url fell back to the ' +


What conditions lead to this?

Wording seems a bit odd to me.

We couldn't read the start_url from the manifest. As a result, the start_url was assumed to be the document's URL. ${warning}

What conditions lead to this?

If you do something like {start_url: 5} as your manifest.

(more realistically, maybe you have a typo in the URL making it invalid, or it's a URL that's not same origin as the page being tested)

Wording seems a bit odd to me.

done

paulirish · 2018-10-26T22:04:44Z

lighthouse-core/audits/offline-start-url.js

+  static get meta() {
+    return {
+      id: 'offline-start-url',
+      title: 'start_url responds with a 200 when offline',


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.

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?

brendankenny · 2018-10-26T22:49:00Z

looks good to me outside i18n question! boy it is going to be so hard to remember all the various states of checking across LH versions

yeah, I don't envy anyone doing that :(

exterkamp

LGTM. Just a nit.

exterkamp · 2018-10-27T00:14:42Z

lighthouse-core/audits/offline-start-url.js

+  static get meta() {
+    return {
+      id: 'offline-start-url',
+      title: 'start_url responds with a 200 when offline',


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

exterkamp · 2018-10-27T00:33:06Z

lighthouse-core/audits/offline-start-url.js

+      id: 'offline-start-url',
+      title: '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).',


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.."

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

exterkamp · 2018-10-27T00:49:45Z

lighthouse-core/test/results/sample_v2.json

+    "offline-start-url": {
+      "id": "offline-start-url",
+      "title": "start_url does not respond with a 200 when offline",
+      "description": "If you're building a Progressive Web App, consider using a service worker so that your app can work offline. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/http-200-when-offline).",


yarn update:sample-json

yarn update:sample-json

done

brendankenny requested review from exterkamp, patrickhulce and paulirish as code owners October 26, 2018 03:35

brendankenny force-pushed the start_url branch 4 times, most recently from aab1fc2 to 443e3ba Compare October 26, 2018 19:03

brendankenny commented Oct 26, 2018

View reviewed changes

brendankenny mentioned this pull request Oct 26, 2018

PWA category refactor tracking issue #6395

Closed

31 tasks

patrickhulce reviewed Oct 26, 2018

View reviewed changes

paulirish reviewed Oct 26, 2018

View reviewed changes

exterkamp approved these changes Oct 27, 2018

View reviewed changes

paulirish approved these changes Oct 30, 2018

View reviewed changes

brendankenny added 6 commits October 30, 2018 11:40

new_audit: offline-start-url

33a6227

add to config, smoketests

b341a99

golden json x 3, isn't it great

46a1b9a

feedback and test fix

788ca28

golden

326952e

urg

c3c0a4b

brendankenny force-pushed the start_url branch from 08a9488 to c3c0a4b Compare October 30, 2018 18:41

brendankenny merged commit c8ebbe6 into master Oct 30, 2018

brendankenny deleted the start_url branch October 30, 2018 20:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new_audit: offline-start-url #6397

new_audit: offline-start-url #6397

brendankenny commented Oct 26, 2018

brendankenny commented Oct 26, 2018

brendankenny Oct 26, 2018

brendankenny Oct 26, 2018 •

edited

Loading

brendankenny Oct 26, 2018

brendankenny Oct 26, 2018

brendankenny Oct 26, 2018 •

edited

Loading

brendankenny Oct 26, 2018

patrickhulce Oct 26, 2018

brendankenny Oct 26, 2018

patrickhulce left a comment

patrickhulce Oct 26, 2018

brendankenny Oct 26, 2018

exterkamp Oct 27, 2018

brendankenny Oct 29, 2018

patrickhulce Oct 26, 2018

paulirish Oct 26, 2018

brendankenny Oct 26, 2018

patrickhulce Oct 26, 2018

paulirish Oct 26, 2018

paulirish Oct 26, 2018

brendankenny Oct 26, 2018

paulirish Oct 26, 2018

brendankenny Oct 26, 2018

brendankenny commented Oct 26, 2018

exterkamp left a comment

exterkamp Oct 27, 2018

exterkamp Oct 27, 2018

brendankenny Oct 29, 2018

exterkamp Oct 27, 2018

brendankenny Oct 29, 2018

new_audit: offline-start-url #6397

new_audit: offline-start-url #6397

Conversation

brendankenny commented Oct 26, 2018

brendankenny commented Oct 26, 2018

Choose a reason for hiding this comment

brendankenny Oct 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Oct 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Oct 26, 2018

exterkamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Oct 26, 2018 •

edited

Loading

brendankenny Oct 26, 2018 •

edited

Loading