core(start-url): stay offline for entirety of offlinePass #9451

patrickhulce · 2019-07-24T21:59:07Z

Summary
tl;dr - StartUrl gatherer is not actually offline while it is checking for service worker start URL fetching. This PR fixes that.

In #6957, the start url gatherer was refactored to allow WebAppManifest to be part of core artifacts and avoid being double fetched. The code appeared to assume that the page was offline at the start of the afterPass because it went back online to fetch the manifest before going back offline. This wasn't actually the case though. Due to refactorings long ago in gather runner we actually always run the entirety of afterPass while online.

For pages with service workers that are always controlling the page this doesn't end up making much of a difference because of good ol' #709. However, if the service worker only handles requests while the page is in an offline state (not really clear how one does this), then all requests actually go out over the network and we trip our !response.fromServiceWorker line to falsely fail the page (which is normally a good thing because it means that all pages that didn't have a service worker didn't magically start passing when this bug was introduced! 😄)

Related Issues/PRs
fixes #9450

brendankenny

nice catch. Ugh this gatherer just won't ever be happy :)

lighthouse-core/gather/gatherers/offline.js

brendankenny · 2019-07-25T01:20:47Z

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

@@ -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: 'Fetched start URL but not via service worker.',


it seems like this message is more confusing than the original (should this audit care about that it's fetchable like that?)

Right now the message is indistinguishable from not being able to fetch it at all which is a problem IMO. The fact that it fetched but was falling back to disk cache or something is worth flagging. Totally up for whatever wording others feel captures this goal.

The start URL did respond, but not via a service worker

mostly trying to align with the text of the audit.. (where we dont talk about fetching, etc)

The fact that it fetched but was falling back to disk cache or something is worth flagging

I don't know if everyone knows the priority order here, though. If I'm looking at the start_url audit LH tells me

The start URL did respond, but not via a service worker

my first question would be "well why didn't LH try fetching via the SW? Do I need to force it somehow?"

It seems like the only important information is that LH tried to fetch it and the SW didn't return it. Just because it was in the disk cache during the LH run doesn't mean it will be for returning users, and isn't that the case we're testing for?

Alternatively, should we just call out the fromDiskCache case with a separate explanation?

paulirish · 2019-07-25T20:51:45Z

lighthouse-core/gather/gatherers/offline.js

    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)


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

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

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

paulirish

nit on wording otherwise lgtm

paulirish · 2019-08-14T03:22:04Z

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

@@ -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: 'Fetched start URL but not via service worker.',


The start URL did respond, but not via a service worker

mostly trying to align with the text of the audit.. (where we dont talk about fetching, etc)

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

brendankenny · 2019-08-14T20:36:18Z

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

+      evaluateAsync: unimplemented,
+      on: unimplemented,
+      off: unimplemented,
+    };


moving off bespoke mocks is good, but I have to say this is a nice model for this file :)

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

brendankenny · 2019-08-14T20:39:45Z

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

-      assert.equal(artifactWithFragment.statusCode, 200);
+    const result = await gatherer.afterPass(passContext);
+    expect(mockDriver.goOffline).toHaveBeenCalled();
+    expect(mockDriver.goOnline).toHaveBeenCalled();


what do you think about an extra level of checking for these that it was offline when evaluateAsync was called, not just that off and on were both called?

I like it as a separate test but not on every single one :) done!

core(start-url): fetch start url while offline

b706bab

patrickhulce requested a review from paulirish as a code owner July 24, 2019 21:59

googlebot added the cla: yes label Jul 24, 2019

patrickhulce changed the title ~~core(offline): stay offline for entirety of offlinePass~~ core(start-url): stay offline for entirety of offlinePass Jul 24, 2019

brendankenny reviewed Jul 25, 2019

View reviewed changes

lint

d96b796

vercel bot temporarily deployed to staging July 25, 2019 15:16 Inactive

Merge branch 'master' into offline_for_start_url

828e71b

vercel bot deployed to staging July 25, 2019 15:16 View deployment

paulirish reviewed Jul 25, 2019

View reviewed changes

add comment

c450ce3

vercel bot deployed to staging July 25, 2019 21:47 View deployment

paulirish approved these changes Aug 14, 2019

View reviewed changes

patrickhulce added 2 commits August 14, 2019 10:30

wording tweaks

1d40c11

Merge branch 'master' into offline_for_start_url

7711ef2

vercel bot temporarily deployed to staging August 14, 2019 15:30 Inactive

vercel bot deployed to staging August 14, 2019 15:30 View deployment

brendankenny reviewed Aug 14, 2019

View reviewed changes

feedback

6820ccf

vercel bot deployed to staging August 14, 2019 21:23 View deployment

lint

b3a1825

vercel bot deployed to staging August 14, 2019 21:29 View deployment

patrickhulce merged commit 6861412 into master Aug 15, 2019

patrickhulce deleted the offline_for_start_url branch August 15, 2019 19:03

patrickhulce mentioned this pull request Aug 27, 2019

Lighthouse 5.2.0 mistakes browser window width for device width in mobile audit #9476

Closed

wardpeet mentioned this pull request Sep 3, 2019

Update throttling/cache/js state on the SW target #709

Open

paulirish pushed a commit that referenced this pull request Nov 6, 2019

core(start-url): stay offline for entirety of offlinePass (#9451)

7c03369

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(start-url): stay offline for entirety of offlinePass #9451

core(start-url): stay offline for entirety of offlinePass #9451

patrickhulce commented Jul 24, 2019

brendankenny left a comment

brendankenny Jul 25, 2019

patrickhulce Jul 25, 2019

paulirish Aug 14, 2019

brendankenny Aug 14, 2019

paulirish Jul 25, 2019

paulirish Jul 25, 2019

patrickhulce Jul 25, 2019 •

edited

Loading

paulirish left a comment

paulirish Aug 14, 2019

brendankenny Aug 14, 2019

brendankenny Aug 14, 2019

patrickhulce Aug 14, 2019

core(start-url): stay offline for entirety of offlinePass #9451

core(start-url): stay offline for entirety of offlinePass #9451

Conversation

patrickhulce commented Jul 24, 2019

brendankenny 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

patrickhulce Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

paulirish 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

patrickhulce Jul 25, 2019 •

edited

Loading