Merge lp:~tveronezi/juju-gui/uglify into lp:juju-gui/experimental

[Thiago Veronezi]

Resource minification/aggregation js

* We should aggregate and minimize the js sources in order to improve the load speed of the application;
* We dont want to use the yui combo loader feature;
* The final product will provide three js files: one for the YUI dependencies, one for our custom js code and one for third part js like d3.

https://codereview.appspot.com/6821090/

[Thiago Veronezi (tveronezi)]

Reviewers: mp+133116_code.launchpad.net,

Message:
Please take a look.

Description:
Resource minification/aggregation js

* We should aggregate and minimize the js sources in order to improve
the load speed of the application;
* We dont want to use the yui combo loader feature;
* The final product will provide three js files: one for the YUI
dependencies, one for our custom js code and one for third part js like
d3.

https://code.launchpad.net/~tveronezi/juju-gui/uglify/+merge/133116

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6821090/

Affected files:
   M .bzrignore
   M Makefile
   A [revision details]
   M app/index.html
   A app/modules-debug.js
   M app/modules.js
   M grunt.js
   M lib/server.js
   A merge-files.js
   M package.json
   A properties-dev.js
   A properties-prod.js
   M test/index.html

[lp:~tveronezi/juju-gui/uglify]

233. By Thiago Veronezi on 2012-11-06

using yui loop functions

[Thiago Veronezi (tveronezi)]

https://codereview.appspot.com/6821090/diff/1/merge-files.js
File merge-files.js (right):

https://codereview.appspot.com/6821090/diff/1/merge-files.js#newcode25
merge-files.js:25: function loop(arr, callback) {
Please ignore this method. I am using the Y.each version of it.

https://codereview.appspot.com/6821090/

[Thiago Veronezi (tveronezi)]

https://codereview.appspot.com/6821090/diff/1/merge-files.js
File merge-files.js (right):

https://codereview.appspot.com/6821090/diff/1/merge-files.js#newcode148
merge-files.js:148:
I've already removed this blank line.

https://codereview.appspot.com/6821090/

[lp:~tveronezi/juju-gui/uglify]

234. By Thiago Veronezi on 2012-11-06

generated combined files should go under /app/Error: listen EADDRINUSE

[Thiago Veronezi (tveronezi)]

Please take a look.

https://codereview.appspot.com/6821090/

[lp:~tveronezi/juju-gui/uglify]

235. By Thiago Veronezi on 2012-11-06

the combine code should go under the combine directory

236. By Thiago Veronezi on 2012-11-06

the combine code should go under the combine directory

[Thiago Veronezi (tveronezi)]

Please take a look.

https://codereview.appspot.com/6821090/

[Thiago Veronezi (tveronezi)]

Thanks, Benji!

https://codereview.appspot.com/6821090/diff/7001/Makefile
File Makefile (right):

https://codereview.appspot.com/6821090/diff/7001/Makefile#newcode12
Makefile:12: node_modules/grunt node_modules/node-spritesheet
node_modules/node-minify
This line does not have more than 80 chars, but I changed it anyway.

On 2012/11/07 17:05:18, benji wrote:
> This line is too long.

https://codereview.appspot.com/6821090/diff/7001/Makefile#newcode75
Makefile:75: @cp app/assets/combine/properties-dev.js
app/assets/javascripts/generated/properties.js
On 2012/11/07 17:05:18, benji wrote:
> This line is too long.

Done.

https://codereview.appspot.com/6821090/diff/7001/Makefile#newcode80
Makefile:80: @cp app/assets/combine/properties-prod.js
app/assets/javascripts/generated/properties.js
On 2012/11/07 17:05:18, benji wrote:
> This line is too long.

Done.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js
File app/assets/combine/merge-files.js (right):

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode1
app/assets/combine/merge-files.js:1: 'use strict';
This is entirely ours.

On 2012/11/07 17:05:18, benji wrote:
> Is this code we wrote or was it imported from somewhere? If it was
written
> elsewhere, we need to specify its license and it would be nice to know
where it
> came from.

> I will forgo reviewing the code until I know the answers to the above.

https://codereview.appspot.com/6821090/diff/7001/app/index.html
File app/index.html (right):

https://codereview.appspot.com/6821090/diff/7001/app/index.html#newcode72
app/index.html:72: <script src="/source/yui.js"></script>
What about '/assets/yui.js'? Id this ok?

On 2012/11/07 17:05:18, benji wrote:
> We have been calling these things "assets". If the change to "source"
was a
> considered one, then I am fine with it. If not, we should continue to
use the
> same terminology that we have been using.

https://codereview.appspot.com/6821090/diff/7001/lib/server.js
File lib/server.js (right):

https://codereview.appspot.com/6821090/diff/7001/lib/server.js#newcode50
lib/server.js:50: server.get('/source/all-third.js', function(req, res)
{
I want to hide the real path.
I can change it to '/assets/all-third.js', would it be ok?

On 2012/11/07 17:05:18, benji wrote:
> Why should the path exposed via HTTP ("source") be different than the
path on
> disk ("assets")?

https://codereview.appspot.com/6821090/

[lp:~tveronezi/juju-gui/uglify]

237. By Thiago Veronezi on 2012-11-07

code review

[Gary Poster (gary)]

Hi Thiago. Thank you for this nice step forward: it will be great when
we have this functionality, and the debug/developer story you've
arranged is nicer than I expected.

This is a preliminary review. As I mention, I want to review
merge-files.js more later, but I'm going to let Benji take a first pass
and send my notes so far to you sooner.

Gary

https://codereview.appspot.com/6821090/diff/7001/Makefile
File Makefile (right):

https://codereview.appspot.com/6821090/diff/7001/Makefile#newcode64
Makefile:64: combinejs: $(NODE_TARGETS)
As I mentioned on IRC, this has the same problem we talked about with
sprite generation: every time you run the target it will try to compress
the files. Since install depends on this, and just about everything
depends on install, this can affect most make targets and be pretty
annoying.

One approach to solving this is http://pastebin.ubuntu.com/1340722/

https://codereview.appspot.com/6821090/diff/7001/Makefile#newcode89
Makefile:89: @rm -Rf app/assets/javascripts/generated/
Good.

https://codereview.appspot.com/6821090/diff/7001/Makefile#newcode107
Makefile:107: combinejs
Good.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js
File app/assets/combine/merge-files.js (right):

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode1
app/assets/combine/merge-files.js:1: 'use strict';
I don't think this file belongs in app/assets. This is a build
component, and we never want to serve it as part of the application. I
suggest that we put it in the top directory, or in bin. I lean towards
bin.

This is as good a time as any to mention that I think this branch is a
great example of something that could have probably benefitted from a
pre-implementation call (and maybe a prototyping).

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode3
app/assets/combine/merge-files.js:3: //
http://stackoverflow.com/questions/5348685/node-js-require-inheritance
Nice that you gave attribution. I personally don't think it is
necessary in this case. It was good to see as a reviewer, but it would
have been even better as part of your "cover letter" introduction to
your branch.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode6
app/assets/combine/merge-files.js:6: var Y = YUI(),
Why did you choose to do this rather than "YUI().use(..., function(Y)
{...});"?

You could even remove line 4. One approach:

require('yui').YUI().use(...);

another approach:

var Y = require('yui').YUI(),

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode27
app/assets/combine/merge-files.js:27: (function() {
I have a number of comments I already suspect I want to make about this
file--here, for instance, I think I will request that you add a lot more
comments, and possibly refactor, and possibly reconsider your approach
to compartmentalizing the various parts of the file. However, I
conferred with Benji, and I am going to be lazy and see what he has to
say first, and then follow along afterwards. That way I can get my
other comments to you faster.

https://code...

[Thiago Veronezi (tveronezi)]

Thanks Gary.

https://codereview.appspot.com/6821090/diff/7001/Makefile
File Makefile (right):

https://codereview.appspot.com/6821090/diff/7001/Makefile#newcode64
Makefile:64: combinejs: $(NODE_TARGETS)
On 2012/11/07 20:49:31, gary.poster wrote:
> As I mentioned on IRC, this has the same problem we talked about with
sprite
> generation: every time you run the target it will try to compress the
files.
> Since install depends on this, and just about everything depends on
install,
> this can affect most make targets and be pretty annoying.

> One approach to solving this is http://pastebin.ubuntu.com/1340722/

Done.

https://codereview.appspot.com/6821090/diff/7001/Makefile#newcode75
Makefile:75: @cp app/assets/combine/properties-dev.js
app/assets/javascripts/generated/properties.js
On 2012/11/07 17:05:18, benji wrote:
> This line is too long.

Done.

https://codereview.appspot.com/6821090/diff/7001/Makefile#newcode89
Makefile:89: @rm -Rf app/assets/javascripts/generated/
On 2012/11/07 20:49:31, gary.poster wrote:
> Good.

Done.

https://codereview.appspot.com/6821090/diff/7001/Makefile#newcode107
Makefile:107: combinejs
On 2012/11/07 20:49:31, gary.poster wrote:
> Good.

Done.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js
File app/assets/combine/merge-files.js (right):

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode1
app/assets/combine/merge-files.js:1: 'use strict';
On 2012/11/07 20:49:31, gary.poster wrote:
> I don't think this file belongs in app/assets. This is a build
component, and
> we never want to serve it as part of the application. I suggest that
we put it
> in the top directory, or in bin. I lean towards bin.

> This is as good a time as any to mention that I think this branch is a
great
> example of something that could have probably benefitted from a
> pre-implementation call (and maybe a prototyping).

This branch had a pre-implementation call. I moved this file here
because Ben asked me to do it.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode3
app/assets/combine/merge-files.js:3: //
http://stackoverflow.com/questions/5348685/node-js-require-inheritance
On 2012/11/07 20:49:31, gary.poster wrote:
> Nice that you gave attribution. I personally don't think it is
necessary in
> this case. It was good to see as a reviewer, but it would have been
even better
> as part of your "cover letter" introduction to your branch.

This is not part of the global solution. This is here just to explain
why I use the "global" object.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode6
app/assets/combine/merge-files.js:6: var Y = YUI(),
On 2012/11/07 20:49:31, gary.poster wrote:
> Why did you choose to do this rather than "YUI().use(..., function(Y)
{...});"?

> You could even remove line 4. One approach:

> require('yui').YUI().use(...);

> another approach:

> var Y = require('yui').YUI(),

I did it because I dont require any internal yui code. I use it to get
the Y.Object, Y.Array and Y.Loader objects.

I cannot remove the line 4. It is used by the line 82. When node
executes...

[lp:~tveronezi/juju-gui/uglify]

238. By Thiago Veronezi on 2012-11-07

code review

239. By Thiago Veronezi on 2012-11-07

trunk merge

240. By Thiago Veronezi on 2012-11-07

code review

241. By Thiago Veronezi on 2012-11-07

code review

[Thiago Veronezi (tveronezi)]

Tkx Benji.

https://codereview.appspot.com/6821090/diff/7001/Makefile
File Makefile (right):

https://codereview.appspot.com/6821090/diff/7001/Makefile#newcode12
Makefile:12: node_modules/grunt node_modules/node-spritesheet
node_modules/node-minify
On 2012/11/07 22:00:47, benji wrote:
> On 2012/11/07 18:59:19, thiago wrote:
> > This line does not have more than 80 chars, but I changed it anyway.

> Correct, it has fewer than 80 characters but one of those characters
is a tab,
> which in most editors displays as 8 spaces, pushing the line over 80
displayed
> characters.

Ok

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js
File app/assets/combine/merge-files.js (right):

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode1
app/assets/combine/merge-files.js:1: 'use strict';
On 2012/11/07 22:00:47, benji wrote:
> On 2012/11/07 18:59:19, thiago wrote:
> > This is entirely ours.

> Top-posting makes replying to comments difficult. Please reply below
the
> previous comment.

Ok

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode3
app/assets/combine/merge-files.js:3: //
http://stackoverflow.com/questions/5348685/node-js-require-inheritance
On 2012/11/07 22:00:47, benji wrote:
> Even after reading the page at the other end of this link I don't know
what it
> has to do with the code here. Additional comments would help.

Done.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode5
app/assets/combine/merge-files.js:5:
On 2012/11/07 22:00:47, benji wrote:
> I am surprised that we had to write this script. I would have
expected someone,
> somewhere to have already done this for us.

Not really. People usually use the comboloader for yui applications. I
had a lot of fun writing it though.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode14
app/assets/combine/merge-files.js:14: var execution = new
compressor.minify({
On 2012/11/07 22:00:47, benji wrote:
> object literal style

How should it be?

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode88
app/assets/combine/merge-files.js:88: var reqs = (function() {
On 2012/11/07 22:00:47, benji wrote:
> This function would benefit from a name.

This is not a function. This is an object: a list of all the yui
requirements we need to load.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode105
app/assets/combine/merge-files.js:105: // Using the example
http://yuilibrary.com/yui/docs/yui/loader-resolve.html
On 2012/11/07 22:00:47, benji wrote:
> I am not sure what this comment is trying to communicate. Is it that
I can find
> an explanation of the YUI loader mechanism at the given URL?

Nop. It uses the reqs object that we created in the previous block and
figures out the js files that contain it.

I will add it as comment here.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode111
app/assets/combine/merge-files.js:111: loader = new Y.Loader({
On 2012/11/07 22:00:47, benji wrote:
> object literal sty...

[lp:~tveronezi/juju-gui/uglify]

242. By Thiago Veronezi on 2012-11-07

making lint happy

[Thiago Veronezi (tveronezi)]

Please take a look.

https://codereview.appspot.com/6821090/

[lp:~tveronezi/juju-gui/uglify]

243. By Thiago Veronezi on 2012-11-07

missing commit

[Thiago Veronezi (tveronezi)]

https://codereview.appspot.com/6821090/diff/3002/Makefile
File Makefile (right):

https://codereview.appspot.com/6821090/diff/3002/Makefile#newcode12
Makefile:12: node_modules/grunt node_modules/node-spritesheet
node_modules/node-minify
Missing commit. You should see this line break in the next commit.

https://codereview.appspot.com/6821090/

[lp:~tveronezi/juju-gui/uglify]

244. By Thiago Veronezi on 2012-11-08

code review

245. By Thiago Veronezi on 2012-11-08

code review

246. By Thiago Veronezi on 2012-11-09

moving the functions to the lib directory

[Thiago Veronezi (tveronezi)]

Please take a look.

https://codereview.appspot.com/6821090/

[Thiago Veronezi (tveronezi)]

https://codereview.appspot.com/6821090/diff/15005/bin/merge-files
File bin/merge-files (right):

https://codereview.appspot.com/6821090/diff/15005/bin/merge-files#newcode20
bin/merge-files:20: global.YUI = require('yui').YUI;
Please ignore this entry. I moved it to the lib/merge-files.js file.

=== modified file 'bin/merge-files'
--- bin/merge-files 2012-11-09 12:54:56 +0000
+++ bin/merge-files 2012-11-09 13:00:19 +0000
@@ -14,12 +14,7 @@

  'use strict';

-// We need the yui name to be available in all modules (as a global
variable).
-// This only happens if we remove the 'var' keyword or add it to the
"global"
-// variable.
-global.YUI = require('yui').YUI;
-
-YUI().use(['yui'], function(Y) {
+require('yui').YUI().use(['yui'], function(Y) {
    var merge = require('../lib/merge-files.js'),
        syspath = require('path'),
        paths,

https://codereview.appspot.com/6821090/

[lp:~tveronezi/juju-gui/uglify]

247. By Thiago Veronezi on 2012-11-09

code review

[Gary Poster (gary)]

This is looking good, Thiago. Thank you!

As we discussed, I'm OK with holding off on tests. The last non-trivial
thing I'd like to see is removing the code that writes properties. I
give two possible approaches below.

I'm happy to have a call if it helps, or leave it to you, as you prefer.

Thanks!

Gary

https://codereview.appspot.com/6821090/diff/15005/Makefile
File Makefile (right):

https://codereview.appspot.com/6821090/diff/15005/Makefile#newcode82
Makefile:82: @./bin/write-properties true
I really would like to see this (and the corresponding line in 87 and
the corresponding "write-properties" script) go.

My preferred approach, which we talked about yesterday, would be to make
the server always have both the debug version (debug.html?) and the
production version (index.html) available, by having the server
dynamically change the js files that index.html points to.

Alternatively, server.js would accept an argument to turn on debug mode.
  By default, it would run in prodcution mode.

https://codereview.appspot.com/6821090/diff/15005/app/modules-debug.js
File app/modules-debug.js (right):

https://codereview.appspot.com/6821090/diff/15005/app/modules-debug.js#newcode1
app/modules-debug.js:1: GlobalConfig = {
An idea (as in, do this if you agree): it would be nice to have a
comment at the top of this file saying that it is only used for the
debug (developer) views of the application, and pointing to the
mechanism that is used (e.g., in server.js) to serve this file rather
than the other one.

Maybe it's overkill. I'd do it, but I'd be fine with accepting
disagreement.

https://codereview.appspot.com/6821090/diff/15005/app/modules.js
File app/modules.js (right):

https://codereview.appspot.com/6821090/diff/15005/app/modules.js#newcode4
app/modules.js:4: ignoreRegistered: true,
Is there a setting we can use to say "never download anything from the
internet, ever"? If so, I suggest we use that both for this and the
debug modules file.

Is it bootstrap: false? Or is this already handled somehow?

https://codereview.appspot.com/6821090/diff/15005/bin/merge-files
File bin/merge-files (right):

https://codereview.appspot.com/6821090/diff/15005/bin/merge-files#newcode1
bin/merge-files:1: #!/usr/bin/env node
These changes look great to me. Thanks.

https://codereview.appspot.com/6821090/diff/15005/bin/merge-files#newcode4
bin/merge-files:4: * We should aggregate and minimize the js sources in
order to improve the load
suggestion: delete "should". (We are doing it. :-) )

https://codereview.appspot.com/6821090/diff/15005/bin/merge-files#newcode8
bin/merge-files:8: * to run from only static files and we want to be
able to run behaind a
"behind" (I realize these are probably my fault. sorry :-) )

https://codereview.appspot.com/6821090/diff/15005/bin/write-properties
File bin/write-properties (right):

https://codereview.appspot.com/6821090/diff/15005/bin/write-properties#newcode1
bin/write-properties:1: #!/usr/bin/env node
As I said, I'd like to delete.

https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js
File lib/merge-files.js (right):

https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode14
lib...

[Thiago Veronezi (tveronezi)]

https://codereview.appspot.com/6821090/diff/15005/Makefile
File Makefile (right):

https://codereview.appspot.com/6821090/diff/15005/Makefile#newcode82
Makefile:82: @./bin/write-properties true
On 2012/11/09 13:57:38, gary.poster wrote:
> I really would like to see this (and the corresponding line in 87 and
the
> corresponding "write-properties" script) go.

> My preferred approach, which we talked about yesterday, would be to
make the
> server always have both the debug version (debug.html?) and the
production
> version (index.html) available, by having the server dynamically
change the js
> files that index.html points to.

> Alternatively, server.js would accept an argument to turn on debug
mode. By
> default, it would run in prodcution mode.

Using the server approach for now.
Thanks for this idea. Really good.

https://codereview.appspot.com/6821090/diff/15005/app/modules-debug.js
File app/modules-debug.js (right):

https://codereview.appspot.com/6821090/diff/15005/app/modules-debug.js#newcode1
app/modules-debug.js:1: GlobalConfig = {
On 2012/11/09 13:57:38, gary.poster wrote:
> An idea (as in, do this if you agree): it would be nice to have a
comment at the
> top of this file saying that it is only used for the debug (developer)
views of
> the application, and pointing to the mechanism that is used (e.g., in
server.js)
> to serve this file rather than the other one.

> Maybe it's overkill. I'd do it, but I'd be fine with accepting
disagreement.

Done.

https://codereview.appspot.com/6821090/diff/15005/bin/merge-files
File bin/merge-files (right):

https://codereview.appspot.com/6821090/diff/15005/bin/merge-files#newcode1
bin/merge-files:1: #!/usr/bin/env node
On 2012/11/09 13:57:38, gary.poster wrote:
> These changes look great to me. Thanks.

Done.

https://codereview.appspot.com/6821090/diff/15005/bin/merge-files#newcode4
bin/merge-files:4: * We should aggregate and minimize the js sources in
order to improve the load
On 2012/11/09 13:57:38, gary.poster wrote:
> suggestion: delete "should". (We are doing it. :-) )

Done.

https://codereview.appspot.com/6821090/diff/15005/bin/merge-files#newcode8
bin/merge-files:8: * to run from only static files and we want to be
able to run behaind a
On 2012/11/09 13:57:38, gary.poster wrote:
> "behind" (I realize these are probably my fault. sorry :-) )

Done.

https://codereview.appspot.com/6821090/diff/15005/bin/write-properties
File bin/write-properties (right):

https://codereview.appspot.com/6821090/diff/15005/bin/write-properties#newcode1
bin/write-properties:1: #!/usr/bin/env node
On 2012/11/09 13:57:38, gary.poster wrote:
> As I said, I'd like to delete.

Done.

https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js
File lib/merge-files.js (right):

https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode14
lib/merge-files.js:14: // end.
On 2012/11/09 13:57:38, gary.poster wrote:
> Another option (up to you): remove this comment, and do the export of
each
> function as you define it ("expose.minify = function(file) {...}" for
instance)
> or immediately after you define it.

Done.

https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newc...

[lp:~tveronezi/juju-gui/uglify]

248. By Thiago Veronezi on 2012-11-09

code review

249. By Thiago Veronezi on 2012-11-09

add comments about 'bootstrap=false'

[Thiago Veronezi (tveronezi)]

https://codereview.appspot.com/6821090/diff/15005/app/modules.js
File app/modules.js (right):

https://codereview.appspot.com/6821090/diff/15005/app/modules.js#newcode4
app/modules.js:4: ignoreRegistered: true,
On 2012/11/09 13:57:38, gary.poster wrote:
> Is there a setting we can use to say "never download anything from the
internet,
> ever"? If so, I suggest we use that both for this and the debug
modules file.

> Is it bootstrap: false? Or is this already handled somehow?

We agreed that it wont be possible to use it. I've added comments about
this know issue in the "merge-files" file.

https://codereview.appspot.com/6821090/

[lp:~tveronezi/juju-gui/uglify]

250. By Thiago Veronezi on 2012-11-09

add comments about 'bootstrap=false'

[Thiago Veronezi (tveronezi)]

Please take a look.

https://codereview.appspot.com/6821090/

[Gary Poster (gary)]

Hi Thiago. This looks good to me.

Per our discussions:

- We are foregoing tests on the lib functions. You can see now how you
would do them. You might do a follow-up branch with tests or might not.
  I would be happy to see that change. However, I won't stop you from
landing this without them.

- We are foregoing the "bootstrap=false" change that forces us to keep
from downloading files. It would be a nice seatbelt but it had problems
that you documented.

Thank you!

Gary

https://codereview.appspot.com/6821090/diff/2005/Makefile
File Makefile (right):

https://codereview.appspot.com/6821090/diff/2005/Makefile#newcode83
Makefile:83: @node server.js debug
yay, much nicer, thanks

https://codereview.appspot.com/6821090/diff/2005/app/modules-debug.js
File app/modules-debug.js (right):

https://codereview.appspot.com/6821090/diff/2005/app/modules-debug.js#newcode3
app/modules-debug.js:3: // "lib/server.js".
Great.

https://codereview.appspot.com/6821090/

[Benji York (benji)]

review: Approve (code)

[lp:~tveronezi/juju-gui/uglify]

251. By Thiago Veronezi on 2012-11-09

trunk merge

[Thiago Veronezi (tveronezi)]

*** Submitted:

Resource minification/aggregation js

* We should aggregate and minimize the js sources in order to improve
the load speed of the application;
* We dont want to use the yui combo loader feature;
* The final product will provide three js files: one for the YUI
dependencies, one for our custom js code and one for third part js like
d3.

R=benji, gary.poster
CC=
https://codereview.appspot.com/6821090

https://codereview.appspot.com/6821090/

=== modified file '.bzrignore'
--- .bzrignore	2012-10-31 09:16:13 +0000
+++ .bzrignore	2012-11-09 20:53:21 +0000
@@ -20,3 +20,4 @@
 yuidoc
 app/assets/manifest.appcache
 app/assets/sprite
+app/assets/javascripts/generated/

=== modified file 'Makefile'
--- Makefile	2012-11-06 13:16:34 +0000
+++ Makefile	2012-11-09 20:53:21 +0000
@@ -9,10 +9,15 @@
 	node_modules/mocha node_modules/d3 node_modules/graceful-fs \
 	node_modules/should node_modules/jshint node_modules/expect.js \
 	node_modules/express node_modules/yui node_modules/yuidoc \
-	node_modules/grunt node_modules/node-spritesheet
+	node_modules/grunt node_modules/node-spritesheet \
+	node_modules/node-minify
 TEMPLATE_TARGETS=$(shell bzr ls -k file app/templates)
 SPRITE_SOURCE_FILES=$(shell bzr ls -R -k file app/assets/images)
 SPRITE_GENERATED_FILES=app/assets/sprite/sprite.css app/assets/sprite/sprite.png
+COMPRESSED_FILES=app/assets/javascripts/generated/all-app-debug.js \
+	app/assets/javascripts/generated/all-app.js \
+	app/assets/javascripts/generated/all-third.js \
+	app/assets/javascripts/generated/all-yui.js
 DATE=$(shell date -u)
 APPCACHE=app/assets/manifest.appcache
 
@@ -37,7 +42,7 @@
 	@ln -sf `pwd`/node_modules/yui ./app/assets/javascripts/
 	@ln -sf `pwd`/node_modules/d3/d3.v2* ./app/assets/javascripts/
 
-install: appcache $(NODE_TARGETS) app/templates.js yuidoc spritegen
+install: appcache $(NODE_TARGETS) app/templates.js yuidoc spritegen combinejs
 
 gjslint: virtualenv/bin/gjslint
 	@virtualenv/bin/gjslint --strict --nojsdoc --jslint_error=all \
@@ -61,11 +66,22 @@
 
 spritegen: $(SPRITE_GENERATED_FILES)
 
+$(COMPRESSED_FILES): node_modules/yui node_modules/d3/d3.v2.min.js $(JSFILES)
+	@rm -Rf app/assets/javascripts/generated/
+	@mkdir app/assets/javascripts/generated/
+	@./bin/merge-files
+
+combinejs: $(COMPRESSED_FILES)
+
 prep: beautify lint
 
 test: install
 	@./test-server.sh
 
+debug: install
+	@echo "Customize config.js to modify server settings"
+	@node server.js debug
+
 server: install
 	@echo "Customize config.js to modify server settings"
 	@node server.js
@@ -75,6 +91,7 @@
 	@make -C docs clean
 	@rm -Rf bin/sprite/
 	@rm -Rf app/assets/sprite/
+	@rm -Rf app/assets/javascripts/generated/
 
 $(APPCACHE): manifest.appcache.in
 	@cp manifest.appcache.in $(APPCACHE)
@@ -91,4 +108,5 @@
 appcache-force: appcache-touch appcache
 
 .PHONY: test lint beautify server install clean prep jshint gjslint \
-	appcache appcache-touch appcache-force yuidoc spritegen yuidoc-lint
+	appcache appcache-touch appcache-force yuidoc spritegen yuidoc-lint \
+	combinejs

=== modified file 'app/index.html'
--- app/index.html	2012-11-01 13:21:53 +0000
+++ app/index.html	2012-11-09 20:53:21 +0000
@@ -69,9 +69,9 @@
         <div id="vp-right-border"></div>
     </div>
     <!-- javascript away -->
-    <script src="/juju-ui/assets/javascripts/yui/yui/yui-debug.js"></script>
-    <script src="/juju-ui/modules.js"></script>
-    <script src="/config.js"></script>
+    <script src="/assets/yui.js"></script>
+    <script src="/assets/all-third.js"></script>
+    <script src="/assets/modules.js"></script>
     <script>
       YUI(GlobalConfig).use(["juju-gui"], function(Y) {
         app = new Y.juju.App(juju_config);

=== added file 'app/modules-debug.js'
--- app/modules-debug.js	1970-01-01 00:00:00 +0000
+++ app/modules-debug.js	2012-11-09 20:53:21 +0000
@@ -0,0 +1,131 @@
+// This file is used for development only. In order to use it you should call
+// the "make debug" command. This command passes the "debug" argument to the
+// "lib/server.js".
+GlobalConfig = {
+  filter: 'debug',
+  // Set "true" for verbose logging of YUI
+  debug: false,
+
+  // Use Rollups
+  combine: false,
+
+  groups: {
+    d3: {
+      modules: {
+        'd3': {
+          'fullpath': '/juju-ui/assets/javascripts/d3.v2.min.js'
+        }
+      }
+    },
+    juju: {
+      modules: {
+        // Primitives
+
+        'svg-layouts': {
+          fullpath: '/juju-ui/assets/javascripts/svg-layouts.js'
+        },
+
+        'reconnecting-websocket': {
+          fullpath: '/juju-ui/assets/javascripts/reconnecting-websocket.js'
+        },
+
+        // Views
+        'juju-view-utils': {
+          fullpath: '/juju-ui/views/utils.js'
+        },
+
+        'juju-charm-panel': {
+          fullpath: '/juju-ui/views/charm-panel.js'
+        },
+
+        'juju-notifications': {
+          fullpath: '/juju-ui/views/notifications.js'
+        },
+
+        'juju-view-environment': {
+          fullpath: '/juju-ui/views/environment.js'
+        },
+
+        'juju-view-service': {
+          fullpath: '/juju-ui/views/service.js'
+        },
+
+        'juju-view-unit': {
+          fullpath: '/juju-ui/views/unit.js'
+        },
+
+        'juju-view-charm-collection': {
+          fullpath: '/juju-ui/views/charm.js'
+        },
+
+        'juju-view-charm': {
+          fullpath: '/juju-ui/views/charm.js'
+        },
+
+        'juju-templates': {
+          fullpath: '/juju-ui/templates.js'
+        },
+
+        'juju-views': {
+          use: [
+            'juju-templates',
+            'juju-notifications',
+            'juju-view-utils',
+            'juju-view-environment',
+            'juju-view-service',
+            'juju-view-unit',
+            'juju-view-charm',
+            'juju-view-charm-collection'
+          ]
+        },
+
+        // Models
+        'juju-endpoints': {
+          fullpath: '/juju-ui/models/endpoints.js'
+        },
+
+        'juju-charm-models': {
+          requires: ['juju-charm-id'],
+          fullpath: '/juju-ui/models/charm.js'
+        },
+
+        'juju-models': {
+          requires: [
+            'model', 'model-list', 'juju-endpoints', 'juju-charm-models'],
+          fullpath: '/juju-ui/models/models.js'
+        },
+
+        // Connectivity
+        'juju-env': {
+          requires: ['reconnecting-websocket'],
+          fullpath: '/juju-ui/store/env.js'
+        },
+
+        'juju-notification-controller': {
+          fullpath: '/juju-ui/store/notifications.js'
+        },
+
+        'juju-charm-store': {
+          requires: ['juju-charm-id'],
+          fullpath: '/juju-ui/store/charm.js'
+        },
+
+        'juju-controllers': {
+          use: ['juju-env', 'juju-charm-store',
+            'juju-notification-controller']
+        },
+
+        // App
+        'juju-gui': {
+          fullpath: '/juju-ui/app.js',
+          requires: [
+            'juju-controllers',
+            'juju-views',
+            'juju-models',
+            'juju-view-charm-search'
+          ]
+        }
+      }
+    }
+  }
+};

=== modified file 'app/modules.js'
--- app/modules.js	2012-11-01 13:30:58 +0000
+++ app/modules.js	2012-11-09 20:53:21 +0000
@@ -1,69 +1,11 @@
 GlobalConfig = {
-  // Uncomment for debug versions of YUI.
-  filter: 'debug',
-  // Uncomment for verbose logging of YUI
   debug: false,
-
-  // Use Rollups
-  combine: false,
+  // YUI will not download the modules. They are supposed to be already loaded.
+  ignoreRegistered: true,
 
   groups: {
-    d3: {
-      modules: {
-        'd3': {
-          'fullpath': '/juju-ui/assets/javascripts/d3.v2.min.js'
-        }
-      }
-    },
     juju: {
       modules: {
-        // Primitives
-
-        'svg-layouts': {
-          fullpath: '/juju-ui/assets/javascripts/svg-layouts.js'
-        },
-
-        'reconnecting-websocket': {
-          fullpath: '/juju-ui/assets/javascripts/reconnecting-websocket.js'
-        },
-
-        // Views
-        'juju-view-utils': {
-          fullpath: '/juju-ui/views/utils.js'
-        },
-
-        'juju-charm-panel': {
-          fullpath: '/juju-ui/views/charm-panel.js'
-        },
-
-        'juju-notifications': {
-          fullpath: '/juju-ui/views/notifications.js'
-        },
-
-        'juju-view-environment': {
-          fullpath: '/juju-ui/views/environment.js'
-        },
-
-        'juju-view-service': {
-          fullpath: '/juju-ui/views/service.js'
-        },
-
-        'juju-view-unit': {
-          fullpath: '/juju-ui/views/unit.js'
-        },
-
-        'juju-view-charm-collection': {
-          fullpath: '/juju-ui/views/charm.js'
-        },
-
-        'juju-view-charm': {
-          fullpath: '/juju-ui/views/charm.js'
-        },
-
-        'juju-templates': {
-          fullpath: '/juju-ui/templates.js'
-        },
-
         'juju-views': {
           use: [
             'juju-templates',
@@ -77,51 +19,12 @@
           ]
         },
 
-        // Models
-        'juju-endpoints': {
-          fullpath: '/juju-ui/models/endpoints.js'
-        },
-
-        'juju-charm-models': {
-          requires: ['juju-charm-id'],
-          fullpath: '/juju-ui/models/charm.js'
-        },
-
-        'juju-models': {
-          requires: [
-            'model', 'model-list', 'juju-endpoints', 'juju-charm-models'],
-          fullpath: '/juju-ui/models/models.js'
-        },
-
-        // Connectivity
-        'juju-env': {
-          requires: ['reconnecting-websocket'],
-          fullpath: '/juju-ui/store/env.js'
-        },
-
-        'juju-notification-controller': {
-          fullpath: '/juju-ui/store/notifications.js'
-        },
-
-        'juju-charm-store': {
-          requires: ['juju-charm-id'],
-          fullpath: '/juju-ui/store/charm.js'
-        },
-
+        // 'juju-controllers' is just an alias for the modules defined by 'use'
+        // If we use "requires: ['juju-controllers']" that means
+        // "requires: ['juju-env', 'juju-charm-store',
+        // 'juju-notification-controller']"
         'juju-controllers': {
-          use: ['juju-env', 'juju-charm-store',
-            'juju-notification-controller']
-        },
-
-        // App
-        'juju-gui': {
-          fullpath: '/juju-ui/app.js',
-          requires: [
-            'juju-controllers',
-            'juju-views',
-            'juju-models',
-            'juju-view-charm-search'
-          ]
+          use: ['juju-env', 'juju-charm-store', 'juju-notification-controller']
         }
       }
     }

=== added file 'bin/merge-files'
--- bin/merge-files	1970-01-01 00:00:00 +0000
+++ bin/merge-files	2012-11-09 20:53:21 +0000
@@ -0,0 +1,75 @@
+#!/usr/bin/env node
+
+/**
+ * We aggregate and minimize the js sources in order to improve the load speed
+ * of the application.
+ *
+ * We don't want to use the yui combo loader feature because we want to be able
+ * to run from only static files and we want to be able to run behind a
+ * firewall without access to the internet.
+ *
+ * The final product will provide three js files: one for the YUI dependencies,
+ * one for our custom js code and one for third party js like d3.
+ * 
+ * Known issues:
+ * (1) If we set "bootstrap=false" in the GlobalConfig object, yui disables the
+ *     loader object. It means it wont even try to download modules. We cannot
+ *     do it because the loader also manages the "use" property which defines
+ *     aliases for some of your modules ('juju-views' and 'juju-controllers').
+ * (2) During development, we've noticed that some of the yui modules weren't
+ *     included in the list of yui files (lang/datatype-date-format_en-US,
+ *     parallel, app-transitions-native, gallery-markdown, loader-base). For
+ *     some reason, the loader does not resolve the names of the files for
+ *     these modules. We need to add them manually in this file.
+ */
+
+'use strict';
+
+require('yui').YUI().use(['yui'], function(Y) {
+  var merge = require('../lib/merge-files.js'),
+      syspath = require('path'),
+      paths,
+      reqs;
+
+  // First we find all of the paths to our custom Javascript in the app
+  // directory.  We need to tell the function to ignore the "assets" directory
+  // and the debug version of the modules file. I need to use
+  // "syspath.join(process.cwd(), ...)" or else I have... "Error: Cannot find
+  // module './app/config.js'" from node's internal module.js file.
+  paths = merge.readdir(syspath.join(process.cwd(), './app'),
+    [syspath.join(process.cwd(), './app/assets'), './app/modules-debug.js']);
+
+  // Get the name of all the YUI modules that our custom js files use directly.
+  reqs = merge.loadRequires(paths);
+
+  // For some reason the loader does not get these requirements.
+  // (Known issue #2)
+  reqs.push('lang/datatype-date-format_en-US');
+  reqs.push('parallel');
+  reqs.push('app-transitions-native');
+  reqs.push('gallery-markdown');
+  reqs.push('loader-base');
+
+  // Get all of the YUI files and their dependencies, and combine them.
+  merge.combine(merge.getYUIFiles(reqs),
+      './app/assets/javascripts/generated/all-yui.js', true);
+
+  // Combine third party js libraries
+  merge.combine([ './app/assets/javascripts/d3.v2.min.js',
+      './app/assets/javascripts/reconnecting-websocket.js',
+      './app/assets/javascripts/svg-layouts.js' ],
+      './app/assets/javascripts/generated/all-third.js', true);
+
+  // Now we only need to generate the file that is used to tell YUI where all
+  // the dependencies are.  We either use a debug version or the production
+  // version.  The debug version is simply pointers to the individual files.
+  // The production version aggregates all of the files together.
+
+  // Creating the combined file for the modules-debug.js and config.js files
+  merge.combine([ './app/modules-debug.js', './app/config.js' ],
+      './app/assets/javascripts/generated/all-app-debug.js', false);
+
+  // Creating the combined file for all our files.  Note that this includes
+  // app/modules.js in the rollup, which is why that file still needs exist.
+  merge.combine(paths, './app/assets/javascripts/generated/all-app.js', true);
+});
\ No newline at end of file

=== modified file 'grunt.js'
--- grunt.js	2012-10-30 11:36:00 +0000
+++ grunt.js	2012-11-09 20:53:21 +0000
@@ -11,7 +11,7 @@
 
         },
         files: {
-          'bin' : 'app/assets/images/*'
+          'bin': 'app/assets/images/*'
         }
       }
     }

=== added file 'lib/merge-files.js'
--- lib/merge-files.js	1970-01-01 00:00:00 +0000
+++ lib/merge-files.js	2012-11-09 20:53:21 +0000
@@ -0,0 +1,137 @@
+'use strict';
+
+// We need the yui name to be available in all modules (as a global variable).
+// This only happens if we remove the 'var' keyword or add it to the "global"
+// variable.
+global.YUI = require('yui').YUI;
+
+YUI().use(['yui'], function(Y) {
+  var fs = require('fs'),
+      syspath = require('path'),
+      compressor = require('node-minify');
+
+  function minify(file) {
+    var execution = new compressor.minify(
+        { type: 'uglifyjs',
+          fileIn: file,
+          fileOut: file,
+          callback: function(err) {
+            if (err) {
+              console.log(err);
+            }
+          }});
+  }
+  exports.minify = minify;
+
+  // It combines the files defined by "files" into a single (compressed or not)
+  // js file.
+  function combine(files, outputFile, shouldMinify) {
+    var str = [];
+    Y.Array.each(files, function(file) {
+      console.log('file -> ' + file);
+      str.push(fs.readFileSync(file, 'utf8'));
+    });
+    fs.writeFileSync(outputFile, str.join('\n'), 'utf8');
+    if (shouldMinify) {
+      minify(outputFile);
+    }
+  }
+  exports.combine = combine;
+
+  // It gets the name of all the files under 'path', recursively. It ignores
+  // files and directories included in the "ignore" array. These will
+  // typically be set to ignore our assets and our module file.
+  function readdir(path, ignore) {
+    var result = [], // These are the filenames we will return.
+        dirs = [], // These are the directories into which we will recurse.
+        files = fs.readdirSync(path);
+
+    Y.Array.each(files, function(value) {
+      var fileName, file;
+      fileName = path + '/' + value;
+      file = fs.statSync(fileName);
+      if (file.isFile()) {
+        if (syspath.extname(fileName).toLowerCase() === '.js') {
+          if (ignore && ignore.indexOf(fileName) >= 0) {
+            console.log('SKIPPING FILE -> ' + fileName);
+          } else {
+            result.push(fileName);
+          }
+        }
+      } else if (file.isDirectory()) {
+        console.log('DIRECTORY -> ' + fileName);
+        if (ignore && ignore.indexOf(fileName) >= 0) {
+          console.log('SKIPPING DIRECTORY -> ' + fileName);
+        } else {
+          dirs.push(fileName);
+        }
+      }
+    });
+    // We have read all the filenames and all the directory names. Now
+    // it is time to recurse into the directories that we have collected.
+    Y.Array.each(dirs, function(directory) {
+      // This is the best Javascript offers for appending one array to
+      // another.
+      result.push.apply(result, readdir(directory));
+    });
+    return result;
+  }
+  exports.readdir = readdir;
+
+  // It reads the YUI 'requires' attribute of all our custom js files
+  // (as assembled by readdir) and returns the YUI modules on which we
+  // directly depend.
+  function loadRequires(paths) {
+    var originalAdd, modules, yuiReqs;
+    modules = {};
+
+    originalAdd = YUI.add;
+    // This is a trick to get the 'requires' value from the module
+    // definition.
+    YUI.add = function(name, fn, version, details) {
+      if (details && details.requires) {
+        modules[name] = details.requires;
+      }
+    };
+
+    Y.Array.each(paths, function(path) {
+      // It triggers the custom 'add' method above
+      require(path);
+    });
+    YUI.add = originalAdd;
+
+    // Getting all the YUI dependencies that we need
+    yuiReqs = [];
+    Y.Object.each(modules, function(requires) {
+      Y.Array.each(requires, function(value) {
+        if (!modules[value]) {
+          // This is not one of our modules but a yui one.
+          if (yuiReqs.indexOf(value) < 0) {
+            // avoid duplicates
+            yuiReqs.push(value);
+          }
+        }
+      });
+    });
+    return yuiReqs;
+  }
+  exports.loadRequires = loadRequires;
+
+  // Using the example http://yuilibrary.com/yui/docs/yui/loader-resolve.html
+  // Given a list of direct YUI requirements, return the filenames of these
+  // modules and their dependencies (indirect and direct).
+  function getYUIFiles(reqs) {
+    var loader, out;
+    // The loader automatically handles following our dependencies.
+    loader = new Y.Loader({
+      // The base tells the loader where to look for the YUI files. We
+      // look directly in the npm YUI package, but we could just as well look
+      // in the symbolic link app/assets/javascripts/yui.
+      base: './node_modules/yui/',
+      ignoreRegistered: true,
+      require: reqs});
+    out = loader.resolve(true);
+    return out.js;
+  }
+  exports.getYUIFiles = getYUIFiles;
+});

=== modified file 'lib/server.js'
--- lib/server.js	2012-10-02 11:29:56 +0000
+++ lib/server.js	2012-11-09 20:53:21 +0000
@@ -5,6 +5,7 @@
     fs = require('fs'),
     path = require('path'),
     config = require('../config').config.server,
+    debugMode = (String(process.argv[2]).toLowerCase() === 'debug'),
     public_dir = config.public_dir,
     Templates = require('./templates.js'),
     view = require('./view.js');
@@ -46,8 +47,24 @@
   });
 });
 
-server.get('/config.js', function(req, res) {
-  res.sendfile('app/config.js');
+server.get('/assets/all-third.js', function(req, res) {
+  res.sendfile('app/assets/javascripts/generated/all-third.js');
+});
+
+server.get('/assets/modules.js', function(req, res) {
+  if (debugMode) {
+    res.sendfile('app/assets/javascripts/generated/all-app-debug.js');
+  } else {
+    res.sendfile('app/assets/javascripts/generated/all-app.js');
+  }
+});
+
+server.get('/assets/yui.js', function(req, res) {
+  if (debugMode) {
+    res.sendfile('app/assets/javascripts/yui/yui/yui-debug.js');
+  } else {
+    res.sendfile('app/assets/javascripts/generated/all-yui.js');
+  }
 });
 
 server.get('*', function(req, res) {

=== modified file 'package.json'
--- package.json	2012-10-30 11:33:21 +0000
+++ package.json	2012-11-09 20:53:21 +0000
@@ -30,7 +30,8 @@
     "graceful-fs": "1.1.x",
     "rimraf": "2.0.x",
     "grunt": ">=0.2.0",
-    "node-spritesheet": ">=0.2.3"
+    "node-spritesheet": ">=0.2.3",
+    "node-minify": "0.5.0"
   },
   "optionalDependencies": {}
 }

=== modified file 'test/index.html'
--- test/index.html	2012-11-01 13:12:28 +0000
+++ test/index.html	2012-11-09 20:53:21 +0000
@@ -4,7 +4,7 @@
   <meta charset="utf-8">
   <link rel="stylesheet" href="assets/mocha.css">
   <script src="../app/assets/javascripts/yui/yui/yui-debug.js"></script>
-  <script src="../app/modules.js"></script>
+  <script src="../app/modules-debug.js"></script>
   <script src="assets/chai.js"></script>
   <script src="assets/mocha.js"></script>
   <script src="utils.js"></script>