-
Notifications
You must be signed in to change notification settings - Fork 90
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
Add Rollup.js tree-shaking and terser minification #80
Conversation
a681bb6
to
c60467e
Compare
6316a0a
to
656a7fa
Compare
d6bd7f1
to
0c72eee
Compare
Thanks for this! If I do:
Everything works great - it seems to minify fine to However adding Any thoughts? How do you feel about just using I know we don't do that for esprima at the moment but it seems like this would be a great opportunity to ditch that and clean things up. Looking at https://github.com/opichals/rollup-plugin-espruino-modules it looks like you implement your own code to get modules, so (for instance) if you're using the project sandbox on the chrome web app and you turn on rollup then it looks like wouldn't find modules any more. Or if you stick a module in a There seems to be quite a lot of How difficult do you think it'd be to pull just the main bit of To get a module it's just |
Just fixed by opichals/rollup-plugin-espruino-modules@4d00e52 .
This is not possible. Normally the internally used
You are right here. I didn't know about the sandbox feature at all till now.
This should work just fine. It does check the modules folder first and if not found there it downloads the module file from the |
I implemented the module resolution logic as well as the built-in module check because my thinking was rather in reverse to how you approach it. I thought we could provide a complete Rollup.js suite for Espruino to be able to separate the JavaScript processing from the rest of the EspruinoTools' internals. The EspurinoTools would rather plugin into the rollup eventually. If you'd find it the idea suitable I am ready to contribute the whole repo to live under the Espruino organization on github for you to have it under control. That said it should not be too much work to make a rollup resolver based on the What do you think? |
Drafted the |
Thanks! Since you're using I guess the real benefit of using the module would be to be able to use NPM packages when running on Node, and you wouldn't get that with Still, It's great at the moment that the CLI and Web IDE on all platforms re-use all the same code - and that makes testing a bug finding much easier. It'd suck to go backwards and suddenly start having issues that occurred in the IDE but not the CLI (and vice-versa).
I'm really not convinced about that at the moment. What is the benefit of making everything a Rollup plugin, rather than just including Rollup as part of Espruino's processing pipeline? EspruinoWebIDE is basically built on top of EspruinoTools, using the same plugin system. Then there's a lot of hidden behind-the-scenes stuff, like figuring out which board is connected and loading the JSON (and getting the list of modules which comes from the board itself), then formatting the output code so it can be parsed correctly by Espruino's REPL and tagging it all with line numbers so the debugger can work. Not to mention things like the JS compiler and assembler. What about writing the code to the board itself, and providing a REPL into it? How would Rollup provide those? I imagine it's going to take me a few weeks of learning/porting/bugfixing/support to get sorted, and then there's going to be a bunch of ongoing effort from having to support maybe 10 or so extra NPM packages and GitHub repositories for all the extra Espruino plugins - so from my point of view there's got to be a big benefit for Espruino users. |
Ah, yes, it should. Will try.
Not absolutely sure about that though with a 900K minified bundle.
Well it would make life easier for JavaScript newcomers. They know or heard of rollup already. They would be able to inspect the default rollup configuration and change it at any point to e.g. include Having said all the above I very much understand your maintenance concern. I am going to get As for the |
That sounds great - thanks! I think you're right about the bundle though - hopefully the two bits of code should be identical? Just one is minified/bundled, one isn't? You're right, debugging anything in rollup when it's minified will be a nightmare - can we leave it as-is? |
Do you mean leaving the browser bundle unminified? That would balloon its size quite a bit (I think around 3MB) which I am not sure is a good thing for the Web IDE. |
No, just what you have already - dependencies on |
They are bundled by rollup itself with It is only necessary to not to forget to do 'npm run build' after any |
f64c064
to
9a97903
Compare
Uses `opichals/rollup-plugin-espruino-modules` * Add Config.ROLLUP which replaces `loadModules` The core/modules.js transformForEspruino directly use the `espruinoRollup.loadModulesRollup` instead of `loadModules` * Separately add TERSER minification option * Srap with UMD preamble for browser compat
Runing `npm run build` gets the bundle updated from the ./libs/rollup folder contents.
@gfwilliams Pushed the changes, please review. |
@gfwilliams What do you think about it as it is now? Do you see any issue? |
Thanks for the prod. I guess I'd prefer that rollup-plugin-espruino-modules was actually part of EspruinoTools, but I can see why you want to make it standalone in its own right. Let's merge and see what happens :) |
@gfwilliams Yes, I like the fact it is possible to use But as I suggested earlier I would be for moving the |
The functionality is off by default so is should not affect users unless they set
ROLLUP
to true or set(MODULE_)MINIFICATION_LEVEL
toTERSER
in the job file.Uses https://github.com/opichals/rollup-plugin-espruino-modules
Add Config.ROLLUP which replaces
loadModules
The core/modules.js transformForEspruino directly use the
espruinoRollup.loadModulesRollup
instead ofloadModules
Separately add TERSER minification option
The minified
libs/rollup/espruino-rollup.browser.js
bundle currently commited is 907K. The libs/rollup is built itself bynpm run build
command (which by itself uses rollup to create the browser suitable bundle).Perhaps this PR could be deemed superior to the #68 as it is considerably smaller in size and by an order of magnitude better in performance.
Fixes the #64 by providing offline minifier method which gives us a possibility to deprecate and eventually decommission the Esprima-based minification.
Using rollup fixes #27