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.

Sign up for GitHub

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

Jump to bottom

report: handle non-numeric numericValues in calc link #10880

Merged
merged 5 commits into from
Apr 25, 2022
Merged

report: handle non-numeric numericValues in calc link #10880

merged 5 commits into from
Apr 25, 2022

Conversation

paulirish
Copy link
Member

Whelp.. in LR, our undefined's become null and we end up throwing.
image

definitely shoulda gone with this version of the typeof check to begin with. :/

this is a 6.0.1 fix. i imagine we've already hit it in PSI a couple times

@paulirish paulirish requested a review from a team as a code owner May 29, 2020 21:27
@paulirish paulirish requested review from Beytoven and removed request for a team May 29, 2020 21:27
@brendankenny
Copy link
Member

our undefined's become null and we end up throwing

OK, something is wrong, then. This really shouldn't happen and we should fix.

DoubleValue will work well for us. 0 comes through, and then any audit with it not set will come back with no numericValue in JSON, which works just fine for us.

#8385 (comment)

@brendankenny
Copy link
Member

OK, something is wrong, then. This really shouldn't happen and we should fix.

Yeah, something did break. The CI version of the proto test isn't seeing this problem (both missing numericValue and details are dropped as expected instead of made null). See this purposefully failed sample_v2 roundtrip for what it looks like there: https://github.com/GoogleChrome/lighthouse/runs/722125320#step:14:596 (test branch)

@exterkamp is is possible the PSI proto handling is diverging from the library we're using on the null/default value front?

@paulirish
Copy link
Member Author

paulirish commented Jun 2, 2020 •
edited
Loading

  • @paulirish to make a repro.
    • try out in both LR demo app and PSI (because tex..)
  • @exterkamp to investigate the default value of DoubleValue and why numericValue is giving us a null

@paulirish paulirish assigned paulirish and unassigned Beytoven Jul 6, 2020
@connorjclark
Copy link
Collaborator

Either way, this change is a net positive, can we merge?

@patrickhulce patrickhulce changed the title report(calclink): handle error'd audit numericValues in LR report: handle non-numeric audit numericValues Oct 12, 2020
@connorjclark connorjclark changed the title report: handle non-numeric audit numericValues report: handle non-numeric numericValues in calc link Oct 12, 2020
paulirish
paulirish commented Apr 25, 2022
View reviewed changes
lighthouse-core/report/html/renderer/performance-category-renderer.js Outdated
@@ -117,7 +117,7 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
if (fmp) v5andv6metrics.push(fmp);

const metricPairs = v5andv6metrics.map(audit => {
const value = typeof audit.result.numericValue !== 'undefined' ?
const value = typeof audit.result.numericValue === 'number' ?
Copy link
Member Author

Choose a reason for hiding this comment

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

this fix went in with #10954

@devtools-bot devtools-bot merged commit d486855 into master Apr 25, 2022
@devtools-bot devtools-bot deleted the nullcalclink branch April 25, 2022 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrickhulce patrickhulce patrickhulce left review comments

@brendankenny brendankenny brendankenny left review comments

@Beytoven Beytoven Beytoven approved these changes

@connorjclark connorjclark connorjclark approved these changes

Assignees

@paulirish paulirish

Labels
cla: yes waiting4reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@paulirish @brendankenny @connorjclark @patrickhulce @Beytoven @googlebot @devtools-bot