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(trace-elements): remove element score field #15677

Merged
merged 2 commits into from
Dec 8, 2023
Merged

Conversation

adamraine
Copy link
Member

@adamraine adamraine commented Dec 8, 2023

Follow up to #15593

With impactByNodeId on the CLS computed artifact, we don't need the trace elements score field anymore.

@adamraine adamraine requested a review from a team as a code owner December 8, 2023 18:06
@adamraine adamraine requested review from connorjclark and removed request for a team December 8, 2023 18:06
@adamraine adamraine changed the title core(layout-shift-elements): aggregate all remaining elements core(trace-elements): remove score Dec 8, 2023
@adamraine adamraine changed the title core(trace-elements): remove score core(trace-elements): remove element score field Dec 8, 2023
Base automatically changed from layout-shift-event-aggregate to main December 8, 2023 21:59
{nodeId: 13, score: 0.1},
{nodeId: 14, score: 0.1},
{nodeId: 15, score: 0.1},
{nodeId: 3}, // score: 1.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

did you mean to keep?

Copy link
Member Author

@adamraine adamraine Dec 8, 2023

Choose a reason for hiding this comment

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

No, score isn't on the trace elements anymore. This was just a way for the test to justify the order given.

@@ -43,12 +43,12 @@ class LayoutShiftElements extends Audit {
const {cumulativeLayoutShift: clsSavings, impactByNodeId} =
await CumulativeLayoutShiftComputed.request(artifacts.traces[Audit.DEFAULT_PASS], context);

/** @type {Array<{node: LH.Audit.Details.ItemValue, score?: number}>} */
/** @type {Array<{node: LH.Audit.Details.ItemValue, score: number}>} */
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this refactor mean the impactByNodeId map should always have a value, and so || 0 was done to placate typescript? or are we setting 0 where before it would be undefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's kinda both. Every LS element that appears in TraceElements should also appear in impactByNodeId. I'm not aware of any situation where the || 0 would get triggered, but if it does the previous behavior would have been to leave as undefined.

@adamraine adamraine merged commit a60c73d into main Dec 8, 2023
29 checks passed
@adamraine adamraine deleted the trace-els-rm-score branch December 8, 2023 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants