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

core(unminified-javascript): replace esprima with custom tokenizer #5925

Merged
merged 7 commits into from
Sep 27, 2018

Conversation

patrickhulce
Copy link
Collaborator

Summary
Removes dependency on esprima for JS minification savings and replaces it with a custom scanner/token counter similar to our CSS one. Putting this up for discussion before hardening it and going down the edge case rabbit hole.

Pros

  • Faster than esprima (down to ~750ms from multiple seconds)
  • Removes another bundle dependency
  • We control our own destiny with bug fixes, etc

Cons

  • We maintain a pseudo-JS parser
  • Not nearly as well tested (though given the problems we've encountered with esprima I'm not super confident anyway)

Anecdotal testing I've done so far is run on a couple sites and make sure the % savings are the same for each file.

Related Issues/PRs
fixes #5723

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.

Personally think this is a good direction to go, particularly since we're only interested in testing such a superficial level of minification anyways.

It would be cool to be able to guess at local variable names and replace them with minified versions to really approximate the baseline level uglify/whatever minification of JS, but not as important and just more state to track.

Anecdotal testing I've done so far is run on a couple sites and make sure the % savings are the same for each file.

Post these once you're more seriously tackling this?

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.

hot. lgtm

@patrickhulce
Copy link
Collaborator Author

patrickhulce commented Sep 24, 2018

some stats from runs, byte numbers are LH-reported savings, the usual culprits have gone and fixed their minification savings I used to find (yay!), so if you've got good examples, happy to have them :)

Site Before After
The Verge <2KB 0ms savings (0.5s) <2KB 0ms savings (0.2s)
CNN 3KB 0ms savings (1s) <2KB 0ms savings (0.5s)

Instead I used a few libraries during my testing, here's the results

Library Full Unminified Size Predicted Savings Before Predicted Savings After Real Savings
jQuery 75 KB 37 KB 37 KB 46 KB
Angular 297 KB 223 KB 210 KB 240 KB
React 15 KB 7 KB 7 KB 12 KB
Moment 31 KB 13 KB 13 KB 15 KB

@patrickhulce
Copy link
Collaborator Author

@brendankenny @exterkamp how is this lookin' to you folks?

fine if no opinion, just a bit on the big side to expect no comments ;)

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! This is great. Bundle size goes way down.

Since we'll fully control this tokenizer (and likely not change it much), it does feel like we could run it over largish files and get repeatable results without much test churn, but I don't have a good suggestion for candidates for this (files under our control we have, but not ones we change so rarely that they'd function as good canaries...)

@brendankenny
Copy link
Member

candidates for this

we could bring in a dev dependency at a fixed version that we won't ever be using, but could use the contents entirely for smoke testing the CSS/JS tokenizer...

@patrickhulce
Copy link
Collaborator Author

we could bring in a dev dependency at a fixed version that we won't ever be using, but could use the contents entirely for smoke testing the CSS/JS tokenizer...

I like it. What's the biggest, most complex front end dependency we can think of? :)

@paulirish paulirish merged commit 54c2b5d into master Sep 27, 2018
@paulirish paulirish deleted the no_more_esprima branch September 27, 2018 21:51
@patrickhulce
Copy link
Collaborator Author

Oh sorry! Was waiting on ideas for smoke test :)

Will do angular in a follow-up if no other suggestions.

@paulirish
Copy link
Member

paulirish commented Sep 27, 2018 via email

@brendankenny
Copy link
Member

Was waiting on ideas for smoke test :)

ha, oh yeah, sorry!

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.

Minified JS overestimates savings when esprima fails
3 participants