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

deps: upgrade trace_engine and drop manually written types #15810

Merged
merged 4 commits into from
Feb 15, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions core/audits/layout-shifts.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,16 @@ class LayoutShifts extends Audit {

/** @type {Item[]} */
const items = [];
const layoutShiftEvents = clusters.flatMap(c => c.events);
const layoutShiftEvents =
/** @type {import('../lib/trace-engine.js').SaneSyntheticLayoutShift[]} */(
clusters.flatMap(c => c.events)
);
const topLayoutShiftEvents = layoutShiftEvents
.sort((a, b) => b.args.data.weighted_score_delta - a.args.data.weighted_score_delta)
.slice(0, MAX_LAYOUT_SHIFTS);
for (const event of topLayoutShiftEvents) {
const biggestImpactNodeId = TraceElements.getBiggestImpactNodeForShiftEvent(
event.args.data.impacted_nodes, impactByNodeId);
event.args.data.impacted_nodes || [], impactByNodeId);
const biggestImpactElement = traceElements.find(t => t.nodeId === biggestImpactNodeId);

// Turn root causes into sub-items.
Expand Down
8 changes: 7 additions & 1 deletion core/computed/metrics/cumulative-layout-shift.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,13 @@
LayoutShifts: TraceEngine.TraceHandlers.LayoutShifts,
Screenshots: TraceEngine.TraceHandlers.Screenshots,
});
await processor.parse(events);
// eslint-disable-next-line max-len
await processor.parse(/** @type {import('@paulirish/trace_engine').Types.TraceEvents.TraceEventData[]} */ (
events
));
if (!processor.data) {
throw new Error('null trace engine result');
}

Check warning on line 168 in core/computed/metrics/cumulative-layout-shift.js

View check run for this annotation

Codecov / codecov/patch

core/computed/metrics/cumulative-layout-shift.js#L167-L168

Added lines #L167 - L168 were not covered by tests
return processor.data.LayoutShifts.sessionMaxScore;
};
const cumulativeLayoutShift = await run(allFrameShiftEvents.map(e => e.event));
Expand Down
15 changes: 12 additions & 3 deletions core/computed/trace-engine-result.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,13 @@
Samples: TraceEngine.TraceHandlers.Samples,
Screenshots: TraceEngine.TraceHandlers.Screenshots,
});
await engine.parse(traceEvents);
return engine.data;
// eslint-disable-next-line max-len
await engine.parse(/** @type {import('@paulirish/trace_engine').Types.TraceEvents.TraceEventData[]} */ (
traceEvents
));
// TODO: use TraceEngine.TraceProcessor.createWithAllHandlers above.
Copy link
Member

Choose a reason for hiding this comment

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

relevant chat thread:

image

shrugz.

return /** @type {import('@paulirish/trace_engine').Handlers.Types.TraceParseData} */(
engine.data);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will follow up with this change. Didn't want a behavior change with this patch.

}

/**
Expand Down Expand Up @@ -61,7 +66,11 @@
}
}

return TraceEngineResult.runTraceEngine(traceEvents);
const result = await TraceEngineResult.runTraceEngine(traceEvents);
if (!result) {
throw new Error('null trace engine result');
}

Check warning on line 72 in core/computed/trace-engine-result.js

View check run for this annotation

Codecov / codecov/patch

core/computed/trace-engine-result.js#L71-L72

Added lines #L71 - L72 were not covered by tests
return result;
}
}

Expand Down
31 changes: 22 additions & 9 deletions core/gather/gatherers/root-causes.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,26 +32,33 @@
// nodeIds if the DOM domain was enabled before this gatherer, invoke it to be safe.
await driver.defaultSession.sendCommand('DOM.getDocument', {depth: -1, pierce: true});

/** @type {import('@paulirish/trace_engine').RootCauses.RootCauses.RootCauseProtocolInterface} */
const protocolInterface = {
/** @param {string} url */
// eslint-disable-next-line no-unused-vars
getInitiatorForRequest(url) {
return null;
},
/** @param {number[]} backendNodeIds */
/** @param {import('@paulirish/trace_engine/generated/protocol.js').DOM.BackendNodeId[]} backendNodeIds */
async pushNodesByBackendIdsToFrontend(backendNodeIds) {
const response = await driver.defaultSession.sendCommand(
'DOM.pushNodesByBackendIdsToFrontend', {backendNodeIds});
return response.nodeIds;
const nodeIds =
/** @type {import('@paulirish/trace_engine/generated/protocol.js').DOM.NodeId[]} */(
response.nodeIds);
return nodeIds;
},
/** @param {number} nodeId */
/** @param {import('@paulirish/trace_engine/generated/protocol.js').DOM.NodeId} nodeId */
async getNode(nodeId) {
try {
const response = await driver.defaultSession.sendCommand('DOM.describeNode', {nodeId});
// This always zero, so let's fix it here.
// https://bugs.chromium.org/p/chromium/issues/detail?id=1515175
response.node.nodeId = nodeId;
return response.node;
const node =
/** @type {import('@paulirish/trace_engine/generated/protocol.js').DOM.Node} */(
response.node);
return node;
} catch (err) {
if (err.message.includes('Could not find node with given id')) {
// TODO: when injecting an iframe, the engine gets the node of that frame's document element.
Expand All @@ -64,7 +71,8 @@
// When this is fixed, remove this try/catch.
// Note: this could be buggy by giving the wrong node detail if a node id meant for a non-main frame
// happens to match one from the main frame ... which is pretty likely...
return null;
// TODO: fix trace engine type to allow returning null.
return /** @type {any} */(null);
}
throw err;
}
Expand All @@ -79,17 +87,20 @@
return [];
}
},
/** @param {number} nodeId */
/** @param {import('@paulirish/trace_engine/generated/protocol.js').DOM.NodeId} nodeId */
async getMatchedStylesForNode(nodeId) {
try {
const response = await driver.defaultSession.sendCommand(
'CSS.getMatchedStylesForNode', {nodeId});
return response;
} catch {
return [];
return {...response, getError() {}};
} catch (err) {
return /** @type {any} */({getError() {
return err.toString();
}});

Check warning on line 99 in core/gather/gatherers/root-causes.js

View check run for this annotation

Codecov / codecov/patch

core/gather/gatherers/root-causes.js#L97-L99

Added lines #L97 - L99 were not covered by tests
}
},
/** @param {string} url */
// @ts-expect-error
Copy link
Member

Choose a reason for hiding this comment

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

nit: can you add a comment explaining this @ts-expect-error

connorjclark marked this conversation as resolved.
Show resolved Hide resolved
// eslint-disable-next-line no-unused-vars
async fontFaceForSource(url) {
return null;
Expand All @@ -104,6 +115,8 @@
const layoutShiftEvents = traceEngineResult.LayoutShifts.clusters.flatMap(c => c.events);
for (const event of layoutShiftEvents) {
const r = await rootCausesEngine.layoutShifts.rootCausesForEvent(traceEngineResult, event);
if (!r) continue;

for (const cause of r.fontChanges) {
// TODO: why isn't trace engine unwrapping this promise ...
cause.fontFace = await cause.fontFace;
Expand Down
8 changes: 6 additions & 2 deletions core/gather/gatherers/trace-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,15 +117,19 @@ class TraceElements extends BaseGatherer {
static async getTopLayoutShifts(trace, traceEngineResult, rootCauses, context) {
const {impactByNodeId} = await CumulativeLayoutShift.request(trace, context);
const clusters = traceEngineResult.LayoutShifts.clusters ?? [];
const layoutShiftEvents = clusters.flatMap(c => c.events);
const layoutShiftEvents =
/** @type {import('../../lib/trace-engine.js').SaneSyntheticLayoutShift[]} */(
clusters.flatMap(c => c.events)
);

return layoutShiftEvents
.sort((a, b) => b.args.data.weighted_score_delta - a.args.data.weighted_score_delta)
.slice(0, MAX_LAYOUT_SHIFTS)
.flatMap(event => {
const nodeIds = [];
const impactedNodes = event.args.data.impacted_nodes || [];
const biggestImpactedNodeId =
this.getBiggestImpactNodeForShiftEvent(event.args.data.impacted_nodes, impactByNodeId);
this.getBiggestImpactNodeForShiftEvent(impactedNodes, impactByNodeId);
if (biggestImpactedNodeId !== undefined) {
nodeIds.push(biggestImpactedNodeId);
}
Expand Down
7 changes: 3 additions & 4 deletions core/lib/trace-engine.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
// @ts-expect-error missing types
import * as TraceEngine from '@paulirish/trace_engine';

import {polyfillDOMRect} from './polyfill-dom-rect.js';

/** @typedef {import('@paulirish/trace_engine').Types.TraceEvents.SyntheticLayoutShift} SyntheticLayoutShift */
/** @typedef {SyntheticLayoutShift & {args: {data: NonNullable<SyntheticLayoutShift['args']['data']>}}} SaneSyntheticLayoutShift */

polyfillDOMRect();

/** @type {import('../../types/trace-engine.js').TraceProcessor & typeof import('../../types/trace-engine.js').TraceProcessor} */
const TraceProcessor = TraceEngine.Processor.TraceProcessor;
/** @type {import('../../types/trace-engine.js').TraceHandlers} */
const TraceHandlers = TraceEngine.Handlers.ModelHandlers;
/** @type {import('../../types/trace-engine.js').RootCauses & typeof import('../../types/trace-engine.js').RootCauses} */
const RootCauses = TraceEngine.RootCauses.RootCauses.RootCauses;

export {
Expand Down
8 changes: 4 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -180,13 +180,13 @@
"webtreemap-cdt": "^3.2.1"
},
"dependencies": {
"@paulirish/trace_engine": "^0.0.7",
"@paulirish/trace_engine": "^0.0.12",
"@sentry/node": "^6.17.4",
"axe-core": "^4.8.4",
"chrome-launcher": "^1.1.0",
"configstore": "^5.0.1",
"csp_evaluator": "1.1.1",
"devtools-protocol": "0.0.1211954",
"devtools-protocol": "0.0.1232444",
"enquirer": "^2.3.6",
"http-link-header": "^1.1.1",
"intl-messageformat": "^10.5.3",
Expand All @@ -211,8 +211,8 @@
"yargs-parser": "^21.0.0"
},
"resolutions": {
"puppeteer/**/devtools-protocol": "0.0.1211954",
"puppeteer-core/**/devtools-protocol": "0.0.1211954"
"puppeteer/**/devtools-protocol": "0.0.1232444",
"puppeteer-core/**/devtools-protocol": "0.0.1232444"
},
"repository": "GoogleChrome/lighthouse",
"keywords": [
Expand Down
7 changes: 4 additions & 3 deletions types/artifacts.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
*/

import {Protocol as Crdp} from 'devtools-protocol/types/protocol.js';
import * as TraceEngine from '@paulirish/trace_engine';
import {LayoutShiftRootCausesData} from '@paulirish/trace_engine/models/trace/root-causes/LayoutShift.js';

import {parseManifest} from '../core/lib/manifest-parser.js';
import {Simulator} from '../core/lib/dependency-graph/simulator/simulator.js';
Expand All @@ -23,7 +25,6 @@ import LHResult from './lhr/lhr.js'
import Protocol from './protocol.js';
import Util from './utility-types.js';
import Audit from './audit.js';
import {TraceProcessor, LayoutShiftRootCauses} from './trace-engine.js';

export type Artifacts = BaseArtifacts & GathererArtifacts;

Expand Down Expand Up @@ -569,10 +570,10 @@ declare module Artifacts {
type?: string;
}

type TraceEngineResult = TraceProcessor['data'];
type TraceEngineResult = TraceEngine.Handlers.Types.TraceParseData;

interface TraceEngineRootCauses {
layoutShifts: Record<number, LayoutShiftRootCauses>;
layoutShifts: Record<number, LayoutShiftRootCausesData>;
}

interface ViewportDimensions {
Expand Down
Loading
Loading