core(legacy-javascript): make opportunity#10881
Conversation
|
I'll take a closer look at this tomorrow @connorjclark I promise :) |
patrickhulce
left a comment
There was a problem hiding this comment.
I think this is great! I love the graph direction and would love to see it included in the test of legacy-js to make sure it doesn't fall out of sync. I can imagine us forgetting to update it in the future if we don't
| 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>
patrickhulce
left a comment
There was a problem hiding this comment.
flushing for now but didn't realize how much had changed since last review, i'll update the label and take a look tomorrow
| * @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
patrickhulce
left a comment
There was a problem hiding this comment.
few naming q's and the location of estimate transfer ratio up for debate but other than that looks good
| estimatedWastedBytesFromTransforms += pattern.estimator(result); | ||
| } | ||
|
|
||
| const estimatedWastedBytes = |
There was a problem hiding this comment.
love the explicitness here 👍
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