core(legacy-javascript): make opportunity #10881
Conversation
|
I'll take a closer look at this tomorrow @connorjclark I promise :) |
lighthouse-core/scripts/legacy-javascript/create-polyfill-size-estimation.js
Show resolved
Hide resolved
lighthouse-core/scripts/legacy-javascript/create-polyfill-size-estimation.js
Show resolved
Hide resolved
| estimatedWastedBytesFromPolyfills += [...modulesSeen].reduce((acc, module) => { | ||
| return acc + graph.moduleSizes[module]; | ||
| }, 0); | ||
| // Not needed? |
There was a problem hiding this comment.
yeah maybe just add a test for this rather than an extra min?
| {key: null, itemType: 'code', subRows: {key: 'signals'}, text: ''}, | ||
| {key: 'url', valueType: 'url', subRows: {key: 'locations', valueType: 'source-location'}, label: str_(i18n.UIStrings.columnURL)}, | ||
| {key: null, valueType: 'code', subRows: {key: 'signals'}, label: ''}, | ||
| {key: 'wastedBytes', valueType: 'bytes', granularity: 0.05, label: str_(i18n.UIStrings.columnWastedBytes)}, |
There was a problem hiding this comment.
love the additional actionability of this 👍
…-estimation.js Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
| * @param {Array<LH.Artifacts.NetworkRequest>} networkRecords | ||
| * @param {LH.Crdp.Network.ResourceType=} resourceType | ||
| */ | ||
| static async estimateTransferRatio(transferRatioByUrl, url, artifacts, |
There was a problem hiding this comment.
ah nice catch! +1 to sharing this piece between the two
I will say though that this has an unusual API compared to its surroundings to live as a utility like this (cache state that you keep outside of this method but this staticish method also updates it?). also seems overly specific to scripts and not stylesheet content, HTML, etc?
If we want to land this quickly I think I'd prefer to just leave it on duplicated-javascript and have legacy-javascript.js pull it in so other audits don't get the idea that it's generic and ready to roll. a name like estimateTransferRatioForScript might help too 👍
how do you feel about that?
| * @param {Array<LH.Artifacts.NetworkRequest>} networkRecords | ||
| * @param {LH.Crdp.Network.ResourceType=} resourceType | ||
| */ | ||
| static async estimateTransferRatio(transferRatioByUrl, url, artifacts, |
There was a problem hiding this comment.
also kinda weird that duplicated-javascript only really cares about the transfer size ultimately, so maybe a future refactor could augment that helper with some script ability instead of this new one?
| const script = artifacts.ScriptElements.find(script => script.src === url); | ||
|
|
||
| if (!script || script.content === null) { | ||
| // Can't find content, so just use 1. |
There was a problem hiding this comment.
now that I'm looking at it with fresh eyes this feels wrong, should probably do something like borrowing ratio from the main resource or something that estimateTransferSize does
| estimatedWastedBytesFromTransforms += pattern.estimator(result); | ||
| } | ||
|
|
||
| const estimatedWastedBytes = |
There was a problem hiding this comment.
love the explicitness here 👍
lighthouse-core/scripts/legacy-javascript/create-polyfill-size-estimation.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
|
FYI @connorjclark I think you have a few conflicts to resolve before this lands |
Related: #10369