Skip to content

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

Merged
connorjclark merged 6 commits into
masterfrom
bundle-inline-terser
Sep 21, 2020
Merged

misc(build): use terser on inline assets#11461
connorjclark merged 6 commits into
masterfrom
bundle-inline-terser

Conversation

@connorjclark

Copy link
Copy Markdown
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
Comment thread build/build-utils.js Outdated
}

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

Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread build/build-utils.js

@patrickhulce patrickhulce left a comment

Copy link
Copy Markdown
Collaborator

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?

Comment thread build/build-utils.js
Comment thread build/build-utils.js
Comment thread build/build-utils.js Outdated
}

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

Copy link
Copy Markdown
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

connorjclark commented Sep 18, 2020

Copy link
Copy Markdown
Collaborator Author

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
Copy Markdown
Collaborator

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

SGTM 👍

@patrickhulce patrickhulce left a comment

Copy link
Copy Markdown
Collaborator

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