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

misc(build): use terser on inline assets #11461

Merged
merged 6 commits into from
Sep 21, 2020
Merged

Conversation

connorjclark
Copy link
Collaborator

Saves ~32 KB in CDT bundle.

We did the same thing already for the viewer #9823, but the build-bundle never got the same treatment.

Here's what got trimmed:

minifying /Users/cjamcl/src/lighthouse/node_modules/axe-core/axe.min.js. saved 3.195 KB
minifying /Users/cjamcl/src/lighthouse/node_modules/js-library-detector/library/libraries.js. saved 26.963 KB

axe.min.js is quite big, and surfaced the fact that minifyFileTransform only worked if the amount of code fits in w/e Node decides to be the chunk size in the file streaming. Had to fix that.

@connorjclark connorjclark requested a review from a team as a code owner September 18, 2020 22:03
@connorjclark connorjclark requested review from patrickhulce and removed request for a team September 18, 2020 22:03
@connorjclark connorjclark changed the title misc(build): use terser on inline assets in all bundles misc(build): use terser on inline assets Sep 18, 2020

if (result.code) {
// const saved = code.length - result.code.length;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Figured some wouldn't want this logged all the time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Who sees this? maybe behind a debug flag or something instead?

the dead code option is probably my last preference among

  • just log it anyway
  • log it when asked with a flag/env var/something
  • never log it
  • commented code

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

I do get a smidge nervous about the quality of our devtools tests with changes like this though. it's times like these that full smoke coverage for DT would be awesome :)

how would you feel about skipping axe and just running on library detector for now since that's where most of the savings comes from anyway?


if (result.code) {
// const saved = code.length - result.code.length;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Who sees this? maybe behind a debug flag or something instead?

the dead code option is probably my last preference among

  • just log it anyway
  • log it when asked with a flag/env var/something
  • never log it
  • commented code

@connorjclark
Copy link
Collaborator Author

connorjclark commented Sep 18, 2020

I do get a smidge nervous about the quality of our devtools tests with changes like this though. it's times like these that full smoke coverage for DT would be awesome :)

We have yarn test-bundle, which loads the devtools bundle directly. We can add the a11y to yarn test-bundle. Is that sufficient?

@vercel vercel bot temporarily deployed to Preview September 18, 2020 23:19 Inactive
@patrickhulce
Copy link
Collaborator

We can add the a11y to yarn test-bundle. Is that sufficient?

SGTM 👍

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

✂️ ✂️

@connorjclark connorjclark merged commit 0200358 into master Sep 21, 2020
@connorjclark connorjclark deleted the bundle-inline-terser branch September 21, 2020 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants