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

Merged
merged 6 commits into from Mar 10, 2017

Conversation

Projects
None yet
3 participants
@sethmurphy18
Contributor

sethmurphy18 commented Feb 26, 2017

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.

@sethmurphy18

This comment has been minimized.

Show comment
Hide comment
@sethmurphy18

sethmurphy18 Feb 26, 2017

Contributor

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

Contributor

sethmurphy18 commented Feb 26, 2017

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

packages/spacebars-compiler/package.js
@@ -5,6 +5,11 @@ Package.describe({
git: 'https://github.com/meteor/blaze.git'
});
+// Use uglify-js from NPM

This comment has been minimized.

@mitar

mitar Feb 27, 2017

Collaborator

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

@mitar

mitar Feb 27, 2017

Collaborator

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

This comment has been minimized.

@sethmurphy18

sethmurphy18 Feb 27, 2017

Contributor

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

@sethmurphy18

sethmurphy18 Feb 27, 2017

Contributor

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

This comment has been minimized.

@mitar

mitar Mar 10, 2017

Collaborator

Ping for this as well.

@mitar

mitar Mar 10, 2017

Collaborator

Ping for this as well.

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'

This comment has been minimized.

@mitar

mitar Feb 27, 2017

Collaborator

@abernix, is this good?

@mitar

mitar Feb 27, 2017

Collaborator

@abernix, is this good?

This comment has been minimized.

@abernix

abernix Feb 27, 2017

Member

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

abernix Feb 27, 2017

Member

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?

This comment has been minimized.

@sethmurphy18

sethmurphy18 Feb 27, 2017

Contributor

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

@sethmurphy18

sethmurphy18 Feb 27, 2017

Contributor

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

This comment has been minimized.

@sethmurphy18

sethmurphy18 Feb 27, 2017

Contributor

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.

@sethmurphy18

sethmurphy18 Feb 27, 2017

Contributor

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.

This comment has been minimized.

@sethmurphy18

sethmurphy18 Feb 27, 2017

Contributor

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.

@sethmurphy18

sethmurphy18 Feb 27, 2017

Contributor

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.

This comment has been minimized.

@sethmurphy18

sethmurphy18 Feb 27, 2017

Contributor

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

@sethmurphy18

sethmurphy18 Feb 27, 2017

Contributor

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

This comment has been minimized.

@sethmurphy18

sethmurphy18 Feb 27, 2017

Contributor

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.

@sethmurphy18

sethmurphy18 Feb 27, 2017

Contributor

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.

This comment has been minimized.

@sethmurphy18

sethmurphy18 Feb 27, 2017

Contributor

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

@sethmurphy18

sethmurphy18 Feb 27, 2017

Contributor

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

This comment has been minimized.

@sethmurphy18

sethmurphy18 Feb 27, 2017

Contributor

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.

@sethmurphy18

sethmurphy18 Feb 27, 2017

Contributor

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.

@sethmurphy18

This comment has been minimized.

Show comment
Hide comment
@sethmurphy18

sethmurphy18 Feb 27, 2017

Contributor

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.

Contributor

sethmurphy18 commented Feb 27, 2017

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.

@sethmurphy18

This comment has been minimized.

Show comment
Hide comment
@sethmurphy18

sethmurphy18 Feb 28, 2017

Contributor

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

Contributor

sethmurphy18 commented Feb 28, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@mitar

mitar Mar 8, 2017

Collaborator

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.

Collaborator

mitar commented Mar 8, 2017

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.

@sethmurphy18

This comment has been minimized.

Show comment
Hide comment
@sethmurphy18

sethmurphy18 Mar 8, 2017

Contributor

Sounds like a plan. Nixed it will be.

Contributor

sethmurphy18 commented Mar 8, 2017

Sounds like a plan. Nixed it will be.

@sethmurphy18

This comment has been minimized.

Show comment
Hide comment
@sethmurphy18

sethmurphy18 Mar 10, 2017

Contributor

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

Contributor

sethmurphy18 commented Mar 10, 2017

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

@mitar

This comment has been minimized.

Show comment
Hide comment
@mitar

mitar Mar 10, 2017

Collaborator

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

Collaborator

mitar commented Mar 10, 2017

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

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

1 of 2 checks passed

ci/circleci CircleCI is running your tests
Details
CLA Author has signed the Meteor CLA.
Details
@sethmurphy18

This comment has been minimized.

Show comment
Hide comment
@sethmurphy18

sethmurphy18 Mar 10, 2017

Contributor

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

Contributor

sethmurphy18 commented Mar 10, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment