Skip to content
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

fix(unused-css-rules): update to support new coverage format #2518

Merged
merged 8 commits into from
Aug 18, 2017

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented Jun 16, 2017

config changes are commented out until #2499 lands and I can split into a full-config.js and a default-config.js

a couple caveats to note

  • I had to remove Number Unused Rules because the new coverage only reports string offsets of used portions, I had a version of this PR that actually did the mapping of lines/columns to string offsets and counted the number of rules but it required the parsed stylesheets and because the parsing so frequently fails this meant that we rarely got them anyway (not to mentioned it greatly increased the complexity and readability of what was going on)
  • The size numbers reported here are still not going to match what you see in DevTools since the point of this is to audit is to highlight the network savings not the uncompressed/expanded CSS bytes that the coverage tool reports. Example: on paulirish.com DevTools reports 240kb of unused CSS when the total downloaded CSS is only ~40kb
  • The URL reported by header.sourceURL doesn't seem to be 100% accurate, I added a debugString and hopefully we'll be able to track some more cases with core(lib): add error reporting #2420
  • The perf implications of enabling coverage were somewhat inconclusive but the styles gatherer is what can really take significant amounts of time, so I stopped investigating and opted for feat(config): add audit blacklist support #2499 instead since we'll have a place going forward to collect these "extras" and make them accessible with a simple flag or checkbox instead of forcing the slowdown on all users

// 'styles',
// 'css-usage',
// ],
// },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with #2499 landed, time to uncomment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep rebased it all 👍

@patrickhulce
Copy link
Collaborator Author

PTAL :)

@patrickhulce
Copy link
Collaborator Author

🏏 🏏


let debugString;
if (!isInline && !stylesheetInfo.networkRecord) {
debugString = `Unable to find network record for ${stylesheetInfo.header.sourceURL}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get this error on both paulirish.com and theverge.com.. can we avoid that?

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice updates

can also drop the Chrome 56 warning in css-usage.js now :)

// }
// },
'unused-css-rules': {
score: '<100',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make this more precise since we control the test page?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, the score gets bucketed by the generic byte efficiency audit though so it's not that useful. I added an assertion on the "wastedKb" instead which is a little more explicit and useful

@@ -4,7 +4,7 @@ node lighthouse-cli/test/fixtures/static-server.js &

sleep 0.5s

config="lighthouse-core/config/default.js"
config="lighthouse-core/config/full-config.js"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to update byte-config.js instead (after #2732)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

// we can guess it was roughly the size of the content gzipped.
const totalBytes = stylesheetInfo.networkRecord ?
stylesheetInfo.networkRecord.transferSize :
Math.round(stylesheetInfo.content.length / 3);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is the full run, not the default run, instead of estimating like this can we just actually run gzip on it like response-compression does?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't actually know it was gzipped anyway I'm inclined to not introduce that complexity here too, but I don't feel that strongly.

/**
* @param {!Object} stylesheetInfo
* @param {boolean} isInline
* @return {{debugString: string|undefined, wastedBytes: number, totalBytes: number}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wastedPercent, too

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return {
debugString,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add to return type

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


// If we don't know for sure how many bytes this sheet used on the network,
// we can guess it was roughly the size of the content gzipped.
const totalBytes = stylesheetInfo.networkRecord ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this be weird for inline stylesheets that came in a page that was gzipped?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be weird for inline stylesheets that came in a page that was not gzipped since we'll underestimate the savings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be weird for inline stylesheets that came in a page that was not gzipped since we'll underestimate the savings.

ah, yeah, that was based on how it was (I believe) filtering out networkRecords for non-stylesheets before

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just the two last questions

const wastedBytes = Math.round(percentUnused * totalBytes);

return {
wastedBytes,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not worth logging debugString anymore for network records not found (it'll show up in HTTP Archive data that way)? Or we could sentry it, maybe. Otherwise we won't have any feedback on how often we're failing at finding network records...or it might not matter :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah didn't seem worth it anymore, the cases where it happens now are wholly expected and there's no sense being noisy

const wasCompressed = !networkRecord ||
networkRecord._transferSize !== networkRecord._resourceSize;
totalBytes = wasCompressed ? Math.round(totalUncompressedBytes / 3) : totalUncompressedBytes;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a little confusing to follow. What do you think of something like totalTransferredBytes for totalBytes and then maybe folding the first short circuit into the conditional like

let totalTransferredBytes;
if (networkRecord && networkRecord._resourceType._name === 'stylesheet') {
  totalTransferredBytes = networkRecord.transferSize;
} else {
  // If we don't know for sure how many bytes this sheet used on the network,
  // we can guess it was roughly the size of the content length.
  const wasCompressed = !networkRecord ||
      networkRecord._transferSize !== networkRecord._resourceSize;
  totalTransferredBytes = wasCompressed ? Math.round(totalUncompressedBytes / 3) :
      totalUncompressedBytes;
}

the variable rename alone might be enough to clear things up, though

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah sg, done

@patrickhulce
Copy link
Collaborator Author

done! c'mon travis 🤞

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🎨🔢


const networkRecord = stylesheetInfo.networkRecord;
let totalTransferredBytes;
if (!networkRecord) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulirish how's this?

@paulirish paulirish merged commit fb19596 into master Aug 18, 2017
@paulirish paulirish deleted the css_coverage branch August 18, 2017 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants