Issue 6821090: Resource minification/aggregation js
|
Created:
12 years, 4 months ago by thiago Modified:
12 years, 4 months ago Reviewers:
mp+133116 Visibility:
Public. |
DescriptionResource 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)
Patch Set 1 #
Total comments: 2
Patch Set 2 : Resource minification/aggregation js #Patch Set 3 : Resource minification/aggregation js #
Total comments: 72
Patch Set 4 : Resource minification/aggregation js #
Total comments: 1
Patch Set 5 : Resource minification/aggregation js #
Total comments: 27
Patch Set 6 : Resource minification/aggregation js #
Total comments: 2
Patch Set 7 : Resource minification/aggregation js #
MessagesTotal messages: 22
Please take a look.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Thanks for the branch. I had a few small questions/suggestions and one bigger question (the one about merge-files.js) that needs to be answered before we can move forward. 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 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 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 This line is too long. https://codereview.appspot.com/6821090/diff/7001/Makefile#newcode80 Makefile:80: @cp app/assets/combine/properties-prod.js app/assets/javascripts/generated/properties.js This line is too long. https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... File app/assets/combine/merge-files.js (right): https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... app/assets/combine/merge-files.js:1: 'use strict'; 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> 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) { Why should the path exposed via HTTP ("source") be different than the path on disk ("assets")?
Sign in to reply to this message.
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-fil... File app/assets/combine/merge-files.js (right): https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... 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")?
Sign in to reply to this message.
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-fil... File app/assets/combine/merge-files.js (right): https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... 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-fil... 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-fil... 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-fil... 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/propertie... File app/assets/combine/properties-dev.js (right): https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/propertie... 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/propertie... File app/assets/combine/properties-prod.js (right): https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/propertie... 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: <script src="/source/yui.js"></script> 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?
Sign in to reply to this message.
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-fil... File app/assets/combine/merge-files.js (right): https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... 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-fil... 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-fil... 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/propertie... File app/assets/combine/properties-dev.js (right): https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/propertie... 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/propertie... File app/assets/combine/properties-prod.js (right): https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/propertie... 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: <script src="/source/yui.js"></script> 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. :)
Sign in to reply to this message.
I have added a review of app/assets/combine/merge-files.js (which should probably be in a different place, "assets" are things we use in the browser. I would put it in the bin directory). 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 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. https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... File app/assets/combine/merge-files.js (right): https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... app/assets/combine/merge-files.js:1: 'use strict'; 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. https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... app/assets/combine/merge-files.js:3: // http://stackoverflow.com/questions/5348685/node-js-require-inheritance 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. https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... app/assets/combine/merge-files.js:5: I am surprised that we had to write this script. I would have expected someone, somewhere to have already done this for us. https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... app/assets/combine/merge-files.js:14: var execution = new compressor.minify({ object literal style https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... app/assets/combine/merge-files.js:88: var reqs = (function() { This function would benefit from a name. https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... app/assets/combine/merge-files.js:105: // Using the example http://yuilibrary.com/yui/docs/yui/loader-resolve.html 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? https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... app/assets/combine/merge-files.js:111: loader = new Y.Loader({ object literal style https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... app/assets/combine/merge-files.js:126: (function() { Why are there all of these anonymous functions that are just called immediately? https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... app/assets/combine/merge-files.js:142: outputFile = syspath.join(process.cwd(), This looks redundant. Why do we need to join the current working directory with a path that would have been interpreted as relative to the CWD anyway? https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... app/assets/combine/merge-files.js:152: (function() { This and the above two function -- and, to a lesser extent, the one above that -- have very similar structures. Here is a refactoring that combines all four into a smaller function: http://paste.ubuntu.com/1341063/ https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... app/assets/combine/merge-files.js:162: })(); This program is sufficiently large that it would benefit from tests. They would have helped me a great deal while doing my refactoring. https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... app/assets/combine/merge-files.js:163: There is an unnecessary blank line at the end of the file.
Sign in to reply to this message.
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-fil... File app/assets/combine/merge-files.js (right): https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... 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-fil... 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-fil... 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-fil... 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-fil... 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-fil... 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-fil... app/assets/combine/merge-files.js:111: loader = new Y.Loader({ 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-fil... app/assets/combine/merge-files.js:126: (function() { On 2012/11/07 22:00:47, benji wrote: > Why are there all of these anonymous functions that are just called immediately? The have different scopes. So I can separate concerns and I can reuse variables names :O) https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... app/assets/combine/merge-files.js:142: outputFile = syspath.join(process.cwd(), On 2012/11/07 22:00:47, benji wrote: > This looks redundant. Why do we need to join the current working directory with > a path that would have been interpreted as relative to the CWD anyway? I removed some of them, but I still need to keep two calls to it. You will see it in the new proposal. https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... app/assets/combine/merge-files.js:152: (function() { On 2012/11/07 22:00:47, benji wrote: > This and the above two function -- and, to a lesser extent, the one above that > -- have very similar structures. Here is a refactoring that combines all four > into a smaller function: http://paste.ubuntu.com/1341063/ Nice. Done. https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... app/assets/combine/merge-files.js:162: })(); On 2012/11/07 22:00:47, benji wrote: > This program is sufficiently large that it would benefit from tests. They would > have helped me a great deal while doing my refactoring. This is not part of the application. This is a build tool. Do we need to create tests for it? https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-fil... app/assets/combine/merge-files.js:163: On 2012/11/07 22:00:47, benji wrote: > There is an unnecessary blank line at the end of the file. It seems all our files have it.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
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.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
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,
Sign in to reply to this message.
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/merge-files.js:14: // end. 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. https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode29 lib/merge-files.js:29: // or not) Trivial: Please adjust the comment formatting. https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode44 lib/merge-files.js:44: // ignores Trivial: Please adjust the comment formatting. https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode75 lib/merge-files.js:75: // it is Trivial: Please adjust the comment formatting. https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode86 lib/merge-files.js:86: // (as Trivial: Please adjust the comment formatting. https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode128 lib/merge-files.js:128: // these Trivial: Please adjust the comment formatting.
Sign in to reply to this message.
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#newcode29 lib/merge-files.js:29: // or not) On 2012/11/09 13:57:38, gary.poster wrote: > Trivial: Please adjust the comment formatting. My formatter formats comments... code and formatter fixed. https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode44 lib/merge-files.js:44: // ignores On 2012/11/09 13:57:38, gary.poster wrote: > Trivial: Please adjust the comment formatting. Done. https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode75 lib/merge-files.js:75: // it is On 2012/11/09 13:57:38, gary.poster wrote: > Trivial: Please adjust the comment formatting. Done. https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode86 lib/merge-files.js:86: // (as On 2012/11/09 13:57:38, gary.poster wrote: > Trivial: Please adjust the comment formatting. Done. https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode128 lib/merge-files.js:128: // these On 2012/11/09 13:57:38, gary.poster wrote: > Trivial: Please adjust the comment formatting. Done.
Sign in to reply to this message.
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.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
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.
Sign in to reply to this message.
This is in landable shape now. Thanks for the work on it, especially in review. Re. testing: My stance is that we can survive with the code being untested for the time being, but if/when we touch the code again solid tests need to be written first. I know if I'm the one doing the touching I'll certainly want the confidence that I am not breaking anything by changing the code.
Sign in to reply to this message.
*** 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
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
