report(pwa): move service-worker to the pwa-optimized group

[adrianaixba]

Follow up to #11745

Create a hidden category renderer group for the service worker audit, remove the is-on-https section from installable category and move service-worker to pwa-optimized group

[patrickhulce]

Is there a specific thread in #11745 that prompted this change? :)

My big question right now is why keep this audit in the PWA category at all if it's not part of installability and not shown anywhere?

[adrianaixba]

Hey @patrickhulce! :)

Is there a specific thread in #11745 that prompted this change? :)

Connor, Brendan, Paul and I talked about this offline !

My big question right now is why keep this audit in the PWA category at all if it's not part of installability and not shown anywhere?

The service worker audit exposes a useful script and scope url for HTTP archive, so we determined that it would be best to keep it and just hide it. I don't believe there was much discussion about it necessarily being in the PWA category, though

[patrickhulce]

Ah OK, if this was already an agreed upon path I'll let someone involved review then :) @paulirish do you want to wrap it up since you've already chimed in?

It feels a little weird to have this be handled differently from the same hidden situation in performance where group is omitted to signal non-visibility, but I suppose the argument was that it makes more sense to use this hidden group for those as well eventually?

[paulirish]

Is there a specific thread in #11745 that prompted this change? :)

yeah a conversation i had with adriana that i then added connor and brendan into. i shoulda added to speakeasy.my bad.

My big question right now is why keep this audit in the PWA category at all if it's not part of installability and not shown anywhere?

it's because of the debugdata on service-worker. adriana added it just a few months ago so that HA could collect data about SW and manifest URLs. we didn't come up with another, more reasonable place for it. so the idea was going hidden like all our perf buddies.

(oops i just see that adriana already said all this.)

It feels a little weird to have this be handled differently from the same hidden situation in performance where group is omitted to signal non-visibility, but I suppose the argument was that it makes more sense to use this hidden group for those as well eventually?

yup, exactly! we'll add 'hidden' to perf, yes. nice and explicitly hidden instead of relying on the "// intentionally left out of metrics group so they won't be displayed" comments. ;)

[paulirish]

this lgtm!

i think you need one more tweak:

diff --git a/lighthouse-viewer/test/viewer-test-pptr.js b/lighthouse-viewer/test/viewer-test-pptr.js
index 45c99f57e..67e32dd22 100644
--- a/lighthouse-viewer/test/viewer-test-pptr.js
+++ b/lighthouse-viewer/test/viewer-test-pptr.js
@@ -107,7 +107,7 @@ describe('Lighthouse Viewer', () => {
         if (category === 'performance') {
           expected = getAuditsOfCategory(category).filter(a => !!a.group);
         }
-        expected = expected.map(audit => audit.id);
+        expected = expected.filter(a => a.group !== 'hidden').map(a => a.id);
         const elementIds = await getAuditElementsIds({category, selector: selectors.audits});
 
         assert.deepStrictEqual(

then it should be good

[paulirish]

• should hidden audits be forced to have a weight of 0?

@brendankenny Thanks for bringing that up, I was asking myself the same. When it comes to the weight, I'm more inclined to think that if it's weighted, it shouldn't be hidden. That it should be clear what's affecting the score.

wfm! we could assert all this when we build the config... oh yeah in assertValidCategories. perfect. @adrianaixba you wanna add that in?

nvm because of the ⬇️ ....

• should service-worker be moved to "PWA Optimized" instead of hidden? Or added info on the new installability audit? In theory the scope URLs are useful debug info but isn't exposed by the protocol, as far as I can tell.

good to raise this yeah......... fwiw we do have some valuable explanations that might help someone debug why their shit is broke

I don't know if any of these checks came from user-need, but it's definitely the case that they'd help if things are broken

sooooo yeah........... i think you just convinced me that we should keep service-worker visible... mostly so these explanations have visibility. (and i guess a doublecheck if installabilityErrors gives unexpected results).

if you two agree.. then I'd propose we walk back the hidden group changes and just move sw audit to PWA Optimized and leave the results as is. (urls still only in debugdata). to adriana's point.. it feels a tiny bit out of place to my brain, but i can imagine a developer looking at the category wouldn't mind at all.

wdyt?

[patrickhulce]

if you two agree.. then I'd propose we walk back the hidden group changes and just move sw audit to PWA Optimized and leave the results as is. (urls still only in debugdata). to adriana's point.. it feels a tiny bit out of place to my brain, but i can imagine a developer looking at the category wouldn't mind at all.

This sounds great to me 👍

[adrianaixba]

if you two agree.. then I'd propose we walk back the hidden group changes and just move sw audit to PWA Optimized and leave the results as is. (urls still only in debugdata). to adriana's point.. it feels a tiny bit out of place to my brain, but i can imagine a developer looking at the category wouldn't mind at all.

wdyt?

@paulirish Good points, I do like the idea of just moving it. I'm for the idea of making this info visible. Would we still want to depend on the installable checks to define the user's installability score? Aka have the sw audit's weight be 0?

[patrickhulce]

Would we still want to depend on the installable checks to define the user's installability score?

I would say, yes, but that should be handled by the fact it's not in the installability group anymore IIRC 🤞

Aka have the sw audit's weight be 0?

I think having some weight in score could still work when it's in PWA Optimized group. I don't personally think it matters very much either way though since we never show the PWA score to the user.

[paulirish]

one small nit and it's good

thanks for rolling with the changes on this one!