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): minify bundles with terser #9605

Merged
merged 35 commits into from
Sep 17, 2020
Merged

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Aug 23, 2019

  1. removes usage of babel for minifying, replace with terser
  2. emits source maps for each bundle
  3. adds a more detailed comment at the top of each bundle

image

I encourage reviewers to yarn build-lr and view the map on https://sokra.github.io/source-map-visualization/

dist/lightrider/lighthouse-lr-bundle.js 9.60 MB -> 8.94 MB (7%)
dist/lighthouse-dt-bundle.js 2.10 MB -> 1.62 MB (23%)

build/build-bundle.js Outdated Show resolved Hide resolved
@connorjclark
Copy link
Collaborator Author

connorjclark commented Aug 23, 2019

Note: the viewer needs additional changes to its build script to emit a map. This only emits a map for LR, the extension, and DT.

@connorjclark
Copy link
Collaborator Author

connorjclark commented Aug 23, 2019

some changes....

there's an extra space in the fn declaration for modules:
image

@connorjclark
Copy link
Collaborator Author

connorjclark commented Aug 24, 2019

not obvious why the devtools bundle got so much bigger (+ 504KB on disk).

Here's the visual bundle:
image

Can't do the same for "base"/master, because this tool requires a source map :)

@connorjclark
Copy link
Collaborator Author

connorjclark commented Aug 24, 2019

got a visual bundle from the previous process of browserify -> babel (ccf966d).

image

as you can see, "unmapped" went up a lot.

@connorjclark
Copy link
Collaborator Author

connorjclark commented Aug 24, 2019

the answer is in here somewhere... (diff b/wn dt bundles) https://gist.github.com/connorjclark/c80ca014042950ff0dd15ee23fcb6f42

Must be related to the babel version bump. I think it's related to "minifying" with a transform during browserify, instead of processing the output of browserify.

@@ -5412,6 +5228,14 @@ module-deps@^6.0.0:
through2 "^2.0.0"
xtend "^4.0.0"

mold-source-map@~0.4.0:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is an interesting package

@connorjclark
Copy link
Collaborator Author

The culprit is the browserify builtins. They don't get processed by the babelify transform - so comments, whitespace, all remain.

@connorjclark
Copy link
Collaborator Author

connorjclark commented Aug 27, 2019

Here where we're at:

We do browserify -> babel. Bundle is 1.6M. Babel's job is just to reduce the size of the script (via retain_lines and compact). Great when their were two passes. However, emitting source maps with this two-pass compilation created off-by-one issues.

Moving babel into browserify via babelify is cool, but it missed out on minimizing the builtins that browserify injects. This adds ~0.3M to the devtools bundle.

We could completely remove the line retaining minimization, and just use Terser. This save ~0.4MB. Source maps are perfect in this two-pass compilation. Terser handles input maps better than babel, apparently.

I asked @patrickhulce about the utility of "retain lines" in the bundle output. He said it originated from wanting "readable" diffs when rolling to DevTools. I wonder if that is a behavior we should bother keeping?

@patrickhulce
Copy link
Collaborator

He said it originated from wanting "readable" diffs when rolling to DevTools

yeah that and the moderately improved stack trace is a nice feature of retainLines, but I'm not sure how much we need either at this point. I can't remember the last time we got a legit bug report where the bundled stack trace was useful. If we had a mechanism (script?) that could easily translate a stack trace using our sourcemap, saving 0.5MB sounds like a steal to me.

@connorjclark
Copy link
Collaborator Author

If we had a mechanism (script?) that could easily translate a stack trace using our sourcemap, saving 0.5MB sounds like a steal to me.

covered! https://www.npmjs.com/package/stack-beautifier

npx stack-beautifier -t <file_with_stack.txt> <app.js.map>

@brendankenny
Copy link
Member

I like getting rid of yet another minifier in our deps :)

Did we figure out the serving thing, though? My main thing is probably the occasional stepping through LH in devtools and the current formatting does make that actually viable without a source map.

@brendankenny
Copy link
Member

Agreed with Patrick that it's pretty rare I find myself actually doing that, but then again that means what use are source maps? :)

@connorjclark
Copy link
Collaborator Author

connorjclark commented Aug 27, 2019

Did we figure out the serving thing, though? My main thing is probably the occasional stepping through LH in devtools and the current formatting does make that actually viable without a source map.

For devtools / Lightrider, you can attach source maps from a URL, which you would get by checking out the correct sha, building, and starting a local server in dist ... I think that's the best solution we have.

But don't forget the viewer - it's trivial to use maps there.

@connorjclark
Copy link
Collaborator Author

Worth noting: we have no tests that utilize the output of our bundled code (ala how React tests their lib). We might do good to run the smoke tests on the bundled code (#9501 references how).

@connorjclark connorjclark changed the title misc: emit source map for bundles misc: minify bundles with terser, emit source map Oct 4, 2019
Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

unfortunately, building the LR bundle now takes ~60 seconds. but there's not much we can do about that.

a few small things, but is good to get a green

@@ -308,7 +310,6 @@ class TapTargets extends Gatherer {
*/
afterPass(passContext) {
const expression = `(function() {
const tapTargetsSelector = "${tapTargetsSelector}";
Copy link
Member

Choose a reason for hiding this comment

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

i looked through the other uses of evalAsync for a similar situation and i think we're good.

only one that caught my eye was L116 of the start-url gatherer

.evaluateAsync(`window.location = '${startUrl}'`)

but looking at the minified source, i think we're ok

image

ya?


in minified land this was that taptarget section:

image

Copy link
Collaborator Author

@connorjclark connorjclark Sep 16, 2020

Choose a reason for hiding this comment

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

yea code inside template strings' ${...} are safe

build/build-bundle.js Show resolved Hide resolved
@@ -1,7 +1,7 @@
{
"name": "lighthouse",
"version": "6.3.0",
"description": "Lighthouse",
"description": "Automated auditing, performance metrics, and best practices for the web.",
Copy link
Member

Choose a reason for hiding this comment

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

👍

package.json Outdated Show resolved Hide resolved
@@ -108,10 +108,9 @@
"@wardpeet/brfs": "2.1.0-0",
"angular": "^1.7.4",
"archiver": "^3.0.0",
"babel-core": "^6.26.0",
Copy link
Member

Choose a reason for hiding this comment

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

👋 bye!

package.json Outdated
"browserify": "^16.2.3",
"browserify-banner": "^1.0.15",
"bundlesize": "^0.18.0",
Copy link
Member

Choose a reason for hiding this comment

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

what now

:)

assert.ok(!minified.includes('\nrequire='), 'contained unexpected browserify require stub');
// Add the banner and modify globals for DevTools if necessary.
// This will mess up the source map for the first line, but we don't ship
// the map to DevTools, so that's fine.
Copy link
Member

Choose a reason for hiding this comment

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

what does that leave that gets a source map?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just removed this comment since we aren't shipping any maps in CDT or LR. I put the default map generation back to false. can set to true whenever we want to get a bundle viz.

build/build-bundle.js Outdated Show resolved Hide resolved
build/build-bundle.js Show resolved Hide resolved
Co-authored-by: Brendan Kenny <bckenny@gmail.com>
@connorjclark connorjclark changed the title misc(build): minify bundles with terser, emit source map misc(build): minify bundles with terser Sep 17, 2020
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.

None yet

5 participants