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://codereview.appspot.com/6821090/diff/7001/app/assets/combine/properties-dev.js File app/assets/combine/properties-dev.js (right): https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/properties-dev.js#newcode1 app/assets/combine/properties-dev.js:1: exports.debugMode = true; So, AIUI, this is just a way to make the "make debug" target work. Could we instead use a flag to merge-files.js and avoid messing with a file like this? https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/properties-prod.js File app/assets/combine/properties-prod.js (right): https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/properties-prod.js#newcode1 app/assets/combine/properties-prod.js:1: exports.debugMode = false; (same comment as with the dev file: I'd like to see this removed) 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: On 2012/11/07 18:59:19, thiago wrote: > What about '/assets/yui.js'? Id this ok? Yeah, I think that's the kind of thing he means. https://codereview.appspot.com/6821090/diff/7001/app/modules-debug.js File app/modules-debug.js (right): https://codereview.appspot.com/6821090/diff/7001/app/modules-debug.js#newcode2 app/modules-debug.js:2: // Uncomment for debug versions of YUI. Maybe we don't need this comment any more, since it is always in debug mode? https://codereview.appspot.com/6821090/diff/7001/app/modules-debug.js#newcode5 app/modules-debug.js:5: debug: false, I suggest that we keep this one commented. Otherwise, if you want to leave this in place, I suggest that you revise the comment in line 4. https://codereview.appspot.com/6821090/diff/7001/app/modules.js File app/modules.js (right): https://codereview.appspot.com/6821090/diff/7001/app/modules.js#newcode3 app/modules.js:3: ignoreRegistered: true, Maybe add a comment explaining what this does? https://codereview.appspot.com/6821090/diff/7001/app/modules.js#newcode8 app/modules.js:8: 'juju-views': { Why do we still need this? https://codereview.appspot.com/6821090/diff/7001/app/modules.js#newcode21 app/modules.js:21: 'juju-controllers': { Same question 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#newcode56 lib/server.js:56: res.sendfile('app/assets/javascripts/generated/all-app-debug.js'); Interesting. so...we'll never have access to individual files? Oh, no, I see. Cool, nicely done. Something to consider is what a static version of the GUI would look like, without server.js--and then how close we can get to that. The closer we can have our dev environment like our distributed environment, the better. Rather than having the server decide which .js file to serve, what about having two different index.html files, one of which serves the debug and one of which serves the normal version? https://codereview.appspot.com/6821090/