-
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(tsc): fix ImageUsage artifact type and gather bug #5113
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ const Driver = require('../driver.js'); // eslint-disable-line no-unused-vars | |
|
||
/* global window, getElementsInDocument, Image */ | ||
|
||
/** @typedef {MakeOptional<LH.Artifacts.SingleImageUsage, 'networkRecord'>} ImageSizeInfo */ | ||
/** @typedef {Omit<LH.Artifacts.SingleImageUsage, 'networkRecord'>} ImageSizeInfo */ | ||
|
||
/** @return {Array<ImageSizeInfo>} */ | ||
/* istanbul ignore next */ | ||
|
@@ -145,7 +145,7 @@ class ImageUsage extends Gatherer { | |
* @param {LH.Gatherer.LoadData} loadData | ||
* @return {Promise<LH.Artifacts['ImageUsage']>} | ||
*/ | ||
afterPass(passContext, loadData) { | ||
async afterPass(passContext, loadData) { | ||
const driver = passContext.driver; | ||
const indexedNetworkRecords = loadData.networkRecords.reduce((map, record) => { | ||
if (/^image/.test(record._mimeType) && record.finished) { | ||
|
@@ -167,27 +167,24 @@ class ImageUsage extends Gatherer { | |
return (${collectImageElementInfo.toString()})(); | ||
})()`; | ||
|
||
/** @type {Promise<Array<ImageSizeInfo>>} */ | ||
const evaluatePromise = driver.evaluateAsync(expression); | ||
return evaluatePromise.then(elements => { | ||
return elements.reduce((promise, element) => { | ||
return promise.then(collector => { | ||
// Images within `picture` behave strangely and natural size information isn't accurate, | ||
// CSS images have no natural size information at all. | ||
// Try to get the actual size if we can. | ||
const elementPromise = (element.isPicture || element.isCss) && element.networkRecord ? | ||
this.fetchElementWithSizeInformation(driver, element) : | ||
Promise.resolve(element); | ||
|
||
return elementPromise.then(element => { | ||
// link up the image with its network record | ||
element.networkRecord = indexedNetworkRecords[element.src]; | ||
collector.push(/** @type {LH.Artifacts.SingleImageUsage} */ (element)); | ||
return collector; | ||
}); | ||
}); | ||
}, Promise.resolve(/** @type {LH.Artifacts['ImageUsage']} */ ([]))); | ||
}); | ||
/** @type {Array<ImageSizeInfo>} */ | ||
const elements = await driver.evaluateAsync(expression); | ||
|
||
/** @type {LH.Artifacts['ImageUsage']} */ | ||
const imageUsage = []; | ||
for (let element of elements) { | ||
// Images within `picture` behave strangely and natural size information isn't accurate, | ||
// CSS images have no natural size information at all. | ||
// Try to get the actual size if we can. | ||
if (element.isPicture || element.isCss) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we maintain the original concern and only fetch when we can find the network record? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
...actually this conversation clearly shows that it should have been an optional property... |
||
element = await this.fetchElementWithSizeInformation(driver, element); | ||
} | ||
|
||
// link up the image with its network record | ||
imageUsage.push(Object.assign({networkRecord: indexedNetworkRecords[element.src]}, element)); | ||
} | ||
|
||
return imageUsage; | ||
} | ||
} | ||
|
||
|
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 was the bug.
element.networkRecord
was never defined at this point