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

Minification #355

Closed
wants to merge 3 commits into from
Closed

Minification #355

wants to merge 3 commits into from

Conversation

rigrig
Copy link
Contributor

@rigrig rigrig commented Apr 26, 2020

See #3

Not sure if this is the best way to go about it, but I figured I'd give it a shot:

  1. Adds bin/minify.js script to minify *.js files
  2. Uploader checks if a -min.js version of files exists

https://github.com/terser/terser looked like a nice minifier

So if you don't run the minifier, the impact is that the uploader does a bunch of failed requests before falling back to uploading the not-minified version of files.

-min.js files are ignored and not added to the repo. Not sure if there should be a package.json script entry to run this, or some other build process?

What worries me a bit is that having the minifier mangle names seems to result in broken code, and I'm not sure why. I suspect it might have something to do with const and let being treated as var, so actual scopes mismatch expectations?
With mangling off it seems to reduce sizes by about 30-50% although it varies quite a bit (and e.g. image strings don't really compress)

Only for *.js files (except *-min.js), not when evaluate=true
Also adds .gitignore entry for *-min.js files
Minifies javascript files using terser: https://github.com/terser/terser
All storage files with a "*.js" url are minified, except when
 - evaluate=true
 - filename already ends in "-min.js"
 - storage already lists a corresponding  "-min.js" file
Not sure why, but otherwise Stuff Breaks :-(
@Purple-Tentacle
Copy link
Contributor

Removing comments would already help a lot for my code :)

@gfwilliams
Copy link
Member

Thanks! Yeah, I was wondering about this. I reckon probably the most sensible thing to do is to use the existing Espruino tools for minification: https://github.com/espruino/EspruinoTools

Main reason is there have been questions about stuff like including custom fonts and modules, which would get done automatically. Also pretokenisation would compress the files down even further.

So then the question is really: do we just use the tools online, or do something offline? If we did it online then we could still allow folks to test their apps really easily using GitHub pages...

@rigrig
Copy link
Contributor Author

rigrig commented Apr 27, 2020

Using existing Espruino tools does look like the best solution yeah, I noticed it even has different minification tools to pick from :-)

Online would be nice for testing, but re-minifying the code for everybody every time they just want to install an app doesn't feel optimal. Also, while developing an app you probably don't want it minimized all the time, because debug output gets a lot less useful.

How about something like

  1. Adjust the minify.js script to use Espruino tools (add some argument to pick the used minimizer)
  2. Give the app loader a "Minification level" selection under "Utilities". It would have a default option "Use Pre-Minified code": this would look for existing -min.js files like in this PR. Other options (except "no minification") perform minification on the fly when uploading.

How are custom fonts and modules supposed to work? You require() them, and they are included in the generated code?
Or is the idea that they should get uploaded as separate files, so other apps can reuse them? (but then you'd get into dependency management, which seems... a bit more challenging)

@gfwilliams
Copy link
Member

Well, I'd decided to have a go at this - I'd been planning it for a while.

Branch here: https://github.com/espruino/BangleApps/compare/minify_online

Apart from the big blob of JS for EspruinoTools it seems pretty tidy. Give it a try - I think you'll find it's quick enough that you wouldn't really notice anything different.

But are you say it'd be good to have a toggle for whether minification was applied or not. What I'm concerned about with the offline minification is for non-Node.js users they're basically not going to test with minified code at all because it's a bit of a steep learning curve on top of developing an app.

How are custom fonts and modules supposed to work? You require() them, and they are included in the generated code?

Yep, that's right. I think right now it's better to just include them.

@rigrig
Copy link
Contributor Author

rigrig commented Apr 27, 2020

I gave it a try, and it was indeed fast enough to hardly notice.

Seems it breaks the ballmaze game though :-( Maybe because it mangles names and ends up reusing names like E and g?

@gfwilliams
Copy link
Member

Can you try now? It might just be an issue with the tokeniser I was using

@rigrig
Copy link
Contributor Author

rigrig commented Apr 27, 2020

Still broken.
I think I found the problem though: the code uses const/let. Espruino handles those as var. The minifier doesn't know that, so it assumes it can reuse variable names inside different scopes, while it can't.

When I stick in content = content.replace(/\b(let|const) /g, "var "); just before return Espruino.transform(content, {, the minified app works.
It still reuses E and g as variable names though, I'm sure why that doesn't break things.

(And if go to the app code and replace all const/let with var, I get a truckload of warnings about duplicate variable names :-( )

@gfwilliams
Copy link
Member

Thanks for tracking that down!

I just had a quick look to see if we could configure it in the minifier (esmangle), and I'm not sure we can (short of actually modifying the source). Not sure whether another minifier might be better?

While I can and should fix that in Bangle.js firmware that doesn't help people who already have the old firmware, for which minified stuff will fail silently :(

Any thoughts? The simple one I guess is just to manually plough through apps and change the let to var

@gfwilliams
Copy link
Member

for now, one option is just to remove minification and use pretokenization

@rigrig
Copy link
Contributor Author

rigrig commented Apr 28, 2020

Not sure whether another minifier might be better?

Can we not just turn off variable name mangling? Looks like there is a MINIFICATION_Mangle option.
I guess every other minifier would have the same problem. (I know terser did)

fix that in Bangle.js firmware

You mean implement const/let scopes? That would probably be the nicest solution, maybe even do something fancy like disable mangling depending on firmware version?
Slightly problematic that even unminified code can fail if you reuse variable names, and then you get "It works on my device" bugs :-(

manually plough through apps and change the let to var

I don't know, that seems like something we're going to regret. And I think it would also mean banning it's use in all new apps, along with having to explain this issue almost every time a new author submits something.

@gfwilliams
Copy link
Member

Can we not just turn off variable name mangling?

Probably, yes. But then I wonder if there's much point doing it? Pretokenisation removes all whitespace anyway...

You mean implement const/let scopes?

Yes. I guess we could detect when 2v06 is released and in that case enable the proper minification?

Yeah, changing all the apps right now would be a pain. It would solve the problem though ;)

@rigrig
Copy link
Contributor Author

rigrig commented Apr 28, 2020

Pretokenisation removes all whitespace anyway...

I think some minimizers can also do clever inlining of functions.
But I guess that just the pretokenisation is a big win which will work right now, and fiddling with minimizer settings probably isn't worth it (until the firmware is updated and minification just works without fiddling).

@gfwilliams
Copy link
Member

I've just merged the pretokenisation branch, with a setting for turning it on and off.

Closing this for now then... Maybe later when I sort out the let/const issue we can have a toggle for minification

@gfwilliams gfwilliams closed this Apr 29, 2020
@rigrig rigrig deleted the minification branch April 29, 2020 08:30
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.

None yet

3 participants