core(meta): refactor all meta audits to single artifact #7025
Conversation
| @@ -199,7 +199,7 @@ class FontSize extends Audit { | |||
| title: str_(UIStrings.title), | |||
| failureTitle: str_(UIStrings.failureTitle), | |||
| description: str_(UIStrings.description), | |||
| requiredArtifacts: ['FontSize', 'URL', 'Viewport'], | |||
| requiredArtifacts: ['FontSize', 'URL', 'MetaElements'], | |||
There was a problem hiding this comment.
no changes to functionality needed here because it just calls out to the viewport audit
There was a problem hiding this comment.
maybe it's unavoidable, but are we setting ourselves up for ballooning the size of the artifacts? People put crazy things in meta tags sometimes.
We could have a list of allowed meta names and add the ones we need as we go. On the other hand, that would prevent plugins from being able to just use the meta tags they're interested in without getting changes landed in core.
types/artifacts.d.ts
Outdated
| MetaDescription: string|null; | ||
| /** The value of the <meta name="robots">'s content attribute, or null. */ | ||
| MetaRobots: string|null; | ||
| /** The value of the <meta>elements in the head. */ |
| @@ -12,7 +12,7 @@ | |||
| <!-- FAIL(font-size): missing viewport --> | |||
| <meta name="viewport" content="invalid-content=should_have_looked_it_up"> | |||
| <!-- no <meta name="description" content=""> --> | |||
| <meta name="robots" content="nofollow, NOINDEX, all"> | |||
| <meta name="RoBots" content="nofollow, NOINDEX, all"> | |||
There was a problem hiding this comment.
document the reason for this?
| @@ -43,13 +43,13 @@ class ThemedOmnibox extends MultiCheckAudit { | |||
| } | |||
|
|
|||
| /** | |||
| * @param {LH.Artifacts['ThemeColor']} themeColorMeta | |||
| * @param {LH.Artifacts['MetaElements'][0]|undefined} themeColorMeta | |||
There was a problem hiding this comment.
should we make an alias of LH.Artifacts.MetaElement since we use this so often?
There was a problem hiding this comment.
this is the only usage of it, but sure :)
There was a problem hiding this comment.
this is the only usage of it, but sure
ha, whoops
| it('fails when viewport is not set', () => { | ||
| const artifacts = { | ||
| URL, | ||
| Viewport: null, | ||
| MetaElements: makeMetaElements(null), |
There was a problem hiding this comment.
Is the conditional in makeMetaElements just for this case? null could just be undefined or this one could just be [] an no conditional needed :)
| @@ -78,7 +80,7 @@ describe('SEO: Is page crawlable audit', () => { | |||
| const artifacts = { | |||
| devtoolsLogs: {[IsCrawlableAudit.DEFAULT_PASS]: devtoolsLog}, | |||
| URL: {finalUrl}, | |||
| MetaRobots: null, | |||
| MetaElements: makeMetaElements(null), | |||
There was a problem hiding this comment.
switch to no argument or just use [] and no function call?
| @@ -94,7 +94,7 @@ describe('Config', () => { | |||
| const configJson = { | |||
| passes: [{ | |||
| passName: unlikelyPassName, | |||
| gatherers: ['viewport'], | |||
| gatherers: ['meta-elements'], | |||
There was a problem hiding this comment.
I apologize for the number of times I've used viewport as a dummy gatherer name :)
FWIW, I'm not really worried about this. Of all the artifact size concerns, we know these artifacts have to be constrained to roughly the size of the page. trace/devtoolslog/screenshots are always going to dwarf this.
This limitation would kinda defeat the purpose of having generic gatherers IMO, would it not? Seems like if we just limited to what we needed then it's no different from the position we were in before functionality-wise. |
|
@brendankenny LGTY? |
Summary
Replace all our scattered meta gatherers with a single
MetaElementsartifact.Related Issues/PRs
#6747