Sometimes err.url is undefined. so It must has protect code. #893
Conversation
|
Good catch. It's nice to have URLs that expose issues. Can you also add this for the other two audits that recently landed:
and add a test that's missing |
| @@ -57,7 +57,7 @@ class NoDateNowAudit extends Audit { | |||
| const pageHost = url.parse(artifacts.URL.finalUrl).host; | |||
| // Filter usage from other hosts. | |||
| const results = artifacts.DateNowUse.usage.filter(err => { | |||
| return url.parse(err.url).host === pageHost; | |||
| return url.parse(err.url || '').host === pageHost; | |||
There was a problem hiding this comment.
For stacktraces that are missing .url, what do you think about over communicating and reporting those cases as if they were same-domain violations (since we don't know the url)? In other words:
return err.url ? url.parse(err.url).host === pageHost : true;
There was a problem hiding this comment.
@ebidel
The displayed that url was undefined like below image. What do you think about display? I think that - is more good than undefined. either (empty) is good.
->
,
There was a problem hiding this comment.
Agreed. Line/col number without an URL makes them less useful, so let's keep what you have then (err.url || ''). It looks like it finds all the violations on www.inflearn.com just fine :)
| @@ -48,4 +48,14 @@ describe('Page does not use mutation events', () => { | |||
| assert.equal(auditResult.rawValue, false); | |||
| assert.equal(auditResult.extendedInfo.value.length, 3); | |||
| }); | |||
|
|
|||
| it('fails when listener hasn`t url', () => { | |||
There was a problem hiding this comment.
fails when listener is missing a url property
| @@ -69,4 +69,14 @@ describe('Page uses passive events listeners where applicable', () => { | |||
| assert.equal(auditResult.rawValue, true); | |||
| assert.equal(auditResult.extendedInfo.value.length, 0); | |||
| }); | |||
|
|
|||
| it('fails when listener hasn`t url', () => { | |||
There was a problem hiding this comment.
fails when listener is missing a url property
| @@ -66,4 +66,21 @@ describe('Page does not use Date.now()', () => { | |||
| assert.equal(auditResult.rawValue, false); | |||
| assert.equal(auditResult.extendedInfo.value.length, 2); | |||
| }); | |||
|
|
|||
| it('fails when usage hasn`t url', () => { | |||
There was a problem hiding this comment.
maybe "only passes when has url property"
|
|
||
| assert.equal(auditResult.rawValue, false); | ||
| assert.ok(auditResult.extendedInfo.value[0].url === ''); | ||
| assert.equal(auditResult.extendedInfo.value.length, 2); |
There was a problem hiding this comment.
Can you check that extendedInfo.value has a url and is what you expect? From the first assert, it looks like extendedInfo.value will be the first to values in DateNowUse.usage?
There was a problem hiding this comment.
@ebidel
Thank you. I have misunderstood spec. 😭
…hrome#893) * Sometimes err.url is undefined. so It must has protect code. * apply reviews * apply review
When I ran
lighthouse-clifromhttps://www.inflearn.com/. I found error. Sometimes err.url is undefined. so It must has protect code.