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

report: correctly handle CLS values of 0 for calc link #10767

Merged
merged 1 commit into from
May 13, 2020

Conversation

paulirish
Copy link
Member

No description provided.

@paulirish paulirish requested a review from a team as a code owner May 13, 2020 02:48
@paulirish paulirish requested review from connorjclark and removed request for a team May 13, 2020 02:48
@connorjclark connorjclark changed the title report(calclink): correctly handle CLS values of 0 report: correctly handle CLS values of 0 for calc link May 13, 2020
@@ -117,7 +117,8 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
if (fmp) v5andv6metrics.push(fmp);

const metricPairs = v5andv6metrics.map(audit => {
const value = audit.result.numericValue ? audit.result.numericValue.toString() : 'null';
const value = typeof audit.result.numericValue !== 'undefined' ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

wdyt of just

const value = audit.result.numbericValue === undefined ? null : audit.result.numericValue

?

I don't think you need to coerce to string (params will handle that) or use typeof (just check if it is undefined)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need to use typeof

old habits die hard my friend, I don't think I'm ever going to remember to not use typeof for undefined at this point 😆

Copy link
Collaborator

@connorjclark connorjclark May 13, 2020

Choose a reason for hiding this comment

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

you only need it when you are trying to check if a value in the global scope is defined (if it's been declared)

if (window === undefined) // whoops this throws an error when there is no window

if (typeof window === 'undefined') // fine, becausejs


let obj = {};
if (obj.blah === undefined) // fine

Copy link
Collaborator

Choose a reason for hiding this comment

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

i'm sure you know this already but it took me a couple years :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Consistency is definitely a good reason, but not the only reason 😉

Once upon a time undefined was not reserved so you could do undefined = 'hello'. In the heyday of WordPress development and crazy jQuery plugins, I worked on more than one project where someone decided it was a good idea to set undefined = null so that they could do easier comparisons. If you wrote code for a client that could potentially live with such crazy other global nonsense and really wanted to test for undefinedness, you either had to do typeof o.blah === 'undefined' or wrap your junk in an IIFE that accepts an argument called undefined so at least locally it was safe.

e.g.

(function (undefined) {
  // now undefined is safe again :)
})();

AFAIK, it hasn't really been a problem for the past decade or so since it was fixed in ECMAScript 5.1, but like I said, old habits die hard 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

wow I would riot if you ppl touched undefined how did you constrain yourself.

huh I didn't realize it's technically a property defined on the global object

type window.undefined in CDT console and before you even hit enter you see the result is undefined (do some non-existent property and there is no preview)

altho i can't find it via Object.getProperties/whaterver

@connorjclark connorjclark merged commit 73ed152 into master May 13, 2020
@connorjclark connorjclark deleted the calclinkfix branch May 13, 2020 20:53
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.

5 participants