Skip to content
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

core: add hidden audits for each insight #16312

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Jan 22, 2025

  • trace engine now exports all translated strings used by each insight
  • insight audits mostly just need to use the adaptInsightToAuditProduct helper, which is given the insight model and the caller returns a LH.Audit.Product
  • adds yarn generate-insight-audits script, to quickly make and configure new audits (just run when new insights exists, no flags). Calling in CI so we don't forget to add a new insight when upgrading trace engine
  • all insight audits are currently hidden until UI is implemented
  • only dom-size-insight, viewport-insight, and render-blocking-insight have been implemented. The other 11 are blank implementations
  • adds to TraceElements a new group trace-engine, which grabs all the node ids referenced in insights and collects their node information

@connorjclark connorjclark changed the title wip add viewport-insight core: add hidden audits for each insight Feb 4, 2025
@connorjclark connorjclark marked this pull request as ready for review February 4, 2025 22:48
@connorjclark connorjclark requested a review from a team as a code owner February 4, 2025 22:48
@connorjclark connorjclark requested review from adamraine and removed request for a team February 4, 2025 22:48
@@ -44,7 +44,7 @@ function buildStandaloneReport() {
outfile: 'dist/report/standalone.js',
format: 'iife',
bundle: true,
minify: true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drive by. this is very useful. surprised we didn't already do this.

const navigationId = traceEngineResult.data.Meta.mainFrameNavigations[0].args.data?.navigationId;
if (!navigationId) {
throw new Error(`Lantern metrics could not be calculated due to missing navigation id`);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shifts in types with trace engine...

*/
static async audit(artifacts, context) {
return adaptInsightToAuditProduct(artifacts, context, 'DOMSize', (insight) => {
if (!insight.maxDOMStats?.args.data.maxChildren || !insight.maxDOMStats?.args.data.maxDepth) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically we will still have a valid totalElements value even if these are missing. In practice though, this will only happen if the document element is removed so it's an edge case that really doesn't matter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants