core(fr): add `supportedModes` filter to categories #13161

adamraine · 2021-10-01T23:51:01Z

This is a fancy way of hiding PWA in timespan and snapshot. Filter is defined in the config.

adamraine · 2021-10-01T23:51:33Z

flow-report/test/summary/category-test.tsx

 import {flowResult} from '../sample-flow';
 import {I18nProvider} from '../../src/i18n/i18n';
 import {FlowResultContext} from '../../src/util';

 let wrapper: FunctionComponent;

 beforeEach(() => {
+  // Include sample flowResult for locale in I18nProvider.


Drive by change to this test so the expectations don't change every time the sample is updated.

quite a few lines for a drive by ;)

adamraine · 2021-10-01T23:52:52Z

lighthouse-core/scripts/update-flow-fixtures.js

@@ -13,7 +13,10 @@ import {LH_ROOT} from '../../root.js';
 import UserFlow from '../fraggle-rock/user-flow.js';

 (async () => {
-  const browser = await puppeteer.launch({headless: false});
+  const browser = await puppeteer.launch({
+    executablePath: process.env.CHROME_PATH,


I was noticing the FPS was rendered incorrectly on the default puppeteer chrome. We could also update the puppeteer version in this PR. Not sure it started showing up now though.

adamraine · 2021-10-01T23:53:46Z

types/config.d.ts

@@ -134,6 +134,7 @@ declare module Config {
    auditRefs: AuditRefJson[];
    description?: string | IcuMessage;
    manualDescription?: string | IcuMessage;
+    supportedModes?:  Gatherer.GatherMode[];


Required in v9?

meh, leaving as the default to always run seems fine even in v9, but I'll leave it up to you

patrickhulce · 2021-10-04T14:58:55Z

flow-report/test/summary/category-test.tsx

 import {flowResult} from '../sample-flow';
 import {I18nProvider} from '../../src/i18n/i18n';
 import {FlowResultContext} from '../../src/util';

 let wrapper: FunctionComponent;

 beforeEach(() => {
+  // Include sample flowResult for locale in I18nProvider.


quite a few lines for a drive by ;)

patrickhulce · 2021-10-04T15:02:37Z

types/config.d.ts

@@ -134,6 +134,7 @@ declare module Config {
    auditRefs: AuditRefJson[];
    description?: string | IcuMessage;
    manualDescription?: string | IcuMessage;
+    supportedModes?:  Gatherer.GatherMode[];


meh, leaving as the default to always run seems fine even in v9, but I'll leave it up to you

patrickhulce · 2021-10-04T15:03:25Z

sample JSON needs an update

brendankenny · 2021-10-04T16:00:52Z

Thinking out loud but not sure if there's something worthwhile here:

Something about the fact that this is largely about filling in the filtering gaps created because an audit can be in more than one category, and that there's redundancy with onlyCategories, makes this feel like a config structural problem that an additional option will add complexity to, rather than simplify.

I understand sharing the default config between the classic and new navigation modes, but is there a reason we haven't made a default-timespan-config.js and default-snapshot-config.js yet? e.g. rather than implicit filtering, make config entry points, either with extends: 'lighthouse:default'/onlyCategories or more ambitious reorgs (import what a config needs, require('./perf-category.js') or whatever). I don't know if that really fixes the structural issue since it's essentially functionally equivalent, but it would avoid a new config option and it may be more in the spirit of LH supporting config files (plural :).

adamraine · 2021-10-04T17:24:05Z

I understand sharing the default config between the classic and new navigation modes, but is there a reason we haven't made a default-timespan-config.js and default-snapshot-config.js yet?

This would mean we need 6 configs (desktop/mobile and 3 gather modes for each). We would probably have to repeat some config implementation somewhere.

Maybe we could have used separate configs for (#13013), but I think the problem is that we don't want simulated throttling to even be possible with timespan modes. I'm not sure we even want PWA to be available at all in snapshot or timespan through onlyCategories.

Maybe we could hardcode an override for PWA like we did with throttling settings. With this however, a new audit in SEO that works in all three modes would quietly add SEO to all timespan reports (unless we hardcode another exception).

I like the ability to explicitly control which categories appear for each mode, and I don't think a supportedModes filter is any more complicated than having 6 default configs.

patrickhulce · 2021-10-04T17:31:04Z

is there a reason we haven't made a default-timespan-config.js and default-snapshot-config.js yet? e.g. rather than implicit filtering, make config entry points, either with extends: 'lighthouse:default'/onlyCategories or more ambitious reorgs (import what a config needs, require('./perf-category.js') or whatever).

My primary thoughts here in general are that relying on manual configuration rather than the property-based enforcement we see in this PR complicates the story not just for us but all other config authors as well.

For example, a plugin author would need to publish 3 different versions of their plugin for each gather mode, three npm namespaces, with different configs, etc.

Relying on manual configuration instead of automatic enforcement also tends to be more prone to errors (oops we forgot to update the timespan-config rather than "oh hey we adjusted this audit, adjust the top of the same file")

brendankenny · 2021-10-04T22:16:10Z

Ha, well if y'all feel that good about this, that's fine :)

Long term, I don't think six default configs is that big of a deal. We have three pretty different modes, we have multiple methods of config composition available to us to make maintenance easier (our extends, but they're also just JS objects), and presumably the three desktop variants are just going to override the emulation settings of the mobile versions and that's it.

I suspect the more the different modes diverge in expected or required behavior the less desirable it's going to be to smooth over the differences with more configuration options and the easier it's going to be to just define them separately, but obviously this isn't the straw that breaks that particular camel's back :) An option to keep in our back pocket, though.

adamraine added 3 commits October 1, 2021 16:26

start

179f931

update flow

de84c2a

category test

01a4f48

adamraine requested a review from a team as a code owner October 1, 2021 23:51

adamraine requested review from patrickhulce and removed request for a team October 1, 2021 23:51

google-cla bot added the cla: yes label Oct 1, 2021

devtools-bot assigned patrickhulce Oct 1, 2021

devtools-bot added the waiting4reviewer label Oct 1, 2021

adamraine commented Oct 1, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

patrickhulce approved these changes Oct 4, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Oct 4, 2021

update sample

458ced3

test

6b4e384

vercel bot deployed to Preview October 4, 2021 16:11 View deployment

proto

ab494d8

vercel bot deployed to Preview October 4, 2021 18:53 View deployment

fix tests

8e8d70e

vercel bot deployed to Preview October 4, 2021 19:15 View deployment

api test

467b56c

vercel bot deployed to Preview October 4, 2021 22:13 View deployment

adamraine merged commit 45b4ec2 into master Oct 4, 2021

adamraine deleted the fr-category-modes branch October 4, 2021 22:42

adamraine mentioned this pull request Apr 26, 2022

Add supported modes to category googleads/publisher-ads-lighthouse-plugin#339

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(fr): add `supportedModes` filter to categories #13161

core(fr): add `supportedModes` filter to categories #13161

adamraine commented Oct 1, 2021

adamraine Oct 1, 2021

patrickhulce Oct 4, 2021

adamraine Oct 1, 2021

adamraine Oct 1, 2021

patrickhulce Oct 4, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

patrickhulce Oct 4, 2021

patrickhulce Oct 4, 2021

patrickhulce commented Oct 4, 2021

brendankenny commented Oct 4, 2021

adamraine commented Oct 4, 2021

patrickhulce commented Oct 4, 2021

brendankenny commented Oct 4, 2021

core(fr): add supportedModes filter to categories #13161

core(fr): add supportedModes filter to categories #13161

Conversation

adamraine commented Oct 1, 2021

adamraine Oct 1, 2021

Choose a reason for hiding this comment

patrickhulce Oct 4, 2021

Choose a reason for hiding this comment

adamraine Oct 1, 2021

Choose a reason for hiding this comment

adamraine Oct 1, 2021

Choose a reason for hiding this comment

patrickhulce Oct 4, 2021

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

patrickhulce Oct 4, 2021

Choose a reason for hiding this comment

patrickhulce Oct 4, 2021

Choose a reason for hiding this comment

patrickhulce commented Oct 4, 2021

brendankenny commented Oct 4, 2021

adamraine commented Oct 4, 2021

patrickhulce commented Oct 4, 2021

brendankenny commented Oct 4, 2021

core(fr): add `supportedModes` filter to categories #13161

core(fr): add `supportedModes` filter to categories #13161