-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core(fr): convert image-elements gatherer #12474
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This restructure is definitely an improvement, IMO, so this draft LGTM to polish if this is the first step we'd like to take here. We will eventually have to be much more aggressive with restructuring this gatherer to make it work well for other modes, but understood if you'd prefer to do it in small chunks 👍
I'll also throw out #12077 in case there was anything you were looking at restructuring that aligns with imminent breaking change work we'd like to tackle.
|
||
it('fetch size information for image with picture', () => { | ||
const elements = [ | ||
{node: {}, src: 'https://example.com/a.png', isPicture: true, isCss: true, srcset: 'src'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW isPicture
and isCss
should never be true at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure the function this is testing is bugged. networkRecord
will always be true because {}
is truthy.
Co-authored-by: Patrick Hulce <[email protected]>
Co-authored-by: Patrick Hulce <[email protected]>
Possible second bug: lighthouse/lighthouse-core/gather/gatherers/image-elements.js Lines 235 to 239 in 8769ff8
Shouldn't we return after the cache hit? |
heh, whoops, yes indeed @adamraine . at least that's a new one |
oh wait now that you mention it both of those little bugs come from that PR, both quite new then :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also LGTM, nice job!
* @param {LH.Artifacts.ImageElement} element | ||
*/ | ||
async fetchElementWithSizeInformation(driver, element) { | ||
const url = element.src; | ||
if (this._naturalSizeCache.has(url)) { | ||
Object.assign(element, this._naturalSizeCache.get(url)); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work!
Opening as a draft to get some initial thoughts on the new test file which is not exhaustive yet. I restructuredImageElements
a bit to make it easier to test.Ref #11313