core(canonical): move canonical audit to LinkElements #7080
Conversation
| @@ -75,102 +39,102 @@ class Canonical extends Audit { | |||
| * @param {LH.Audit.Context} context | |||
| * @return {Promise<LH.Audit.Product>} | |||
| */ | |||
| static audit(artifacts, context) { | |||
| static async audit(artifacts, context) { | |||
There was a problem hiding this comment.
perhaps you could split up this function in a few sub functions to make it more readable? A lot of things are going on it seems.
There was a problem hiding this comment.
yeah good call @wardpeet!
| */ | ||
| function normalizeUrlOrNull(url, finalUrl) { | ||
| try { | ||
| return new URL(url, finalUrl).href; |
There was a problem hiding this comment.
is this something that we should be calling to anyone's attention? How do these arise in practice? (just typos or something else?)
And why not do this in the getLinkElementsInDOM case?
There was a problem hiding this comment.
accessing the .href of a link from dom already does the normalization for us
we do call attention to these cases when it matters in the audits themselves, i.e. there was an hrefRaw but no href indicates there was an error in parsing the URL
There was a problem hiding this comment.
we do call attention to these cases when it matters in the audits themselves, i.e. there was an hrefRaw but no href indicates there was an error in parsing the URL
I was hoping we could do more to make it less tricky to get right for any audits using the artifact (e.g. boolean props for the different invalid permutations rather than making sure to check both href and hrefRaw), but I don't have a good suggestion, so let's wait for if it's ever an issue :)
| } | ||
| } | ||
|
|
||
| return {uniqueCanonicalURLs, hreflangURLs, invalidCanonicalLink, relativeCanonicallink}; |
There was a problem hiding this comment.
why are some of these plural and some are singular? :)
There was a problem hiding this comment.
short: parity and I'm only willing to do so much cleanup for an unrelated goal :P
There was a problem hiding this comment.
short: parity and I'm only willing to do so much cleanup for an unrelated goal :P
haha, very fair :)
| */ | ||
| function normalizeUrlOrNull(url, finalUrl) { | ||
| try { | ||
| return new URL(url, finalUrl).href; |
There was a problem hiding this comment.
we do call attention to these cases when it matters in the audits themselves, i.e. there was an hrefRaw but no href indicates there was an error in parsing the URL
I was hoping we could do more to make it less tricky to get right for any audits using the artifact (e.g. boolean props for the different invalid permutations rather than making sure to check both href and hrefRaw), but I don't have a good suggestion, so let's wait for if it's ever an issue :)
| crossOrigin: 'anonymous'|'use-credentials'|null | ||
| /** Where the link was found, either in the DOM or in the headers of the main document */ |
There was a problem hiding this comment.
worth linking to https://html.spec.whatwg.org/multipage/links.html#body-ok (chart of which types of link elements are allowed in the body)?
(I find the ridiculously esoteric "body-ok" to be hilarious)
There was a problem hiding this comment.
Actually, should we filter (in a future PR) based on body-ok and not have them appear in the artifact if they aren't ok?
Only issue is that this chart isn't exhaustive and other specs are allowed to define body-okness for their own keywords.
There was a problem hiding this comment.
Ooooooooh that's what you were talking about. I assumed it was a typo lol 😆 Yeah I'll add the link.
I think we should probably leave them in the artifact so some point in the future we could have an audit that flags trying to use non body-ok, but flagging them
Summary
More artifact improvements. This moves us off the
Canonicalartifact and one step closer toLinkElementsbeing the canonical (😆) artifact for links.I was going to try to do it in one swoop, but updating just this audit was already getting to be a very large change.
Related Issues/PRs
#6747