-
Notifications
You must be signed in to change notification settings - Fork 9.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
new_audit: add responsiveness metric for timespans #13917
Conversation
static async audit(artifacts, context) { | ||
const gatherContext = artifacts.GatherContext; | ||
const {settings} = context; | ||
// TODO: only timespan currently supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a lot of these TODOs are items in #13916
*/ | ||
static get meta() { | ||
return { | ||
id: 'interaction-to-next-paint', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulirish do you have feelings about what we should call the audit if we want the name to stand the test of time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on the crux side we're using an experimental_
prefix in both BQ and API. So while we've never done that here, that'd be consistent and fairly reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
experimental-interaction-to-next-paint
it is :)
]; | ||
|
||
/** @type {Record<string, LH.Config.AuditRefJson[]>} */ | ||
const frCategoryAuditRefExtensions = { | ||
'performance': [ | ||
{id: 'uses-responsive-images-snapshot', weight: 0}, | ||
{id: 'interaction-to-next-paint', weight: 30, group: 'metrics', acronym: 'INP'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the same weight as TBT so it's 50/50, but maybe we don't care because timespan reports don't show the weighted score?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is going to be a diagnostic in navigation mode, it might be helpful to make the score 0, but someone reading the config wouldn't know that it's still important for timespans.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is going to be a diagnostic in navigation mode, it might be helpful to make the score 0, but someone reading the config wouldn't know that it's still important for timespans.
yeah, makes sense, done. The scoring is confusing at the JSON level in timespans, but scoring is confusing in timespans, so that's nothing new :)
lighthouse-core/test/audits/metrics/interaction-to-next-paint-test.js
Outdated
Show resolved
Hide resolved
// TODO: only timespan currently supported. | ||
if (gatherContext.gatherMode !== 'timespan' || | ||
// TODO: simulated timespan isn't supported by lantern. | ||
settings.throttlingMethod === 'simulate') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could add a supportedModes: ['timespan']
to the meta for now.
Also throttlingMethod
should always be overriden in timespan mode:
lighthouse/lighthouse-core/fraggle-rock/config/config.js
Lines 170 to 176 in 6ba8dc8
function overrideSettingsForGatherMode(settings, context) { | |
if (context.gatherMode === 'timespan') { | |
if (settings.throttlingMethod === 'simulate') { | |
settings.throttlingMethod = 'devtools'; | |
} | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added supportedModes: ['timespan']
, but kept the explicit simulate
check. I'd like to keep the throttlingMethod
support independent of the gatherMode
until we figure out how good the responsiveness simulation can be
e69e530
to
dad51d4
Compare
hmm:
ok, so update: - expect(notApplicableAudits.length).toMatchInlineSnapshot(`18`);
+ expect(notApplicableAudits.length).toMatchInlineSnapshot(`19`); and now
@adamraine any thoughts? |
|
||
const ExperimentalInteractionToNextPaint = | ||
require('../../../audits/metrics/experimental-interaction-to-next-paint.js'); | ||
const interactionTrace = require('../../fixtures/traces/timespan-responsiveness.trace.json'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add an m10X to this filename.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add an m10X to this filename.
done
I'll investigate |
015bdd6
to
00ec43f
Compare
00ec43f
to
4d617c0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, excited to see this in action
Co-authored-by: Adam Raine <6752989+adamraine@users.noreply.github.com>
Part of #13916
Adds an experimental version of https://web.dev/responsiveness/ for timespans.
Also adds a new timespan test trace from a page with interaction events coming from both the main frame and a child iframe.