core: expose service worker url on service worker audit #11329

adrianaixba · 2020-08-26T22:36:52Z

exposing service worker url, and making it available on the service worker audit

paulirish · 2020-08-27T19:11:17Z

lighthouse-core/audits/service-worker.js

-      .map(v => registrations.find(r => r.registrationId === v.registrationId))
-      .filter(/** @return {r is LH.Crdp.ServiceWorker.ServiceWorkerRegistration} */ r => !!r)
-      .map(r => new URL(r.scopeURL).href);
+    const serviceWorkerUrls = new Map();


we've been slowly adopting a foosByBars convention for our map names.. i think it'd help here... like scriptUrlByScopeUrl

paulirish · 2020-08-27T19:19:41Z

lighthouse-core/audits/service-worker.js

+
+    // Populate serviceWorkerUrls map with the scriptURLs and scopeUrls of matchingSWVersions and registrations
+    matchingSWVersions.forEach(function(version) {
+      const tempRegistration = registrations.find(r => r.registrationId === version.registrationId);


matchedRegistration ? this signal is just that we have a match... and thus can continue, yeah?

paulirish · 2020-08-27T19:26:49Z

lighthouse-core/audits/service-worker.js

@@ -73,23 +73,31 @@ class ServiceWorker extends Audit {
   * @param {Array<LH.Crdp.ServiceWorker.ServiceWorkerVersion>} matchingSWVersions
   * @param {Array<LH.Crdp.ServiceWorker.ServiceWorkerRegistration>} registrations
   * @param {URL} pageUrl
-   * @return {string|undefined}
+   * @return {Array<string> | undefined}
   */
  static getControllingScopeUrl(matchingSWVersions, registrations, pageUrl) {


now that this returns more than just the scope url we should rename it... perhaps getControllingServiceWorker

paulirish · 2020-08-27T19:36:21Z

lighthouse-core/audits/service-worker.js

-      .filter(scopeUrl => pageUrl.href.startsWith(scopeUrl))
-      .sort((scopeA, scopeB) => scopeA.length - scopeB.length)
+    const pageControllingUrls = [...serviceWorkerUrls]
+      /* converts map to array, to properly filer and sort according to scopeUrl */


using a Map() is good, but right now it pretty quickly is thrown away and we turn it into the [['scope1', 'scriptA'],['scope2','scriptB']] representation. So we don't get much value out of it.

and separately the current return value is an array of strings, but you kinda have to know which is which.. I'd feel better making it explicit.

i remember in an earlier version you had named properties. i think within the matchingScopeUrls functional methods you ended up making a Array<{scopeUrl: string, scriptUrl: string}> or something

@adrianaixba you were right and that was better. :) i misled you as i thought doing that wasn't necessary. mea cupla.

so yeah looking at it now.. the best return value for this getControllingServiceWorker() method would probably be {{scopeUrl: string, scriptUrl: string}}, so we might as well build that from the start.

paulirish · 2020-08-27T19:38:39Z

lighthouse-core/test/audits/service-worker-test.js

-    assert.deepStrictEqual(output, {score: 1});
+    assert.deepStrictEqual(output.score, 1);
+    assert.deepStrictEqual(output.details.items[0].scopeURL, 'https://example.com/project/');
+    assert.deepStrictEqual(output.details.items[0].scriptURL, 'https://example.com/project/sw.js');


this is satisfying. and exactly what the test should have been asserting beforehand anyway. without this assertion (/project/sw is the controlling sw), the test was kinda useless.

…erviceWorker more clear

connorjclark · 2020-08-28T18:38:31Z

lighthouse-core/audits/service-worker.js

-      .map(v => registrations.find(r => r.registrationId === v.registrationId))
-      .filter(/** @return {r is LH.Crdp.ServiceWorker.ServiceWorkerRegistration} */ r => !!r)
-      .map(r => new URL(r.scopeURL).href);
+    /** @type {{ scopeUrl: string; scriptUrl: string; }[]} */


nit: prefer Array<...>, especially for complex types

there's a bit of an unofficial war between me, @patrickhulce and @brendankenny about when to use Array<...> vs ...[] :) I prefer using [] for simple, non-compound types like string[] or LH.Artifact.Blah[], but others prefer always using it. eventually you may pick a side ...

for the record I also have absolutely no problem with string[] or number[] :)

I think we all agree when the types grow to be more complex (I'd probably switch earlier than @connorjclark and use Array<LH.Nested.Thing>) the Array<T> form is easier to read 👍

connorjclark · 2020-08-28T18:39:05Z

lighthouse-core/audits/service-worker.js

+    const scriptByScopeUrlList = [];
+
+    // Populate serviceWorkerUrls map with the scriptURLs and scopeUrls of matchingSWVersions and registrations
+    matchingSWVersions.forEach(function(version) {


nit: prefer for (const blah of blahs) over blahs.forEach.

connorjclark · 2020-08-28T19:33:18Z

lighthouse-core/audits/service-worker.js

-      .map(v => registrations.find(r => r.registrationId === v.registrationId))
-      .filter(/** @return {r is LH.Crdp.ServiceWorker.ServiceWorkerRegistration} */ r => !!r)
-      .map(r => new URL(r.scopeURL).href);
+    /** @type {Array<{ scopeUrl: string; scriptUrl: string;}>} */


Suggested change

/** @type {Array<{ scopeUrl: string; scriptUrl: string;}>} */

/** @type {Array<{scopeUrl: string; scriptUrl: string}>} */

Beytoven · 2020-08-28T19:39:05Z

lighthouse-core/audits/service-worker.js

-      .filter(scopeUrl => pageUrl.href.startsWith(scopeUrl))
-      .sort((scopeA, scopeB) => scopeA.length - scopeB.length)
+    const pageControllingUrls = scriptByScopeUrlList
+      .filter(urlPair => pageUrl.href.startsWith(urlPair.scopeUrl.toString()))


Nit: I believe urlPair.scopeUrl is already of type string so the .toString() isn't needed.

paulirish

another driveby from me. i'm the worst.

edit: ummmmmmmmmmmmmm i wrote a mediumsize comment but github lost it? :/

edit edit: shrug. ah well. i rewrote it.

paulirish · 2020-08-28T19:54:03Z

lighthouse-core/audits/service-worker.js

-      .sort((scopeA, scopeB) => scopeA.length - scopeB.length)
+    const pageControllingUrls = scriptByScopeUrlList
+      .filter(urlPair => pageUrl.href.startsWith(urlPair.scopeUrl.toString()))
+      .sort((scopeA, scopeB) => scopeA.scopeUrl.length - scopeB.scopeUrl.length)


extra nit: we were calling this urlPair when it was an array of strings, but now its a typed object... so the identity and type is pretty clear.

also we call this obj urlPair here on 98 but then scopeA/B on 99. we should keep it consistent.

in these sorts of cases we often go for short/singleletter var names... like this:

lighthouse/lighthouse-core/audits/service-worker.js

Lines 65 to 67 in 960d7c7

return versions

.filter(v => v.status === 'activated')

.filter(v => new URL(v.scriptURL).origin === pageUrl.origin);

so perhaps ss and ssA/B ?

Beytoven

LGTM!

…#11329)

adrianaixba and others added 5 commits August 19, 2020 15:56

first commit

903c623

added tests

21f1535

test commit

24aed18

clean up

fd263ca

clean up

eb848d7

adrianaixba requested a review from a team as a code owner August 26, 2020 22:36

adrianaixba requested review from Beytoven and removed request for a team August 26, 2020 22:36

googlebot added the cla: yes label Aug 26, 2020

devtools-bot assigned Beytoven Aug 26, 2020

devtools-bot added the waiting4reviewer label Aug 26, 2020

paulirish reviewed Aug 27, 2020

View reviewed changes

small variable name changes, cleaned up uneeded code, getControllingS…

3680440

…erviceWorker more clear

vercel bot deployed to Preview August 28, 2020 17:57 View deployment

max-len lint :(

3e32e3f

vercel bot deployed to Preview August 28, 2020 18:20 View deployment

connorjclark reviewed Aug 28, 2020

View reviewed changes

clean ups

29ed8f3

vercel bot deployed to Preview August 28, 2020 19:26 View deployment

connorjclark reviewed Aug 28, 2020

View reviewed changes

Beytoven reviewed Aug 28, 2020

View reviewed changes

paulirish reviewed Aug 28, 2020

View reviewed changes

clean up

84e09b6

vercel bot deployed to Preview August 28, 2020 20:40 View deployment

Beytoven approved these changes Aug 28, 2020

View reviewed changes

adrianaixba merged commit ccbc800 into master Aug 28, 2020

adrianaixba deleted the service-worker-script-urls branch August 28, 2020 22:57

connorjclark mentioned this pull request Sep 1, 2020

Surface manifest and service worker URLs in JSON results #9683

Closed

brendankenny mentioned this pull request Sep 1, 2020

misc: flatten new service-worker audit details #11361

Merged

gMakunde pushed a commit to gMakunde/lighthouse that referenced this pull request Sep 1, 2020

core: expose service worker url on service worker audit (GoogleChrome…

49413a7

…#11329)

adrianaixba removed the waiting4reviewer label Sep 16, 2020

connorjclark mentioned this pull request Jan 24, 2022

gather-runner: can we reduce to just one pass? #11822

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: expose service worker url on service worker audit #11329

core: expose service worker url on service worker audit #11329

adrianaixba commented Aug 26, 2020

paulirish Aug 27, 2020

paulirish Aug 27, 2020

paulirish Aug 27, 2020

paulirish Aug 27, 2020

paulirish Aug 27, 2020

connorjclark Aug 28, 2020

patrickhulce Aug 28, 2020

connorjclark Aug 28, 2020

connorjclark Aug 28, 2020

Beytoven Aug 28, 2020

paulirish left a comment •

edited

Loading

paulirish Aug 28, 2020

Beytoven left a comment

	/** @type {Array<{ scopeUrl: string; scriptUrl: string;}>} */
	/** @type {Array<{scopeUrl: string; scriptUrl: string}>} */

	return versions
	.filter(v => v.status === 'activated')
	.filter(v => new URL(v.scriptURL).origin === pageUrl.origin);

core: expose service worker url on service worker audit #11329

core: expose service worker url on service worker audit #11329

Conversation

adrianaixba commented Aug 26, 2020

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

paulirish left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Beytoven left a comment

Choose a reason for hiding this comment

paulirish left a comment •

edited

Loading