yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

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: <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?

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

-- 
https://code.launchpad.net/~tveronezi/juju-gui/uglify/+merge/133116
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~tveronezi/juju-gui/uglify into lp:juju-gui.