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

[Up for grabs] Reimplement minifier-js using Babili or Closure Compiler instead of UglifyJS #8378

Closed
benjamn opened this issue Feb 15, 2017 · 50 comments

Comments

@benjamn
Copy link
Member

@benjamn benjamn commented Feb 15, 2017

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:

I previously tried and failed to update minifier-js to use the harmony branch of uglify-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:

  • Closure Compiler, which is implemented in Java, though a JavaScript version (compiled from the Java) is now available.
  • Babili, by the same folks who brought you Babel.

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.

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 16, 2017

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.

@benjamn
Copy link
Member Author

@benjamn benjamn commented Feb 16, 2017

@sethmurphy18 Awesome! Your questions indicate you understand the problem space.

Here are my thoughts in response:

  1. Since minification generally only happens when deploying, I think it's appropriate to sacrifice some speed to get smaller sizes, within reason.
  2. Bear in mind that Meteor serves static assets compressed with gzip by default. There may be some minification plugins that seem like a good idea, but end up not mattering (much) after the magic of gzip. If you can identify any plugins like that, it might save time to disable them.
  3. I agree that the bulk of this implementation might make more sense as part of meteor-babel, though I think it should be a separate API from require("meteor-babel").compile, because Meteor has a separate plugin system for minifiers. This probably means parsing the code again, unfortunately, but that's how UglifyJS has been used until now.
  4. Another reason minification shouldn't just happen in ecmascript/babel-compiler is that Meteor scans compiled code for imports (require, import, export ... from ..., etc.), so it would be problematic if require and module got renamed by the minifier before that.
@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 16, 2017

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.

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 16, 2017

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.

@benjamn
Copy link
Member Author

@benjamn benjamn commented Feb 16, 2017

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.

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 16, 2017

Ah okay, so since he is part of MDG, does that mean that incorporating parts of his code would be okay?

@abernix
Copy link
Member

@abernix abernix commented Feb 17, 2017

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 uglifyjs NPM to use the harmony branch. Any success my packages have had over @benjamn's own are likely due to me using a more recent commit of the harmony branch which was less prone to failure since it presumably had a more up-to-date feature set (You'll probably find no crazy wizardry in there).

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.

@abernix
Copy link
Member

@abernix abernix commented Feb 17, 2017

As a stop-gap measure until this issue is resolved, I've updated my abernix:standard-minifier-js package to capture the specific location of the error in this commit, by traversing the SourceMap decoration, when the error is captured within package/modules.js (the Meteor package which is a concatenated encapsulation of node_modules deemed necessary to ship to the client).

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!

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 17, 2017

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:

  • Update the meteor-babel package to export a minify function, since Babili is a most easily used as a Babel preset it makes sense not to include Babel in another package. (increment minor version by 1 since this a major, but non-breaking change). [NOTE: I plan to accomplish this by including a second set of options that does not transpile the code, but does minify it. Not sure if this will work with Dynamic Import or not]
  • Update the minifier-js package to bring that minify function into the core of Meteor. (increment major version by 1, because this is a breaking API change).
  • Update the standard-minifier-js package to use the meteorBabel.minify() function to minify the code when in production mode. (increment major version by 1, because this is a breaking API change).
  • Update the standard-minifier-js package to accept options from the user. Including what files to minify, and whether or not to remove server code blocks. Proposal rejected by MDG
    • I will put a package on Atmosphere once this package goes live to implement this.
  • Update the babel-compiler package to export the meteorBabel.minify() function.

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 meteor-babel is published. My testing is being completed by using the local version of the npm package. Code is completed though, I am now in the testing phase.

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 17, 2017

Ladies and gentlemen, I give you progress. Right now I am just testing the meteor-babel package to make sure the output is correct. It's not 100% what Babel's blog post says it should be, but it's a start:

Unminified code: 
class Mangler { constructor(program) { this.program = program; } } /* need this since otherwise Mangler isn't used */ new Mangler();


Minified ES6:
class Mangler{constructor(a){this.program=a}}new Mangler;


Compiled ES6:
var _classCallCheck2 = require("babel-runtime/helpers/classCallCheck");

var _classCallCheck3 = _interopRequireDefault(_classCallCheck2);

function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { "default": obj }; }

var Mangler = function Mangler(program) {
  (0, _classCallCheck3.default)(this, Mangler);
  this.program = program;
}; /* need this since otherwise Mangler isn't used */

new Mangler();


Compiled and minified ES6:
var _classCallCheck2=require("babel-runtime/helpers/classCallCheck"),_classCallCheck3=_interopRequireDefault(_classCallCheck2);function _interopRequireDefault(a){return a&&a.__esModule?a:{"default":a}}var Mangler=function a(b){(0,_classCallCheck3.default)(this,a),this.program=b};new Mangler;
@benjamn
Copy link
Member Author

@benjamn benjamn commented Feb 18, 2017

Great news! 🎉 🙌

Two thoughts:

  1. We should try to keep the number of copies of meteor-babel as close to 1 as possible. This probably means the babel-compiler Meteor package should expose require("meteor-babel").minify on the Babel symbol that it api.exports, so that other packages (like minifier-js) can call it.

  2. Code that UglifyJS is failing to minify is not likely to be using dynamic import(...), because it's either code that we've already compiled with babel-compiler, or it's something that was published to npm, so it's probably just CommonJS require and exports. In other words, the only ES6 code we'll be minifying is what works natively in Node 4.

@PEM--
Copy link

@PEM-- PEM-- commented Feb 18, 2017

@sethmurphy18 Awesome! 👍

Could you also expose some configuration capabilities?

  • Currently, the list of files provided to the minifier includes stubs that only relate to the server (shell-server, for instance) . This adds some unnecessary bytes. A configurable list of files provides the capability to users to remove what they want in the bundle at the file level.
  • Code encapsulated in Meteor.isServer is bundled within the client code which add security risks as well a unnecessary bytes. Having the capability the treat this as dead code in a configurable list would allow users to get rid of server code and even remove part of their sources that are only related to development. This would provides code removal within the file level.

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

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 18, 2017

@benjamn Got you, I will update the babel-compiler package as well then. Also, I will remove the dynamic import plugin from the minifier since it is not needed. Hopefully, Babili is smart enough to not worry about require statements.

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 18, 2017

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 ms result will probably be in the 10's of thousands. But hey, meteor runs on it, albeit slowly.

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 19, 2017

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.

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 19, 2017

@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 settings.json:

{
  "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 server/[whatever].js:

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 settings.json method because then I would have to figure out a way to dynamically install npm plugins which would just not be fun. Also, the only settings you can really adjust with Babili is the plugins, not sure how many bytes that will save you.

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 19, 2017

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.

@PEM--
Copy link

@PEM-- PEM-- commented Feb 19, 2017

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.

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 19, 2017

@PEM-- The 2 main things I am concerned with is aggressive mode and forceMinificationInDevelopment. I am not so sure those two things line up with what MDG would want. Also, I am not 100% sure about the debug mode, I might have to remove that. Other than those things, I think this was a good idea, and I wanted to implement something like this to start with. Plus I am glad to convert it to an ES6 class because the other setup was so node 0.1 and very clunky.

@PEM--
Copy link

@PEM-- PEM-- commented Feb 19, 2017

The aggressive mode provides more mangling and dead code elimination capabilities but put far more pressure on RAM as all sources are loaded in memory. On some big apps, it may fails.

The debug mode allows users to check wether the code elimination has worked as expected. It's a bit verbose but quite handy. Going a bit further and display a diff of the code removed would make it really neat.

The forceMinificationInDevelopment goes with the dead code elimination as it may suppress behavior while development. Not that useful for the vast majority of apps, I must say.

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.

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 19, 2017

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.

@PEM--
Copy link

@PEM-- PEM-- commented Feb 19, 2017

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 😄

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 19, 2017

Oh yeah, for sure.

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 22, 2017

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 meteor-babel has to get its own options.

Testing: backbone@1.2.3
┏━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━┓
┃  minifier               ┃  raw      ┃  raw win  ┃  gzip    ┃  gzip win  ┃  parse time  ┃  run time (average)  ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  uglify                 ┃  21.68kB  ┃  222%     ┃  7.26kB  ┃  170%      ┃  4ms         ┃  754ms               ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  closure                ┃  21.57kB  ┃  223%     ┃  7.33kB  ┃  168%      ┃  5ms         ┃  19398ms             ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  uglify (meteor-babel)  ┃  22.26kB  ┃  213%     ┃  7.41kB  ┃  165%      ┃  5ms         ┃  669ms               ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  babili                 ┃  21.81kB  ┃  220%     ┃  7.44kB  ┃  163%      ┃  5ms         ┃  4102ms              ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  babili (meteor-babel)  ┃  22.39kB  ┃  212%     ┃  7.59kB  ┃  158%      ┃  5ms         ┃  2917ms              ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  closure js             ┃  23.92kB  ┃  192%     ┃  8kB     ┃  145%      ┃  5ms         ┃  10119ms             ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━┻━━━━━━━━━━━┻━━━━━━━━━━━┻━━━━━━━━━━┻━━━━━━━━━━━━┻━━━━━━━━━━━━━━┻━━━━━━━━━━━━━━━━━━━━━━┛


Testing: react@0.14.3 (react/dist/react.js)
┏━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━┓
┃  minifier               ┃  raw       ┃  raw win  ┃  gzip     ┃  gzip win  ┃  parse time  ┃  run time (average)  ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  closure                ┃  171.46kB  ┃  265%     ┃  52.97kB  ┃  168%      ┃  42ms        ┃  45809ms             ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  uglify                 ┃  176.36kB  ┃  255%     ┃  53.13kB  ┃  167%      ┃  40ms        ┃  3899ms              ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  uglify (meteor-babel)  ┃  180.44kB  ┃  247%     ┃  53.71kB  ┃  164%      ┃  36ms        ┃  6480ms              ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  babili                 ┃  177.11kB  ┃  254%     ┃  55.2kB   ┃  157%      ┃  36ms        ┃  19770ms             ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  babili (meteor-babel)  ┃  181.21kB  ┃  246%     ┃  55.86kB  ┃  154%      ┃  36ms        ┃  19903ms             ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  closure js             ┃  312.64kB  ┃  100%     ┃  70.86kB  ┃  100%      ┃  40ms        ┃  7575ms              ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━┻━━━━━━━━━━━━┻━━━━━━━━━━━┻━━━━━━━━━━━┻━━━━━━━━━━━━┻━━━━━━━━━━━━━━┻━━━━━━━━━━━━━━━━━━━━━━┛


Testing: jquery@1.11.3
┏━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━┓
┃  minifier               ┃  raw       ┃  raw win  ┃  gzip     ┃  gzip win  ┃  parse time  ┃  run time (average)  ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  uglify                 ┃  94.27kB   ┃  195%     ┃  32.78kB  ┃  153%      ┃  26ms        ┃  3057ms              ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  uglify (meteor-babel)  ┃  97.11kB   ┃  186%     ┃  33.07kB  ┃  151%      ┃  25ms        ┃  5444ms              ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  closure                ┃  94.14kB   ┃  195%     ┃  33.38kB  ┃  148%      ┃  22ms        ┃  38361ms             ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  closure js             ┃  96.11kB   ┃  189%     ┃  34.07kB  ┃  143%      ┃  22ms        ┃  27676ms             ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  babili                 ┃  102.79kB  ┃  170%     ┃  35.22kB  ┃  136%      ┃  24ms        ┃  19775ms             ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  babili (meteor-babel)  ┃  105.66kB  ┃  163%     ┃  35.62kB  ┃  133%      ┃  26ms        ┃  19630ms             ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━┻━━━━━━━━━━━━┻━━━━━━━━━━━┻━━━━━━━━━━━┻━━━━━━━━━━━━┻━━━━━━━━━━━━━━┻━━━━━━━━━━━━━━━━━━━━━━┛


Testing: three@0.82.1 (three/build/three.js)
┏━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━┓
┃  minifier               ┃  raw       ┃  raw win  ┃  gzip      ┃  gzip win  ┃  parse time  ┃  run time (average)  ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  closure                ┃  472.57kB  ┃  107%     ┃  122.22kB  ┃  61%       ┃  135ms       ┃  64317ms             ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  uglify                 ┃  478.79kB  ┃  104%     ┃  122.53kB  ┃  61%       ┃  95ms        ┃  8200ms              ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  uglify (meteor-babel)  ┃  483.08kB  ┃  103%     ┃  123.06kB  ┃  60%       ┃  99ms        ┃  35968ms             ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  closure js             ┃  480.15kB  ┃  104%     ┃  123.38kB  ┃  60%       ┃  89ms        ┃  136850ms            ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  babili                 ┃  506.85kB  ┃  93%      ┃  128.18kB  ┃  54%       ┃  96ms        ┃  61829ms             ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━╋━━━━━━━━━━━━━━╋━━━━━━━━━━━━━━━━━━━━━━┫
┃  babili (meteor-babel)  ┃  510.77kB  ┃  92%      ┃  128.68kB  ┃  53%       ┃  113ms       ┃  64018ms             ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━┻━━━━━━━━━━━━┻━━━━━━━━━━━┻━━━━━━━━━━━━┻━━━━━━━━━━━━┻━━━━━━━━━━━━━━┻━━━━━━━━━━━━━━━━━━━━━━┛
@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 22, 2017

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.

@krem06
Copy link

@krem06 krem06 commented Feb 22, 2017

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 !

@mitar
Copy link
Collaborator

@mitar mitar commented Feb 22, 2017

@abernix: We could for now support configuration through addFiles options. And then later on we just find a way for global configuration which is passed to all "implicit" addFiles.

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 23, 2017

@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.

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 23, 2017

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.

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 23, 2017

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).

@abernix
Copy link
Member

@abernix abernix commented Feb 23, 2017

If you wanted to try, that'd be interesting to see. The official test262 test is obviously the most complete, but you'll want to look at https://github.com/bterlson/test262-harness to utilize it probably! That being said, you'll probably want to look at babel/babel#4987 first. 😉

@abernix
Copy link
Member

@abernix abernix commented Feb 23, 2017

@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.

@mitar
Copy link
Collaborator

@mitar mitar commented Feb 23, 2017

@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

I have done so. :-)

We have done a similar thing for autoprefixer.

@abernix
Copy link
Member

@abernix abernix commented Feb 23, 2017

@mitar

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 addFiles within packages? Can you provide a link to what you did with autoprefixer?

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 23, 2017

That would actually be an excellent idea if that is what he is suggesting, at least I think @mitar is a he.

@mitar
Copy link
Collaborator

@mitar mitar commented Feb 23, 2017

I mean, it is really easy to add options to addFiles: https://github.com/meteor/meteor/pull/7727/files

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.

@abernix
Copy link
Member

@abernix abernix commented Feb 23, 2017

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!

@lynchem
Copy link

@lynchem lynchem commented Feb 24, 2017

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 ?

@abernix
Copy link
Member

@abernix abernix commented Feb 24, 2017

@lynchem Not any easier with a new minifier, no; the old minifier supported generating source maps as well. Keep following the other issue!

@mwarren2
Copy link

@mwarren2 mwarren2 commented Feb 27, 2017

If this all works, @sethmurphy18 deserves at the very least a new toaster

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Feb 27, 2017

@sethmurphy18 would not complain about anything...lol.

@mwarren2
Copy link

@mwarren2 mwarren2 commented Mar 9, 2017

It seemed that this was near to completion.
I'd be very grateful for any news on what's happening.
Thanks.

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Mar 10, 2017

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 meteor-babel package published before proceeding. Other than a new feature request, I believe that all of the issues regarding changing the minifier have been resolved and pull #8397 should be merged soon, unless there are any other issues I am not aware of. Sorry to keep everyone in the dark, between work, school, my personal projects, and this, I have been swamped.

@abernix
Copy link
Member

@abernix abernix commented Mar 10, 2017

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!

@abernix
Copy link
Member

@abernix abernix commented Mar 16, 2017

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:

Thanks so much for making this happen @sethmurphy18! I think it's going to solve a lot of obscure production build problems for Meteor developers.

@eagerestwolf
Copy link

@eagerestwolf eagerestwolf commented Mar 21, 2017

No problem guys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.