Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Added experimental ngmin implementation for AngularJS minification pr…

…eperation. Relates to #38
  • Loading branch information...
commit d6a411f57a43e29f427bb99c9464dc4fa49f0f86 1 parent 05bbccb
@Munter authored
View
61 lib/transforms/angularPreMinification.js
@@ -0,0 +1,61 @@
+var ngmin = require('ngmin'),
+ seq = require('seq');
+
+module.exports = function () {
+ return function angularPreMinification(assetGraph, cb) {
+ var query = assetGraph.query,
+ htmlAssets = [],
+ isAngular = false,
+ ngminRenderer = function (javaScript, cb) {
+ process.nextTick(function () {
+ cb(null, new assetGraph.JavaScript({
+ text: ngmin.annotate(javaScript.text)

Since this is a sync function you can completely do away with the seq and process.nextTick cruft by turning this into a synchronous transform. Just omit the cb parameter from the angularPreMinification function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ }));
+ });
+ };
+
+ assetGraph.findAssets({
+ isHtml: true,
+ isInitial: true,

I've tried to avoid using isInitial in the low-level transforms themselves. Would be more "idiomatic AssetGraph" (heh) to let the function on line 4 accept a queryObj parameter and use that when finding the assets that might need processing in line 17. That would move the isInitial: true to the buildProduction transform.

I usually make the queryObj parameter optional by using a construct like this: assetGraph.findAssets(_.extend({isHtml: true, isInline: false}, queryObj)).forEach.... Underscore's extend just returns its first parameter if the second one is undefined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ isInline: false
+ }).forEach(function (htmlAsset) {
+ var relationsToAsset = assetGraph.findRelations({
+ to: htmlAsset,
+ type: query.not('HtmlAnchor')
+ });
+
+ if (relationsToAsset.length === 0) {

Why would the existence of an incoming non-HtmlAnchor relation mean that it's not an Angular.js app? What about HtmlFrame and HtmlIFrame relations?

If you meant to exclude AngularJsTemplate assets, change isHtml: true to type: 'Html'.

@Munter Owner
Munter added a note

AngularJsTemplate seems to be of type Html as well. At least when I query for {type: 'Html', isInline: false} I also get an external AngularJsTemplate.
Trying to run htmlAsset.parseTree.querySelector on it fails. I was assuming since it is not a valid html document.

Hmm... That's unfortunate. We have to make sure that all Angular.js templates become AngularJsTemplate assets rather than Html. Otherwise outgoing relations with relative hrefs will resolve wrong when the main HTML and the template reside in different directories.

Looks the assets are being instantiated correctly when they're discovered via outgoing relalations from JavaScript:

https://github.com/One-com/assetgraph/blob/master/lib/assets/JavaScript.js#L182
https://github.com/One-com/assetgraph/blob/master/lib/assets/JavaScript.js#L191
https://github.com/One-com/assetgraph/blob/master/lib/assets/JavaScript.js#L213

Is this happening because the assets have been loaded explicitly with the loadAssets transform via a **/*.html glob or something? At that time it would be indistinguishable from a "regular" Html asset and would become an Html asset based on the file extension. In that case we need to work something out. Damn overloaded file extensions :)

@Munter Owner
Munter added a note

The globbing is exactly what is happening. I had a suspicion that we would need to do more about those templates when we realize that they are templates and not html pages.

I prefer this to work with globbing, since that is what seems to be the industry standard, at least with grunt, to define what files should be processed.

It's just angulars convention of naming their htm ltemplates .html that gets in the way. But this is an issue we will eventually run into with other templating systems with a similar convention.

I suggest that we somehow replace the template asset registered as an initial html page with a new template asset when we realize that it is one in the lines you linked to

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ htmlAssets.push(htmlAsset);
+ }
+ });
+
+ htmlAssets.forEach(function (htmlAsset) {
+ var document = htmlAsset.parseTree;
+
+ if (document.documentElement && document.querySelector('[ng-app]')
+ || document.querySelector('[class~="ng-app:"]')) {
+ isAngular = true;
+ }
+ });
+
+ if (isAngular) {
+ console.warn('Angular');
+ seq(assetGraph.findAssets({type: 'JavaScript'}))

In the event that the graph contains other things, it would make sense to only process the assets that are reachable via relations from the Angular.js Html asset:

assetGraph.eachAssetPostOrder(htmlAsset, {type: query.not('HtmlAnchor')}, function (asset) {
    if (asset.type === 'JavaScript') {
        // ...
    }
});
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ .parEach(function (javaScript) {
+ ngminRenderer(javaScript, this.into(javaScript.id));
+ })
+ .parEach(function (javaScript) {
+ var newAsset = this.vars[javaScript.id];
+ newAsset.minify(); // FIXME: This smells

Yeah, would probably make more sense to switch things around in buildProduction so that the minification happens after angularPreMinification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ javaScript.replaceWith(newAsset);
+ this()
+ })
+ .seq(function () {
+ cb();
+ })
+ ['catch'](cb);
+ } else {
+ cb();
+ }
+ };
+};
View
1  lib/transforms/buildProduction.js
@@ -69,6 +69,7 @@ module.exports = function (options) {
.removeAssets({type: 'I18n'}, true)
.removeAssets({isLoaded: true, isEmpty: true, type: ['Css', 'JavaScript']}, true)
.if(!options.noCompress)
+ .angularPreMinification()
.compressJavaScript({type: 'JavaScript', isLoaded: true}, 'uglifyJs', {toplevel: options.mangleTopLevel, defines: options.defines})
.endif()
.inlineRelations({
View
1  package.json
@@ -18,6 +18,7 @@
"jpegtran": "=0.0.4",
"less": "=1.3.0",
"memoizer": "=0.0.4",
+ "ngmin": "=0.0.4",
"optimist": "=0.2.6",
"optipng": "=0.0.4",
"passerror": "=0.0.1",

1 comment on commit d6a411f

@Munter
Owner

Refactored in 99b1ff6

Please sign in to comment.
Something went wrong with that request. Please try again.