core: remove fonts gatherer #6166
Conversation
patrickhulce
changed the title
|
@patrickhulce I don't hate, I kill 🗡 😄 (you're lucky the trip is so far) it's actually something I suggested as well #5676. I'll run it against some sites too, just make sure the results are ok. |
doh! you totally did my bad! |
|
I wouldn't have done such a great job though 😊 |
| // remove any optional quotes before/after | ||
| .map(s => { | ||
| const firstChar = s.charAt(0); | ||
| if (firstChar === s.charAt(s.length - 1) && (firstChar === '"' || firstChar === '\'')) { |
There was a problem hiding this comment.
better like this than in the regex?
There was a problem hiding this comment.
I thought this might be more readable than the backreference regex, but I guess not :) switched!
There was a problem hiding this comment.
I thought this might be more readable than the backreference regex
haha, it might be! I was just asking
| return s; | ||
| }); | ||
|
|
||
| const absoluteURLs = relativeURLs.map(url => new URL(url, artifacts.URL.finalUrl)); |
There was a problem hiding this comment.
how do we want to deal with exceptions here? e.g. invalid font URL probably shouldn't throw the whole audit
There was a problem hiding this comment.
yeah good point, I'll add a try/catch with beacon
|
|
||
| const result = await Audit.audit(getArtifacts()); | ||
| assert.strictEqual(result.rawValue, true); | ||
| assert.deepEqual(result.details.items, []); | ||
| }); |
There was a problem hiding this comment.
how do you feel about populating this with some of the @font-face blocks we can find out on real sites? (like the sites ward mentioned in #6166 (comment)). We'd have better coverage for the regexes, plus we'd have a ready-made place to put future regression tests if we ever find we're missing weird things people are doing.
If all the examples we can find are pretty similar, could use them but vary new lines, font-display value, whitespace, and throw in some invalid URLs. Is there anything interesting for fonts not in the network records or not in stylesheets? (probably not for this audit)
There was a problem hiding this comment.
great idea! will do
| it('fails when not all fonts have a correct font-display rule', async () => { | ||
| stylesheet.content = ` | ||
| @font-face { | ||
| src: url("./font-a.woff"); |
There was a problem hiding this comment.
maybe call out the quote differences here? Either a comment (or comments for each in the css string) or even a test only testing quoting the url? (though it would be identical to this test :)
It's kind of not obvious that this is covering those possibilities if you haven't just read the audit, and I could see them being accidentally dropped in future edits to this file without knowing they were important.
There was a problem hiding this comment.
yeah good call, done
| assert.deepEqual(result.details.items, []); | ||
| }); | ||
|
|
||
| it('should handle real-world font-face declarations', async () => { |
| const absoluteURL = new URL(relativeURL, artifacts.URL.finalUrl); | ||
| passingURLs.add(absoluteURL.href); | ||
| } catch (err) { | ||
| Sentry.captureException(err, {tags: {audit: this.meta.id}}); |
There was a problem hiding this comment.
We should probably only capture once per audit run. Kind of annoying to implement, but some sites end up with a ridiculous number of broken URLs (or whatever we're capturing in a loop). See #5994 (which the only one I had actually checked off was the gatherer this is deleting :)...or disagree with the conclusion there :)
There was a problem hiding this comment.
I have a counter-proposal that I will put up in a different PR :)
don't hate me @wardpeet 😬
Summary
I thought I'd try another approach for the font-display audit when I noticed the
Fontsgatherer taking ~25% of a perf-only CNN run these days. We're already collecting the stylesheet content for everything forunminified/unused-css, so can a simple regex lookup get the job done?It passes smoke tests, but I'd want to try this on a few more higher profile sites too.
(Also noticed we need to update this to use lantern timings, but one thing at a time 😄)