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
[Up for grabs] Reimplement minifier-js using Babili or Closure Compiler instead of UglifyJS #8378
Comments
So, I'm looking into picking up this project from you guys, but I have a few questions. I am thinking of using Babili, which in terms of speed is the clear winner here. However, are you looking for the best minification or the best speed? Additionally, since Babili integrates into the Babel toolchain, it's probably best that this PR comes in on the meteor-babel package as a plugin, thus removing the need for this package entirely since Babili can also minify ES5. |
@sethmurphy18 Awesome! Your questions indicate you understand the problem space. Here are my thoughts in response:
|
Ok, the speed penalty shouldn't be a killer here. Additionally, when I am finished, I will run some benchmarks on actual Meteor code to provide some context into what exactly the ramifications are. Since everything is gzipped we might simply benefit from basic minification, but my benchmarks should reveal more about that. Okay, since Meteor treats minifiers differently, maybe I can look into simply keeping the minify-js, and incorporating some of the elements from the Atmosphere package, assuming the original author doesn't mind. I need to check his license and make sure it's compatible with Meteor's core license. |
Okay, so using what @abernix has created is out unless I get specific permission because the source is not on GitHub, although I am creating a test project and adding it now so I can see if there is a license inside the package itself. |
In case it wasn't clear, @abernix works for Meteor Development Group (with me!), though he lives in Finland, so he's asleep right now. I imagine he'd be willing to let you use the code. |
Ah okay, so since he is part of MDG, does that mean that incorporating parts of his code would be okay? |
Good morning! 😄 Thank you for your consideration of any licensing I may have had! (But mainly sorry that I didn't publish the code for those packages). I had originally intended it to just be a test, but it's caught a bit of traction. Here are the two repos (with MIT licenses!): You'll find that my own packages are taken almost entirely from MDG's (similarly-named) packages here and here respectively. I don't believe the changes were substantial aside from changing the That said, they still fail for many though and I'm not sure you'll find much help in there but you are welcome to use any code in "my" packages in any proposed solution since I'm employed by Meteor Development Group. |
As a stop-gap measure until this issue is resolved, I've updated my This should at least make it easier for developers to know which dependency is using the newer ECMAScript language which UglifyJS does not understand and allow them to react accordingly. Thanks for taking a look at this, @sethmurphy18. If you do have any questions in your Babili/Closure endeavors, don't hesitate to ask! |
Ok, so I have a plan, and I want MDG guidance before I proceed because this is a pretty big change. My plan is as follows:
I will start working on the code, but I will not make any commits until I have the okay from MDG to proceed. One small caveat to be aware of is that the plugin will not function in production until the new npm version of |
Ladies and gentlemen, I give you progress. Right now I am just testing the
|
Great news! 🎉 🙌 Two thoughts:
|
@sethmurphy18 Awesome! 👍 Could you also expose some configuration capabilities?
PS: I've created a package that does just that based on UglifyJS. Here's the options that I've exposed: https://github.com/ssrwpo/uglifyjs2#default-options |
@benjamn Got you, I will update the |
Also, I am adapting Babili's benchmark to use meteor-babel that I created so I can provide some real-life numbers to help quantify exactly what we are doing here. Although my computer is essentially a glorified toaster so the |
Also, I am going to submit my PR for meteor-babel. I have done everything that I think needs to be done there. NOTE: Tests will always fail for versions of Node < 4.0. Babili requires Node 4.0 or higher. |
@PEM-- Before, I get to the point of putting this in my repo, I am going to run it by MDG. If you are part of MDG, I apologize, I can't keep up with who is and who isn't. I created a Gist that transforms the plugin into something like your code. I want MDG guidance on this one. To override the settings, you can do one of two things. One, add this to your {
"standardMiniferJs": {
"agressive": true,
"excludeFiles": ["packages/ddp-server.js", "packages/shell-server.js"],
"debugMode": false,
"forceDevelopmentMinification": false,
"deadCodes": ["_meteor.Meteor.isServer", "Meteor.isServer"]
}
} Two, add this to import { Meteor } from 'meteor/meteor';
import { meteorBabelMinifier } from 'meteor/standard-minifier-js';
Meteor.startup(() => {
meteorBabelMinifier.settings({
agressive: true,
excludeFiles: ['packages/ddp-server.js', 'packages/shell-server.js'],
debugMode: false
forceDevelopmentMinification: false,
minifyPlugins: [
require('babel-plugin-constant-code-folding')
],
deadCodes: ['_meteor.Meteor.isServer', 'Meteor.isServer']
});
}); It should be noted that you cannot declare plugins in the |
I will proceed once I have MDG guidance. Once that is finished though, the pull request can be created. Although, the package won't function until the meteor-babel package containing the minify function is published. |
Super nice @sethmurphy18. I'm not part of MDG. You're right to wait for their guidance on this. Actually bringing this code removal as default for Meteor would close #6123, #6402 and partially close #4831. Having the capability to go even further with a configuration file is a nice practice. The defaults are assuring "conventions over configurations" while still retaining the configuration capability. |
@PEM-- The 2 main things I am concerned with is |
The The The One thing that these options does not resolve is the capability to bundle multiple JS files. With the code splitting capability that @benjamn has introduced, we will need a mean to tell the minifier where to put our files and for which type of bundles. To illustrate what I'm saying, If we want to use service workers for devices capable without losing compatibility for older browsers, we need to bundle a default JS file and a service worker enable one. |
Oh, I know the purpose of everything, but from what I have seen so far when it comes to Meteor, it just works, lol. I understand why those options are helpful which is why I put them in, I just don't want to make changes like that without approval. Yeah, I understand what you mean when it comes to bundling JS files. That is a tricky topic, and would be pretty difficult to implement. Doable though, given Webpack does it. |
100% agreed with MDG's approval 😉 When I've started my journey into this minifier, I've seen in it so many interesting opportunities to catch up with Webpack and Next. This all drills down to a simple list of targets in the settings and a list of files for each target. As the minifier has the list of all files and given him the opportunity to get configured via the settings... well, you know where I'm heading 😄 |
Oh yeah, for sure. |
Alrighty guys, especially @abernix, the benchmark is working and results are in. Before I post the results, I will say that I tried to simulate the Meteor environment as well as I can. The benchmark makes a call for settings, then compiles the code, and minifies it without providing any options so
|
If anyone wants to download and run the code, it's available in my repo, or as an NPM package, and I highly recommend you do, since my computer is an overly glorified toaster. |
Just needed to give a big virtual hug (or beer) to sethmurphy18 and all people that are focusing on this issue... I am not capable to tackle those bugs so I follow very closely your discussions. It is so amazing to see how people work together in open source. Never enough said, thank you guys ! |
@abernix: We could for now support configuration through |
@mitar I'll let you guys sort that one out, I have accomplished what I set out to do. I can write the configuration code one you have it decided how you wanna do it. |
Thank you @krem06. I do what I can. I don't really follow issues per say, I was actually trying to find information on Meteor 1.5 when I came across this, and now it has kinda morphed (at least for me) into proving myself to MDG since they are actually hiring and I would love to work on this project. |
Also, if anyone can come up with/find one. I need an ES6 torture script to test the minifier with. It needs to use all of the ES6 features, then I will modify the benchmark to make it more appropriate, since each minifier will require the file to be compiled (except stock Babili). |
If you wanted to try, that'd be interesting to see. The official |
@mitar I'm sorry you don't like my proposal (as indicated by your 👎 above). Perhaps you could speak to some specific parts of what you don't like while still keeping in mind that I've left the door open to configuration in the future but am simply against making this PR too much more than what it was: A way to bring an ECMAScript compliant minifier back into the Meteor story where UglifyJS has failed. |
I have done so. :-) We have done a similar thing for autoprefixer. |
I definitely saw that, but you can imagine my confusion when you thumbs down a longer comment where I spoke about at least 3 different options. 😉 I guess you are saying you don't like them all! Either way you are still suggesting something more than just releasing this first and then adding configuration later. In consideration of a configuration option for the future though, we need to address the entire app and not just packages. Are you suggesting per-file minification options via |
That would actually be an excellent idea if that is what he is suggesting, at least I think @mitar is a he. |
I mean, it is really easy to add options to I am not sure if minimization happens per package though, I think minimization is just done on everything at once. So maybe this idea for per-package options as a start does not apply. |
There is per package processing of the packages by the minifier, but your solution only addresses packages and not the rest of the app. Anyhow, we can consider that for when configuration of the minifier becomes possible but a partial solution seems less than ideal when a more complete solution (with a likely different implementation that would benefit more users) might be around the corner. It will certainly be something we're looking at as we embark on more 1.5 steps to improve the page load performance and the bundle sizes we ship to the client! |
Thanks a lot @sethmurphy18 for doing this. I'm pretty ignorant of how these packages work but I'm wondering if switching minifier makes it easier to solve 4384 ? |
@lynchem Not any easier with a new minifier, no; the old minifier supported generating source maps as well. Keep following the other issue! |
If this all works, @sethmurphy18 deserves at the very least a new toaster |
@sethmurphy18 would not complain about anything...lol. |
It seemed that this was near to completion. |
As an update, this change actually caused some compatibility issues with Blaze that I worked with @mitar and @abernix to resolve. Additionally, there was some waiting time involved because @benjamn was on vacation and we needed the npm |
We are aiming to get this integrated soon. There are a few other large things in motion at the moment but should have some extra time to look at it as soon as Meteor 1.4.3.2 (#8470) ships (most likely Monday, please help test!). This is a priority for us! |
The PR for this has been approved and merged and will be included in an upcoming version of Meteor. To repeat what @benjamn has already said:
|
No problem guys. |
UglifyJS is the fastest JavaScript minifier, but it STILL does not officially support ECMAScript 2015+ features that are native in Node 4+ and most browsers: mishoo/UglifyJS#448
This leads to problems that only appear when you try to deploy your app or run
meteor run --production
), since that's when minification happens. To make matters worse, the parsing error messages produced by Uglify are pretty bad (no filename, long and useless stack trace).As more and more npm packages use features that UglifyJS can't handle, Meteor developers are exposed to this risk whenever the versions of any packages in
node_modules
change, even transitive dependencies whose versions the Meteor developer cannot control.Here's the most recent issue caused by this regrettable situation: #8370
Others:
--production
#6405I previously tried and failed to update
minifier-js
to use theharmony
branch ofuglify-js
, although @abernix has a package that does something similar.All in all, I think the time has come to choose another minifier.
The most compelling options seem to be:
I don't have a strong preference between the two, so choosing the right one is definitely part of this project. Both are slower than UglifyJS, but in fairness they have more work to do, because they handle a much wider variety of input syntax.
If you want to get involved, have a look at
minifier-js
, and comment here. I'm not necessarily looking for a single champion; any kind of insight is helpful and welcome. I know @abernix and I will be happy to answer any questions you have.The text was updated successfully, but these errors were encountered: