Updated package.js for templating-tools and spacebars-compiler #236

eagerestwolf · 2017-02-26T17:46:09Z

Made changes suggested by @abernix and @mitar. Also updated package.js for templating-tools to accept the new version of minifier-js, since the package never uses the API directly.

eagerestwolf · 2017-02-26T18:59:30Z

Alirght, @mitar all done, PR is done and ready to merge.

mitar · 2017-02-27T02:39:12Z

packages/spacebars-compiler/package.js

@@ -5,6 +5,11 @@ Package.describe({
  git: 'https://github.com/meteor/blaze.git'
 });

+// Use uglify-js from NPM


I mean, this comment is unnecessary. It is pretty clear that this is what this code does.

I will remove it and commit once we figure out the second issue here.

Ping for this as well.

mitar · 2017-02-27T02:39:29Z

packages/templating-tools/package.js

@@ -16,7 +16,7 @@ Package.onUse(function(api) {
    // boilerplate-generator uses spacebars-compiler.)
    // XXX maybe uglify should be applied by this plugin instead of via magic
    // weak dependency.
-    'minifier-js@1.2.14'
+    'minifier-js@1.2.14 || 2.0.0'


@abernix, is this good?

Hmm, this does seem to be defeating the purpose of the original change. Did I miss the discussion leading to this change somewhere?

@sethmurphy18 The original intention of #234 was to get this change included because the new minifier would actually minify in a different way.

Looks like that was reverted with 5594d80 and that possibly the only reason that Blaze is passing this is because we're explicitly pinning it to the current 1.2.14 version via this line (2.0.0 is not published yet)?

I understand the changes to spacebars-compiler in order to get _beautify working and I think the direct dependency on the uglify-js NPM is fine for that, but what happened to the templating-tools change to compensate for the Babili minification? Don't we still need that when the official minifier-js (the 2.0.0 in this version constraint) is published?

@abernix This issue lies in Meteor will only install 1 version of each package at a time. Since we are bumping the version of minifier-js to 2.0.0, meteor will fail to build because there is no package installed that matches those contraints because @1.2.14 means anything 1.x.y where x is >= 2 and y >= 14, but that first number has to be 1. The new constraints I added, say x.y.z, where x = 1 or 2, if x is 1 refer to above. If x is 2, y and z must be >= 0. Ideally, we would just leave the version off, but according to the docs that is bad practice.

To be 100% honest, I am not sure why the package is even included. The minifier isn't accessed anywhere in the code. I am not sure if isobuild will minify code from something just because it uses a minifier plugin.

I just finished running a search through the entire Blaze code, the minify function from the original minifier-js package isn't used anywhere. The reason the tests were failing originally is because the beautification part wasn't there, which was an issue with spacebars-compiler. The reason that templating-tools was failing is because while spacebars-compiler had failsafes in place in case it wasn't there, templating-tools did not. In my opinion that was the only reason the package was included as a strong dependency in templating-tools to begin with. It had nothing to do with minification, it was beautification.

Search results for validation:

Searching 11681 files for "UglifyJSMinify" /home/seth/Projects/blaze/packages/spacebars-compiler/compile_tests.js: 21 // remove initial and trailing parens 22 string = string.replace(/^$([\S\s]*)$$/, '$1'); 23: if (! (Package['minifier-js'] && Package['minifier-js'].UglifyJSMinify)) { 24 // these tests work a lot better with access to beautification, 25 // but let's at least do some sort of test without it. /home/seth/Projects/blaze/packages/spacebars-compiler/compiler.js: 1: var UglifyJSMinify = Npm.require('uglify-js').minify; 2 3 SpacebarsCompiler.parse = function (input) { . 97 98 SpacebarsCompiler._beautify = function (code) { 99: var result = UglifyJSMinify(code, { 100 fromString: true, 101 mangle: false, Searching 11681 files for "minifier-js" /home/seth/Projects/blaze/packages/blaze-html-templates/.versions: 18 jquery@1.11.9 19 meteor@1.6.0 20: minifier-js@1.2.14 21 modules@0.7.6 22 modules-runtime@0.7.6 /home/seth/Projects/blaze/packages/blaze/.versions: 28 logging@1.0.14 29 meteor@1.6.0 30: minifier-js@1.2.14 31 minimongo@1.0.17 32 modules@0.7.6 /home/seth/Projects/blaze/packages/caching-html-compiler/.versions: 10 htmljs@1.0.11 11 meteor@1.2.17 12: minifier-js@1.2.14 13 modules@0.7.6 14 modules-runtime@0.7.6 /home/seth/Projects/blaze/packages/spacebars-compiler/compile_tests.js: 21 // remove initial and trailing parens 22 string = string.replace(/^$([\S\s]*)$$/, '$1'); 23: if (! (Package['minifier-js'] && Package['minifier-js'].UglifyJSMinify)) { 24 // these tests work a lot better with access to beautification, 25 // but let's at least do some sort of test without it. /home/seth/Projects/blaze/packages/spacebars-tests/.versions: 30 markdown@1.0.10 31 meteor@1.6.0 32: minifier-js@1.2.14 33 minimongo@1.0.17 34 modules@0.7.6 /home/seth/Projects/blaze/packages/static-html/.versions: 10 htmljs@1.0.11 11 meteor@1.2.17 12: minifier-js@1.2.14 13 modules@0.7.6 14 modules-runtime@0.7.6 /home/seth/Projects/blaze/packages/templating-compiler/.versions: 10 htmljs@1.0.11 11 meteor@1.2.17 12: minifier-js@1.2.14 13 modules@0.7.6 14 modules-runtime@0.7.6 /home/seth/Projects/blaze/packages/templating-compiler/package.js: 9 Package.registerBuildPlugin({ 10 name: "compileTemplatesBatch", 11: // minifier-js is a weak dependency of spacebars-compiler; adding it here 12 // ensures that the output is minified. (Having it as a weak dependency means 13 // that we don't ship uglify etc with built apps just because /home/seth/Projects/blaze/packages/templating-runtime/.versions: 28 logging@1.0.14 29 meteor@1.2.17 30: minifier-js@1.2.14 31 minimongo@1.0.17 32 modules@0.7.6 /home/seth/Projects/blaze/packages/templating-tools/.versions: 26 logging@1.0.14 27 meteor@1.2.17 28: minifier-js@1.2.14 29 minimongo@1.0.17 30 modules@0.7.6 /home/seth/Projects/blaze/packages/templating-tools/package.js: 11 'ecmascript@0.5.8', 12 13: // minifier-js is a weak dependency of spacebars-compiler; adding it here 14 // ensures that the output is minified. (Having it as a weak dependency means 15 // that we don't ship uglify etc with built apps just because .. 17 // XXX maybe uglify should be applied by this plugin instead of via magic 18 // weak dependency. 19: 'minifier-js@1.2.14 || 2.0.0' 20 ]); 21 /home/seth/Projects/blaze/packages/templating/.versions: 17 jquery@1.11.9 18 meteor@1.2.17 19: minifier-js@1.2.14 20 modules@0.7.6 21 modules-runtime@0.7.6 14 matches across 12 files

Ok, I tried removing it and commited to a test branch and I am running a Circle build now to see if everything passes or fails.

All of the tests passed even without that package. I am building meteor now to see if I get errors over there.

Ok, no errors on the meteor side, except blaze (still) and Mongo oplog, which sometimes passes sometimes fails (I think that one is related to my PC), and DDP which got a timeout instead of a rate-limit, which again I think is my PC. spacebars-compiler and templating-tools both pass, though, on client and server, which means to me that either minifier-js is not needed by templating-tools or it's not tested. Another thing of note is that everything sent to the client is concatenated and minified by default, courtesy of isobuild, so long as you aren't in development mode, so test and production modes will be minified by default.

eagerestwolf · 2017-02-27T14:37:55Z

I'm also going to bump the revision of spacebars-compiler since I made a change that didn't affect the API. I forgot to do that on my last PR.

eagerestwolf · 2017-02-28T17:49:56Z

@abernix and @mitar what are both of your opinions on this PR? Should we keep minifier-js since it is doing nothing, or should we nix it?

mitar · 2017-03-08T22:49:29Z

I think I would just remove dependency on minifier-js@1.2.14 from templating-tools package and document in the History.md that one should decide on minifying by yourself for your app by adding it to the Meteor app. I think this whole dependency from templating-tools to force minimization was done before all this changes to minification becoming its own build step and stuff like this.

eagerestwolf · 2017-03-08T22:50:30Z

Sounds like a plan. Nixed it will be.

eagerestwolf · 2017-03-10T00:53:19Z

Added history entry under vNEXT since this version is not yet released.

mitar · 2017-03-10T08:18:21Z

Cool I think this looks good. I will change text in history a bit after merging, but it looks otherwise very good. Thanks!

eagerestwolf · 2017-03-10T08:20:48Z

No problem. Sorry about that comment. Forgot to remove it, thanks for pointing it out though. Push new commit. Should be building now.

Seth Murphy added 3 commits February 26, 2017 12:42

Updated package.js for templating-tools and spacebars-compiler

b81fe99

Updated package.js

20d450c

Updated package.js

a8dbe35

mitar reviewed Feb 27, 2017

View reviewed changes

mitar added the pr missing history entry label Mar 8, 2017

Seth Murphy added 2 commits March 8, 2017 18:03

Removed minifier-js dependency from templating-tools

83c4ea7

Updated HISTORY.md

7472c55

Removed comment about pulling uglify-js from NPM

f4b2663

mitar merged commit 8537e86 into meteor:master Mar 10, 2017

mitar removed the pr missing history entry label Mar 10, 2017

This was referenced Mar 13, 2017

Updated standard-minifier-js to use Babili from meteor-babel meteor/meteor#8397

Merged

Remove minifier-js version constraints from all .versions files #242

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated package.js for templating-tools and spacebars-compiler #236

Updated package.js for templating-tools and spacebars-compiler #236

eagerestwolf commented Feb 26, 2017

eagerestwolf commented Feb 26, 2017

mitar Feb 27, 2017

eagerestwolf Feb 27, 2017

mitar Mar 10, 2017

mitar Feb 27, 2017

abernix Feb 27, 2017

eagerestwolf Feb 27, 2017 •

edited

Loading

eagerestwolf Feb 27, 2017

eagerestwolf Feb 27, 2017

eagerestwolf Feb 27, 2017 •

edited

Loading

eagerestwolf Feb 27, 2017

eagerestwolf Feb 27, 2017

eagerestwolf Feb 27, 2017

eagerestwolf commented Feb 27, 2017 •

edited

Loading

eagerestwolf commented Feb 28, 2017

mitar commented Mar 8, 2017

eagerestwolf commented Mar 8, 2017

eagerestwolf commented Mar 10, 2017

mitar commented Mar 10, 2017

eagerestwolf commented Mar 10, 2017

Updated package.js for templating-tools and spacebars-compiler #236

Updated package.js for templating-tools and spacebars-compiler #236

Conversation

eagerestwolf commented Feb 26, 2017

eagerestwolf commented Feb 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eagerestwolf Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eagerestwolf Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eagerestwolf commented Feb 27, 2017 • edited Loading

eagerestwolf commented Feb 28, 2017

mitar commented Mar 8, 2017

eagerestwolf commented Mar 8, 2017

eagerestwolf commented Mar 10, 2017

mitar commented Mar 10, 2017

eagerestwolf commented Mar 10, 2017

eagerestwolf Feb 27, 2017 •

edited

Loading

eagerestwolf Feb 27, 2017 •

edited

Loading

eagerestwolf commented Feb 27, 2017 •

edited

Loading