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 the line 82, it loads one of our js files. These js files call "YUI.add(..." and YUI is defined by "global.YUI". 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; On 2012/11/07 20:49:31, gary.poster wrote: > 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? Nice one tkx. 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; On 2012/11/07 20:49:31, gary.poster wrote: > (same comment as with the dev file: I'd like to see this removed) Done. 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 20:49:31, gary.poster wrote: > 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. ok 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. On 2012/11/07 20:49:31, gary.poster wrote: > Maybe we don't need this comment any more, since it is always in debug mode? Done. https://codereview.appspot.com/6821090/diff/7001/app/modules-debug.js#newcode5 app/modules-debug.js:5: debug: false, On 2012/11/07 20:49:31, gary.poster wrote: > 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. Comment revised. 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, On 2012/11/07 20:49:31, gary.poster wrote: > Maybe add a comment explaining what this does? Done. https://codereview.appspot.com/6821090/diff/7001/app/modules.js#newcode8 app/modules.js:8: 'juju-views': { On 2012/11/07 20:49:31, gary.poster wrote: > Why do we still need this? We need it otherwise we need to define all the modules individually. (and we still have the debug mode) https://codereview.appspot.com/6821090/diff/7001/app/modules.js#newcode21 app/modules.js:21: 'juju-controllers': { On 2012/11/07 20:49:31, gary.poster wrote: > Same question ditto. 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) { On 2012/11/07 18:59:19, thiago wrote: > 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")? Done. 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'); On 2012/11/07 20:49:31, gary.poster wrote: > 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? I had this idea. Ben didn't like it, and I agreed with him. :) https://codereview.appspot.com/6821090/