Remove "requires" from modules-debug.js

Remove "requires" from modules-debug.js

The minification process does not read the "requires" parameters defined in "modules-debug.js". This parameter should be defined in each custom yui object that uses some kind of internal or external requirement. The sole use of "all-app-debug.js" is to define the fullpath of the file that defines a given module.

This patch removes all the existing "requires" properties from "modules-debug.js" and add then where they are needed.

https://code.launchpad.net/~tveronezi/juju-gui/change-requires-param/+merge/135198

(do not edit description out of merge proposal)

[thiago, 2012-11-20 17:00:35]

Please take a look.

[thiago, 2012-11-20 19:03:30]

Please take a look.

[gary.poster, 2012-11-20 21:01:19]

Hi Thiago.  Thank you for the quick turnaround on this fix.  I have only done a
code review of this so far, and not a functional test/review.  However, I have
some concerns with the way you changed the tests, and I'd like to see those
resolved before I approve.

Gary

https://codereview.appspot.com/6856070/diff/3001/app/modules-debug.js
File app/modules-debug.js (left):

https://codereview.appspot.com/6856070/diff/3001/app/modules-debug.js#oldcode96
app/modules-debug.js:96: requires: ['juju-charm-id'],
I don't really understand why these were here instead of in the modules to begin
with.  That makes me nervous, because often people have a good reason for this
sort of thing.  Do you know the answer?

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

https://codereview.appspot.com/6856070/diff/3001/app/modules-debug.js#newcode4
app/modules-debug.js:4: // property) and the "fullpath" of the file that
implement a given module. The
Might as well take the opportunity to be clearer about what the "use" property
does.  ("""This file declares which files implement modules, using the
"fullpath" property; and declares the membership of rollup modules, using the
"use" property to specify what the module name aliases.""" or something like
that.)

https://codereview.appspot.com/6856070/diff/3001/app/modules-debug.js#newcode5
app/modules-debug.js:5: // "requires" property should not be used here.
Please explain why  ("...should not be used here because the javascript
minimizer will not parse it" or something like that).

https://codereview.appspot.com/6856070/diff/3001/app/modules-debug.js#newcode16
app/modules-debug.js:16: modules: {
I really am tempted to suggest that the production files should parse this and
rewrite it after removing the "fullpath" modules, instead of making developers
maintain two versions of this "modules" file.  Even if that's a good idea,
that's a separate bug/branch.  I'm just thinking.

https://codereview.appspot.com/6856070/diff/3001/test/index.html
File test/index.html (right):

https://codereview.appspot.com/6856070/diff/3001/test/index.html#newcode40
test/index.html:40: YUI().use('node', 'event', function(runnerY) {
In terms of scoping rules, I'm pretty sure that this could be "Y" and the one
used by tests, below, could be "Y" also.  Did you not do it that way because you
felt it was more readable this way?

https://codereview.appspot.com/6856070/diff/3001/test/index.html#newcode61
test/index.html:61: window.Y = testsYUI;
We usually create our own Y in the tests.  Why do we have to do this?  Wouldn't
it be better to make our own Y in each test?

https://codereview.appspot.com/6856070/diff/3001/test/test_app.js
File test/test_app.js (left):

https://codereview.appspot.com/6856070/diff/3001/test/test_app.js#oldcode184
test/test_app.js:184: 'json-stringify',
This was specifying different requirements than you have put in the global Y. 
We really should not share the Y object.  We should keep our dependencies tight
to our tests in order to increase the maintainability and understandability of
our code.  This comment addresses all changes like this one.

[matthew.scott, 2012-11-20 21:26:04]

From IRC, this branch looks good functionally.  +1 from me, so long as Gary's
comments are addressed to both of your satisfaction.

[thiago, 2012-11-20 23:05:24]

Thanks for your review, guys!

https://codereview.appspot.com/6856070/diff/3001/app/modules-debug.js
File app/modules-debug.js (left):

https://codereview.appspot.com/6856070/diff/3001/app/modules-debug.js#oldcode96
app/modules-debug.js:96: requires: ['juju-charm-id'],
On 2012/11/20 21:01:19, gary.poster wrote:
> I don't really understand why these were here instead of in the modules to
begin
> with.  That makes me nervous, because often people have a good reason for this
> sort of thing.  Do you know the answer?

It is not their fault. The problem is that YUI has two places to define the very
same thing... which is not bad when we are loading non yui js files that depend
on other js files.

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

https://codereview.appspot.com/6856070/diff/3001/app/modules-debug.js#newcode4
app/modules-debug.js:4: // property) and the "fullpath" of the file that
implement a given module. The
On 2012/11/20 21:01:19, gary.poster wrote:
> Might as well take the opportunity to be clearer about what the "use" property
> does.  ("""This file declares which files implement modules, using the
> "fullpath" property; and declares the membership of rollup modules, using the
> "use" property to specify what the module name aliases.""" or something like
> that.)

Done.

https://codereview.appspot.com/6856070/diff/3001/app/modules-debug.js#newcode5
app/modules-debug.js:5: // "requires" property should not be used here.
On 2012/11/20 21:01:19, gary.poster wrote:
> Please explain why  ("...should not be used here because the javascript
> minimizer will not parse it" or something like that).

Done.

https://codereview.appspot.com/6856070/diff/3001/app/modules-debug.js#newcode16
app/modules-debug.js:16: modules: {
On 2012/11/20 21:01:19, gary.poster wrote:
> I really am tempted to suggest that the production files should parse this and
> rewrite it after removing the "fullpath" modules, instead of making developers
> maintain two versions of this "modules" file.  Even if that's a good idea,
> that's a separate bug/branch.  I'm just thinking.

I am tempted to simply remove the aliases. They are one of the reasons we cannot
completely turn the loader off, remember?

https://codereview.appspot.com/6856070/diff/3001/test/index.html
File test/index.html (right):

https://codereview.appspot.com/6856070/diff/3001/test/index.html#newcode40
test/index.html:40: YUI().use('node', 'event', function(runnerY) {
On 2012/11/20 21:01:19, gary.poster wrote:
> In terms of scoping rules, I'm pretty sure that this could be "Y" and the one
> used by tests, below, could be "Y" also.  Did you not do it that way because
you
> felt it was more readable this way?

Reverting...

https://codereview.appspot.com/6856070/diff/3001/test/index.html#newcode61
test/index.html:61: window.Y = testsYUI;
On 2012/11/20 21:01:19, gary.poster wrote:
> We usually create our own Y in the tests.  Why do we have to do this? 
Wouldn't
> it be better to make our own Y in each test?

Reverting...

https://codereview.appspot.com/6856070/diff/3001/test/test_app.js
File test/test_app.js (left):

https://codereview.appspot.com/6856070/diff/3001/test/test_app.js#oldcode184
test/test_app.js:184: 'json-stringify',
On 2012/11/20 21:01:19, gary.poster wrote:
> This was specifying different requirements than you have put in the global Y. 
> We really should not share the Y object.  We should keep our dependencies
tight
> to our tests in order to increase the maintainability and understandability of
> our code.  This comment addresses all changes like this one.

Done.

[thiago, 2012-11-20 23:09:47]

Please take a look.

[gary.poster, 2012-11-21 15:41:51]

Hi Thiago.  I like what you did with the tests, even though I wish it didn't
have to be part of this branch.  I'm OK with it landing though.

As I mentioned on IRC, I do see a test failure, but it seems shallow.  Please
fix that, and maybe the small comment change I suggest.

Thank you,

Gary

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

https://codereview.appspot.com/6856070/diff/9001/app/modules-debug.js#newcode10
app/modules-debug.js:10: // minimizer will not parse it.
Maybe add "Please put in in the module itself, instead."

https://codereview.appspot.com/6856070/diff/9001/test/test_app.js
File test/test_app.js (right):

https://codereview.appspot.com/6856070/diff/9001/test/test_app.js#newcode34
test/test_app.js:34: YUI(GlobalConfig).use(['juju-gui', 'juju-tests-utils'],
function(Y) {
This looks good to me.  However, it is a big change to how we write our tests,
and not consistently applied to our tests.  As I said on IRC, I think there is a
good argument to change our tests to be written this way, but I don't understand
why we need to change them here, in this branch.

In the interest of progress, I'm ok with landing this if you then make a
retrospective discussion card for us to collectively determine the way we want
to move forward.