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.

Sign up for GitHub

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

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

Merged
merged 6 commits into from Mar 10, 2017
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b81fe99
Updated package.js for templating-tools and spacebars-compiler
Feb 26, 2017
20d450c
Updated package.js
Feb 26, 2017
a8dbe35
Updated package.js
Feb 26, 2017
83c4ea7
Removed minifier-js dependency from templating-tools
Mar 8, 2017
7472c55
Updated HISTORY.md
Mar 10, 2017
f4b2663
Removed comment about pulling uglify-js from NPM
Mar 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions packages/spacebars-compiler/package.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ Package.describe({
git: 'https://github.com/meteor/blaze.git'
});

// Use uglify-js from NPM
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping for this as well.

Npm.depends({
'uglify-js': '2.7.5'
});

Package.onUse(function (api) {
api.use('underscore@1.0.9');

Expand All @@ -21,11 +26,6 @@ Package.onUse(function (api) {
'codegen.js',
'compiler.js'
]);

// Pull in uglify-js from NPM
Npm.depends({
'uglify-js': '2.7.5'
});
});

Package.onTest(function (api) {
Expand All @@ -47,8 +47,4 @@ Package.onTest(function (api) {
'compile_tests.js',
'compiler_output_tests.coffee'
]);

Npm.depends({
'uglify-js': '2.7.5'
})
});
2 changes: 1 addition & 1 deletion packages/templating-tools/package.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abernix, is this good?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@eagerestwolf eagerestwolf Feb 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@eagerestwolf eagerestwolf Feb 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

]);

api.export('TemplatingTools');
Expand Down