juju-gui

Merge lp:~hatch/juju-gui/1130787-subapp-app-extension into lp:juju-gui/experimental

  1. 1130787-subapp-app-extension
  2. Merge into trunk
Proposed by Jeff Pihach
Status: Needs review
Proposed branch: lp:~hatch/juju-gui/1130787-subapp-app-extension
Merge into: lp:juju-gui/experimental
Diff against target: 311 lines (+243/-1) (has conflicts)
6 files modified
app/app.js (+6/-1)
app/assets/javascripts/app-subapp-extension.js (+144/-0)
app/modules-debug.js (+7/-0)
bin/merge-files (+5/-0)
test/index.html (+4/-0)
test/test_subapp_app_extension.js (+77/-0)
Text conflict in app/app.js
Text conflict in app/modules-debug.js
Text conflict in bin/merge-files
Text conflict in test/index.html
To merge this branch: bzr merge lp:~hatch/juju-gui/1130787-subapp-app-extension
Reviewer Review Type Date Requested Status
Juju GUI Hackers Pending
Review via email: mp+150414@code.launchpad.net

Description of the change

Created app subapp extension

https://codereview.appspot.com/7381055/

To post a comment you must log in.
Revision history for this message
Jeff Pihach (hatch) wrote :

Reviewers: mp+150414_code.launchpad.net,

Message:
Please take a look.

Description:
Created app subapp extension

https://code.launchpad.net/~hatch/juju-gui/1130787-subapp-app-extension/+merge/150414

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/7381055/

Affected files:
   A [revision details]
   M app/app.js
   A app/assets/javascripts/app-subapp-extension.js
   M app/modules-debug.js
   M bin/merge-files

Reply
Revision history for this message
Jeff Pihach (hatch) wrote :

Please take a look.

https://codereview.appspot.com/7381055/

Reply
Revision history for this message
Benjamin Saller (bcsaller) wrote :

Thanks for the branch.

I have some minor comments below but there are still things I'm not
clear about here.

At minimum it seems that the parent should be the eventTarget of the
subApp when its created? Is that going to live in another branch? I
suppose this branch presents too small view of the final picture for me
to understand how this plays out in reality.

Also is there a use case for subapp removal? I think we can pass on that
for now as well, but we might note this somewhere.

Looking forward to seeing more.

https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-subapp-extension.js
File app/assets/javascripts/app-subapp-extension.js (right):

https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-subapp-extension.js#newcode9
app/assets/javascripts/app-subapp-extension.js:9: value: {}
As we don't currently support removal should this be writeOnce and/or
initOnly?

https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-subapp-extension.js#newcode21
app/assets/javascripts/app-subapp-extension.js:21: config: {}
type -> instance, its more clear and follow the
App.views.viewName.instance model.

this.get('subApps'), this.subApplications and params named subApps all
occur below making things a little harder to follow than they need to
be.

https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-subapp-extension.js#newcode56
app/assets/javascripts/app-subapp-extension.js:56:
this._augmentParentRoutes(routes);
parent seems unnecessary in the naming, _this_ is the parent.

_augmentRoutes or _mixinRoutes?

https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-subapp-extension.js#newcode68
app/assets/javascripts/app-subapp-extension.js:68: @param {array}
subApps an array of sub abb objects and configs.
s/abb/app

https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-subapp-extension.js#newcode71
app/assets/javascripts/app-subapp-extension.js:71: for (var i = 0; i <
subApps.length; i += 1) {
Y.each? We usually favor iterators to native loops.

https://codereview.appspot.com/7381055/

Reply
Revision history for this message
Jeff Pihach (hatch) wrote :

Thanks for the review - I'll make the changes and resubmit

https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-subapp-extension.js
File app/assets/javascripts/app-subapp-extension.js (right):

https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-subapp-extension.js#newcode9
app/assets/javascripts/app-subapp-extension.js:9: value: {}
On 2013/02/25 20:12:13, bcsaller wrote:
> As we don't currently support removal should this be writeOnce and/or
 initOnly?
I understand where you're coming from but I'm not really a fan of adding
that restriction when it doesn't gain us anything - possibly something
to investigate in the future.

https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-subapp-extension.js#newcode21
app/assets/javascripts/app-subapp-extension.js:21: config: {}
On 2013/02/25 20:12:13, bcsaller wrote:
> type -> instance, its more clear and follow the
 App.views.viewName.instance

Sorry yes this documentation is incorrect I will fix
The new syntax mirrors how Y.App does views { type: 'name', ... }
> model.

> this.get('subApps'), this.subApplications and params named subApps all
occur
> below making things a little harder to follow than they need to be.

I'll evaluate a better naming convention as your right this could likely
cause name clashes.

https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-subapp-extension.js#newcode56
app/assets/javascripts/app-subapp-extension.js:56:
this._augmentParentRoutes(routes);
On 2013/02/25 20:12:13, bcsaller wrote:
> parent seems unnecessary in the naming, _this_ is the parent.

> _augmentRoutes or _mixinRoutes?
Agreed, will fix

https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-subapp-extension.js#newcode68
app/assets/javascripts/app-subapp-extension.js:68: @param {array}
subApps an array of sub abb objects and configs.
On 2013/02/25 20:12:13, bcsaller wrote:
> s/abb/app
 will fix

https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-subapp-extension.js#newcode71
app/assets/javascripts/app-subapp-extension.js:71: for (var i = 0; i <
subApps.length; i += 1) {
On 2013/02/25 20:12:13, bcsaller wrote:
> Y.each? We usually favor iterators to native loops.
 Agreed, will fix

https://codereview.appspot.com/7381055/

Reply
Revision history for this message
Jeff Pihach (hatch) wrote :

Please take a look.

https://codereview.appspot.com/7381055/

Reply
Revision history for this message
Gary Poster (gary) wrote :
Download full text (4.2 KiB)

Hi Jeff. I like where this is going. As I said on IRC, my biggest
concern is a lack of tests. I have some small-to-medium sized issues
here as well.

Notice that, because I sat on this review for awhile, the notes are from
an earlier revision. Hopefully they are still mostly worthwhile.

Thanks!

Gary

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js
File app/assets/javascripts/app-subapp-extension.js (right):

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js#newcode1
app/assets/javascripts/app-subapp-extension.js:1: 'use strict';
Please add the yuidoc @module stuff here (see the topology example, for
instance)

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js#newcode5
app/assets/javascripts/app-subapp-extension.js:5: function
SubAppRegistration() {}
Please add a @class directive like the topology example one.

Note that make view-docs is a nice way to double-check that you have
made the docs you want.

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js#newcode25
app/assets/javascripts/app-subapp-extension.js:25: @type {array}
We usually comment these with "*" marks before each line. If YUI doc
does not require this, as far as I am concerned, rejoice!!! And share
the knowledge with the rest of us. But if the doc parser is unhappy,
try adding those.

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js#newcode39
app/assets/javascripts/app-subapp-extension.js:39: Adds the sub
application and it's routes to the parent application.
typo: its

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js#newcode42
app/assets/javascripts/app-subapp-extension.js:42: @param {string}
subApp string referance to an instantiable Y.App object.
typo: reference

I don't understand this comment. The string names a property of the
parent Y.App object that is the factory for the SubApp? No, that's not
it. You get it off of Y? Is this common? Never seen that before. If
I understand correctly that you are supposed to create an application
object hanging off of the Y (framework) object, I don't really like that
on the face of it, but I'll defer to you if you say that you are
confident that this is the YUI way. We have instead made our own
top-level objects, rather than polluting the framework namespace. That
seems safer to me.

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js#newcode77
app/assets/javascripts/app-subapp-extension.js:77: Public method to
refresh routes from the sub apps.
When would you need to call this?

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js#newcode80
app/assets/javascripts/app-subapp-extension.js:80: @param {integer}
index index of the subapp to refresh if undefined
typo: s/index index/index/

...to refresh. If undefined,...

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js#newcode102
app/assets/javascripts/app-subapp-extension.js:102: case 'number'...

Read more...

Reply
Revision history for this message
Jeff Pihach (hatch) wrote :
Download full text (6.0 KiB)

Thanks a bunch for your comments I'll get on them right away! I only
have one follow-up for line 109 so if you could let me know if that was
your expectation that would be great!

Thanks again

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js
File app/assets/javascripts/app-subapp-extension.js (right):

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js#newcode1
app/assets/javascripts/app-subapp-extension.js:1: 'use strict';
On 2013/02/25 21:20:05, gary.poster wrote:
> Please add the yuidoc @module stuff here (see the topology example,
 for
> instance)
 will fix

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js#newcode5
app/assets/javascripts/app-subapp-extension.js:5: function
SubAppRegistration() {}
On 2013/02/25 21:20:05, gary.poster wrote:
> Please add a @class directive like the topology example one.

> Note that make view-docs is a nice way to double-check that you have
made the
> docs you want.
will fix

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js#newcode25
app/assets/javascripts/app-subapp-extension.js:25: @type {array}
On 2013/02/25 21:20:05, gary.poster wrote:
> We usually comment these with "*" marks before each line. If YUI doc
 does not
> require this, as far as I am concerned, rejoice!!! And share the
 knowledge with
> the rest of us. But if the doc parser is unhappy, try adding those.
 YUIdoc doesn't require them it simply requires two stars after the slash
/** docs */ but I may be using a newer version - I'll re-check once I
fix the above missing docs

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js#newcode39
app/assets/javascripts/app-subapp-extension.js:39: Adds the sub
application and it's routes to the parent application.
On 2013/02/25 21:20:05, gary.poster wrote:
> typo: its
 will fix

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js#newcode42
app/assets/javascripts/app-subapp-extension.js:42: @param {string}
subApp string referance to an instantiable Y.App object.
On 2013/02/25 21:20:05, gary.poster wrote:
> typo: reference

> I don't understand this comment. The string names a property of the
parent
> Y.App object that is the factory for the SubApp? No, that's not it.
You get it
> off of Y? Is this common? Never seen that before. If I understand
correctly
> that you are supposed to create an application object hanging off of
the Y
> (framework) object, I don't really like that on the face of it, but
I'll defer
> to you if you say that you are confident that this is the YUI way. We
have
> instead made our own top-level objects, rather than polluting the
framework
> namespace. That seems safer to me.
My thought process behind this was that I wanted to mirror the syntax of
how Y.App deals with views with the syntax { type: 'path.to.view' } to
make it more consistent.

In looking at how the codebase does modules they are almost always hung
off of the Y instance under their own namespace so this allows them to
keep with the same syntax ie) 'juju.subapps.mys...

Read more...

Reply
Revision history for this message
Gary Poster (gary) wrote :
Download full text (5.3 KiB)

Hey Jeff. I made replies to some of your comments.

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js
File app/assets/javascripts/app-subapp-extension.js (right):

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js#newcode42
app/assets/javascripts/app-subapp-extension.js:42: @param {string}
subApp string referance to an instantiable Y.App object.
On 2013/02/25 21:46:59, jeff.pihach wrote:
> On 2013/02/25 21:20:05, gary.poster wrote:
> > typo: reference
> >
> > I don't understand this comment. The string names a property of the
 parent
> > Y.App object that is the factory for the SubApp? No, that's not it.
   You get
> it
> > off of Y? Is this common? Never seen that before. If I understand
 correctly
> > that you are supposed to create an application object hanging off of
 the Y
> > (framework) object, I don't really like that on the face of it, but
 I'll defer
> > to you if you say that you are confident that this is the YUI way.
 We have
> > instead made our own top-level objects, rather than polluting the
 framework
> > namespace. That seems safer to me.
> My thought process behind this was that I wanted to mirror the syntax
 of how
> Y.App deals with views with the syntax { type: 'path.to.view' } to
 make it more
> consistent.

> In looking at how the codebase does modules they are almost always
hung off of
> the Y instance under their own namespace so this allows them to keep
with the
> same syntax ie) 'juju.subapps.mysubapp'

Ah I thought juju was not off of Y. All's well then, thanks.

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js#newcode77
app/assets/javascripts/app-subapp-extension.js:77: Public method to
refresh routes from the sub apps.
On 2013/02/25 21:46:59, jeff.pihach wrote:
> On 2013/02/25 21:20:05, gary.poster wrote:
> > When would you need to call this?
> If you manipulated the routes on a subapp after instantiation you
 would need to
> push those back up to the parent.
> But in all honesty I I created it while prototyping and just left it
 in :-)
Heh, cool. I suggest removing it then. When you write tests, and have
to decide between testing this or removing it, I bet you will prefer
removing it too. :-)

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js#newcode102
app/assets/javascripts/app-subapp-extension.js:102: case 'number':
On 2013/02/25 21:46:59, jeff.pihach wrote:
> On 2013/02/25 21:20:05, gary.poster wrote:
> > Is there really a use case for this? Seems like a YAGNI. If we
 have a use
> > case, never mind
> The idea behind allowing the dev to specify a number was a foreshadow
 into
> future platform differences - they may not want to extract all routes
 on a
> mobile device for example

Huh, ok. I suggest either giving a believable example of how this would
work in a comment, or deleting it as a YAGNI for now. It looks easy to
add back it.

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js#newcode109
app/assets/javascripts/app-subapp-extension.js:109: case 'undefined':
On 2013/02/25 21:46:59, jeff.pi...

Read more...

Reply
Revision history for this message
Benjamin Saller (bcsaller) wrote :

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js
File app/assets/javascripts/app-subapp-extension.js (right):

https://codereview.appspot.com/7381055/diff/1/app/assets/javascripts/app-subapp-extension.js#newcode134
app/assets/javascripts/app-subapp-extension.js:134: if (value.path !==
'*') {
On 2013/02/25 21:59:00, gary.poster wrote:
> On 2013/02/25 21:46:59, jeff.pihach wrote:
> > On 2013/02/25 21:20:05, gary.poster wrote:
> > > hm. I guess this is a reasonable definition of middleware. It
 makes me a
> bit
> > > nervous that there will be conditional middleware of some sort,
 only applied
> > to
> > > certain paths...
> > Maybe I should comment this further - what it's doing is grabbing
 the parent
> > routes and adding any child routes after the 'middleware' which is
 really
> > anything in the parent route with a * path
> Yeah, I understood most of that. That's not a robust definition of
 middleware
> though. Middleware is really something that passes through to more
 routes,
> whether the path is * or /foo/* or even simply /foo. This code
 assumes the
> common case, and slams over less common cases. It makes me nervous
 that it will
> be a source for rare but occasional surprising bugs.

While native Y.App.route doesn't support annotations we do, we could
simply tag routes as `middleware`: true.

Another option is simply to append namespaced routes at the end. Now
that we support qualified matches (requiring the namespace attr to
match) there is no need to place them anywhere but the end. The issue
before was that a default route at the end would match anything
preventing namespaced routes from being matched. Now the default route
would be passed over.

Unless special provision is taken such that namespace attrs on subapp
routes were preserved (rather than set by the binding to App) this
policy would work.

https://codereview.appspot.com/7381055/

Reply
Revision history for this message
Jeff Pihach (hatch) wrote :

Please take a look.

https://codereview.appspot.com/7381055/

Reply
Revision history for this message
Jeff Pihach (hatch) wrote :

Please take a look.

https://codereview.appspot.com/7381055/

Reply
Revision history for this message
Benjamin Saller (bcsaller) wrote :

Thanks for the continued work on this branch.

Personally I think we should switch to a namespaced, append routes
model, that would remove the need to do the placement test that I
suggest below. I think gary would agree that this is simpler and more
foolproof.

https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-subapp-extension.js
File app/assets/javascripts/app-subapp-extension.js (right):

https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-subapp-extension.js#newcode110
app/assets/javascripts/app-subapp-extension.js:110: }
No default still?

https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-subapp-extension.js#newcode126
app/assets/javascripts/app-subapp-extension.js:126: middlewareIndex,
groupedRoutes;
groupedRoutes is unused.

https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extension.js
File test/test_subapp_app_extension.js (right):

https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extension.js#newcode68
test/test_subapp_app_extension.js:68: 'Number of routes does not
match');
With a change to middleware handling we should test that insertion
happens in the currently expected place.

https://codereview.appspot.com/7381055/

Reply
Revision history for this message
Gary Poster (gary) wrote :
Download full text (3.6 KiB)

LGTM with trivials.

Hi Jeff. I have a lot of comments and suggestions and thoughts. The
only thing that's a clear "please do this" request is the trivial
grammatical change ("it's" -> "its").

Everything else is thoughts. What you've done is good and has plenty of
precedent, but I think what we talked about would have been better. If
you'd like to leave it as it is, that's fine; or land it.

Thanks

Gary

https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-subapp-extension.js
File app/assets/javascripts/app-subapp-extension.js (right):

https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-subapp-extension.js#newcode47
app/assets/javascripts/app-subapp-extension.js:47: Adds the sub
application and it's routes to the parent application.
trivial typo, but repeated from previous feedback: you mean "its."

As a matter of policy, we are supposed to go through every comment in a
review and acknowledge the comment, so the reviewer can see that their
effort was recognized, understood, and acted on. For some of us, that
is one of those rules "more honored in the breach" :-) but it really
shouldn't be. Since you missed this (trivial) comment, it might be a
good habit to get into.

https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-subapp-extension.js#newcode54
app/assets/javascripts/app-subapp-extension.js:54: var SubAppObject =
Y.Object.getValue(Y, subApp.split('.')),
I personally think we ought to just pass in factories directly, not
strings. <shrug>

https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-subapp-extension.js#newcode63
app/assets/javascripts/app-subapp-extension.js:63:
That's a lot of whitespace! I think we have a "don't use lots of
whitespace unless you really really want to" kind of non-rule, but could
be wrong. Do what you want here.

https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-subapp-extension.js#newcode64
app/assets/javascripts/app-subapp-extension.js:64:
this._augmentRoutes(routes);
The addSubApp function reads very nicely.

https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-subapp-extension.js#newcode129
app/assets/javascripts/app-subapp-extension.js:129: if (value.path !==
'*') {
I still question this, but <shrug>

https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extension.js
File test/test_subapp_app_extension.js (right):

https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extension.js#newcode16
test/test_subapp_app_extension.js:16: app = new juju.App();
For this level of tests, IMO it would have been nicer to use a generic
App, with your extension mixed in.

https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extension.js#newcode18
test/test_subapp_app_extension.js:18: mocks = {
I thought this was explicitly what you were not going to do! I don't
mind, but IMO the plan we discussed, as driven by the concerns you
expressed, was nicer.

https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extension.js#newcode55
test/test_subapp_app_extension.js:55:
assert.deepEqual(app._extractRoutes(),...

Read more...

Reply
Revision history for this message
Jeff Pihach (hatch) wrote :

Thanks for the reviews! Please see the attached comments

https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-subapp-extension.js
File app/assets/javascripts/app-subapp-extension.js (right):

https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-subapp-extension.js#newcode47
app/assets/javascripts/app-subapp-extension.js:47: Adds the sub
application and it's routes to the parent application.
On 2013/02/26 22:07:25, gary.poster wrote:
> trivial typo, but repeated from previous feedback: you mean "its."

> As a matter of policy, we are supposed to go through every comment in
a review
> and acknowledge the comment, so the reviewer can see that their effort
was
> recognized, understood, and acted on. For some of us, that is one of
those
> rules "more honored in the breach" :-) but it really shouldn't be.
Since you
> missed this (trivial) comment, it might be a good habit to get into.
Fixed.

https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-subapp-extension.js#newcode54
app/assets/javascripts/app-subapp-extension.js:54: var SubAppObject =
Y.Object.getValue(Y, subApp.split('.')),
On 2013/02/26 22:07:25, gary.poster wrote:
> I personally think we ought to just pass in factories directly, not
 strings.
> <shrug>
 No problem - consider it changed.

https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-subapp-extension.js#newcode63
app/assets/javascripts/app-subapp-extension.js:63:
On 2013/02/26 22:07:25, gary.poster wrote:
> That's a lot of whitespace! I think we have a "don't use lots of
 whitespace
> unless you really really want to" kind of non-rule, but could be
 wrong. Do what
> you want here.
 I collapsed them into like commands to tighten it up a bit.

https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-subapp-extension.js#newcode64
app/assets/javascripts/app-subapp-extension.js:64:
this._augmentRoutes(routes);
On 2013/02/26 22:07:25, gary.poster wrote:
> The addSubApp function reads very nicely.
 Thanks

https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-subapp-extension.js#newcode110
app/assets/javascripts/app-subapp-extension.js:110: }
On 2013/02/26 22:00:56, bcsaller wrote:
> No default still?
 Default has now been added.

https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-subapp-extension.js#newcode126
app/assets/javascripts/app-subapp-extension.js:126: middlewareIndex,
groupedRoutes;
On 2013/02/26 22:00:56, bcsaller wrote:
> groupedRoutes is unused.
 Poof it's gone!

https://codereview.appspot.com/7381055/diff/16001/app/assets/javascripts/app-subapp-extension.js#newcode129
app/assets/javascripts/app-subapp-extension.js:129: if (value.path !==
'*') {
On 2013/02/26 22:07:25, gary.poster wrote:
> I still question this, but <shrug>
 I'm going to leave this insertion method in here for now - at least
until we get the namespace code separated out and a functional sub app
using the code - it's trivial to change but I know that this works for
the prototype use case :-)

https://codereview.appspot.com/7381055/

Reply
Revision history for this message
Gary Poster (gary) wrote :

On 2013/02/27 17:04:29, jeff.pihach wrote:
> Thanks for the reviews! Please see the attached comments

:-) thanks, sounds good.

https://codereview.appspot.com/7381055/

Reply
Revision history for this message
Benjamin Saller (bcsaller) wrote :

LGTM

I think this is in shape to land so we can move forward with it. I still
have some reservations about how it all fits together, but I'm convinced
we will be able to make this work.

Thanks again.

https://codereview.appspot.com/7381055/

Reply
Revision history for this message
Jeff Pihach (hatch) wrote :

Hey sorry guys I forgot to 'save' my comments on the test page - please
see those comments below.

https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extension.js
File test/test_subapp_app_extension.js (right):

https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extension.js#newcode16
test/test_subapp_app_extension.js:16: app = new juju.App();
On 2013/02/26 22:07:25, gary.poster wrote:
> For this level of tests, IMO it would have been nicer to use a generic
 App, with
> your extension mixed in.
 Sure thing - no problem I can switch that over.

https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extension.js#newcode18
test/test_subapp_app_extension.js:18: mocks = {
On 2013/02/26 22:07:25, gary.poster wrote:
> I thought this was explicitly what you were not going to do! I don't
 mind, but
> IMO the plan we discussed, as driven by the concerns you expressed,
 was nicer.
True however in practice it ended up being another level of abstraction
which was only valuable in testing and created needless complexity
everywhere else so I decided to bench those changes.

https://codereview.appspot.com/7381055/diff/16001/test/test_subapp_app_extension.js#newcode68
test/test_subapp_app_extension.js:68: 'Number of routes does not
match');
On 2013/02/26 22:07:25, gary.poster wrote:
> Another deepEqual, this time of the trailing routes, would have been
 cool. This
> is alright though, given the previous test.
 Because I'm benching changing the insertion point of the routes I am
going to leave this as is in conjunction with the above test. I foresee
changes in it's future...

also I can't deep equal here because the routes are augmented with quite
a bit of meta data that I am unable to mock. I'll have to investigate a
better way to do this once we have a fleshed out subapp

https://codereview.appspot.com/7381055/

Reply
Revision history for this message
Jeff Pihach (hatch) wrote :

Please take a look.

https://codereview.appspot.com/7381055/

Reply
Revision history for this message
Gary Poster (gary) wrote :

LGTM still--looks even better. :-)

Thanks

Gary

https://codereview.appspot.com/7381055/diff/23001/app/assets/javascripts/app-subapp-extension.js
File app/assets/javascripts/app-subapp-extension.js (right):

https://codereview.appspot.com/7381055/diff/23001/app/assets/javascripts/app-subapp-extension.js#newcode50
app/assets/javascripts/app-subapp-extension.js:50: @param {string}
SubApp string referance to an instantiable Y.App object.
No longer a string reference

https://codereview.appspot.com/7381055/diff/23001/test/test_subapp_app_extension.js
File test/test_subapp_app_extension.js (right):

https://codereview.appspot.com/7381055/diff/23001/test/test_subapp_app_extension.js#newcode12
test/test_subapp_app_extension.js:12: [Y.juju.SubAppRegistration], {});
Cool.

https://codereview.appspot.com/7381055/

Reply
Revision history for this message
Jeff Pihach (hatch) wrote :

*** Submitted:

Created app subapp extension

R=bcsaller, gary.poster
CC=
https://codereview.appspot.com/7381055

https://codereview.appspot.com/7381055/

Reply
Revision history for this message
Jeff Pihach (hatch) wrote :

Please take a look.

https://codereview.appspot.com/7381055/

Reply

Unmerged revisions

415. By Jeff Pihach

calling the subapp route callback in the proper context

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'app/app.js'
2--- app/app.js 2013-02-27 16:38:40 +0000
3+++ app/app.js 2013-02-27 17:50:27 +0000
4@@ -27,7 +27,7 @@
5 *
6 * @class App
7 */
8- var JujuGUI = Y.Base.create('juju-gui', Y.App, [], {
9+ var JujuGUI = Y.Base.create('juju-gui', Y.App, [Y.juju.SubAppRegistration], {
10
11 /*
12 * Views
13@@ -1194,6 +1194,11 @@
14 'app-transitions',
15 'base',
16 'node',
17+<<<<<<< TREE
18 'model',
19 'sub-app']
20+=======
21+ 'model',
22+ 'app-subapp-extension']
23+>>>>>>> MERGE-SOURCE
24 });
25
26=== added file 'app/assets/javascripts/app-subapp-extension.js'
27--- app/assets/javascripts/app-subapp-extension.js 1970-01-01 00:00:00 +0000
28+++ app/assets/javascripts/app-subapp-extension.js 2013-02-27 17:50:27 +0000
29@@ -0,0 +1,144 @@
30+'use strict';
31+
32+YUI.add('app-subapp-extension', function(Y) {
33+
34+ /**
35+ Adds the ability to register sub applications with a parent Y.App instance by
36+ recording all instances in a subApps attribute and augmenting the parent
37+ routes with the sub applications.
38+
39+ @class SubAppRegistration
40+ @extension App
41+ */
42+ function SubAppRegistration() {}
43+
44+ SubAppRegistration.ATTRS = {
45+ subApps: {
46+ value: {}
47+ }
48+ };
49+
50+ SubAppRegistration.prototype = {
51+
52+ /**
53+ A list of sub applications to be instantiated after initialization
54+ of the parent app.
55+
56+ [{
57+ type: path.to.subapp
58+ config: {}
59+ }]
60+
61+ @property subApplications
62+ @type {array}
63+ */
64+ subApplications: [],
65+
66+ /**
67+ Adds all of the sub applications listed in the subApplications property.
68+
69+ @method addSubApplications
70+ */
71+ addSubApplications: function() {
72+ this.addSubApps(this.subApplications);
73+ },
74+
75+ /**
76+ Adds the sub application and its routes to the parent application.
77+
78+ @method addSubApp
79+ @param {string} SubApp string referance to an instantiable Y.App object.
80+ @param {object} config configuration properties for the subapp.
81+ */
82+ addSubApp: function(SubApp, config) {
83+ var subApps = this.get('subApps'),
84+ routes;
85+
86+ SubApp = new SubApp(config);
87+ subApps[SubApp.get('urlNamespace')] = SubApp;
88+
89+ routes = this._extractRoutes(SubApp);
90+ this._augmentRoutes(routes);
91+ },
92+
93+ /**
94+ Wrapper for addSubApp to add multiple sub apps at once.
95+
96+ [{
97+ type: 'path.to.subapplication'
98+ config: {}
99+ }]
100+
101+ @method addSubApps
102+ @param {array} subApps an array of sub app objects and configs.
103+ */
104+ addSubApps: function(subApps) {
105+ Y.Array.each(subApps, function(subApp) {
106+ this.addSubApp(subApp.type, subApp.config);
107+ }, this);
108+ },
109+
110+ /**
111+ Extract the routes out of the sub apps.
112+
113+ @method _extractRoutes
114+ @protected
115+ @param {object | integer | undefined} subApp will extract the routes
116+ out of the supplied subApp, index, or all subApps if undefined.
117+ @return {array} array of sub app route data.
118+ */
119+ _extractRoutes: function(subApp) {
120+ var subApps = this.get('subApps'),
121+ routes, subRoutes, i, j;
122+
123+ switch (typeof subApp) {
124+ case 'number': // If an index is passed in grab that subapp
125+ subApp = subApps[subApp];
126+ /* falls through */
127+ case 'object': // If subapp is passed in fetch it's routes
128+ routes = subApp.getSubAppRoutes();
129+ break;
130+ case 'undefined':
131+ routes = [];
132+ for (i = 0; i < subApps.length; i += 1) {
133+ subRoutes = subApps[i].getSubAppRoutes();
134+ for (j = 0; j < subRoutes.length; j += 1) {
135+ routes.push(subRoutes[j]);
136+ }
137+ }
138+ break;
139+ default:
140+ throw 'subApp valid types are integer, object, or undefined';
141+ }
142+ return routes;
143+ },
144+
145+ /**
146+ Adds the sub app routes to the parent routes after the middleware
147+
148+ @method _augmentRoutes
149+ @protected
150+ @param {array} array of route objects.
151+ */
152+ _augmentRoutes: function(routes) {
153+ var parentRoutes = this.get('routes'),
154+ middlewareIndex;
155+
156+ Y.Array.some(parentRoutes, function(value, index, array) {
157+ if (value.path !== '*') {
158+ middlewareIndex = index;
159+ return true;
160+ }
161+ });
162+
163+ routes.unshift(middlewareIndex, 0);
164+ Array.prototype.splice.apply(parentRoutes, routes);
165+
166+ this.set('routes', parentRoutes);
167+ return parentRoutes;
168+ }
169+ };
170+
171+ Y.namespace('juju').SubAppRegistration = SubAppRegistration;
172+
173+}, '0.1.0');
174
175=== modified file 'app/modules-debug.js'
176--- app/modules-debug.js 2013-02-25 19:53:33 +0000
177+++ app/modules-debug.js 2013-02-27 17:50:27 +0000
178@@ -56,10 +56,17 @@
179 fullpath: '/juju-ui/assets/javascripts/routing.js'
180 },
181
182+<<<<<<< TREE
183 'sub-app': {
184 fullpath: '/juju-ui/assets/javascripts/sub-app.js'
185 },
186
187+=======
188+ 'app-subapp-extension': {
189+ fullpath: '/juju-ui/assets/javascripts/app-subapp-extension.js'
190+ },
191+
192+>>>>>>> MERGE-SOURCE
193 // Views
194 'juju-landscape': {
195 fullpath: '/juju-ui/views/landscape.js'
196
197=== modified file 'bin/merge-files'
198--- bin/merge-files 2013-02-25 19:53:33 +0000
199+++ bin/merge-files 2013-02-27 17:50:27 +0000
200@@ -75,8 +75,13 @@
201 'app/assets/javascripts/routing.js',
202 'app/assets/javascripts/gallery-ellipsis.js',
203 'app/assets/javascripts/gallery-markdown.js',
204+<<<<<<< TREE
205 'app/assets/javascripts/gallery-timer.js',
206 'app/assets/javascripts/sub-app.js']);
207+=======
208+ 'app/assets/javascripts/gallery-timer.js',
209+ 'app/assets/javascripts/app-subapp-extension.js']);
210+>>>>>>> MERGE-SOURCE
211
212 merge.combineJs(filesToLoad.js, 'build-shared/juju-ui/assets/all-yui.js');
213
214
215=== modified file 'test/index.html'
216--- test/index.html 2013-02-26 19:29:43 +0000
217+++ test/index.html 2013-02-27 17:50:27 +0000
218@@ -56,7 +56,11 @@
219 <script src="test_service_module.js"></script>
220 <script src="test_service_view.js"></script>
221 <script src="test_startup.js"></script>
222+<<<<<<< TREE
223 <script src="test_sub_app.js"></script>
224+=======
225+ <script src="test_subapp_app_extension.js"></script>
226+>>>>>>> MERGE-SOURCE
227 <script src="test_templates.js"></script>
228 <script src="test_topology.js"></script>
229 <script src="test_topology_relation.js"></script>
230
231=== added file 'test/test_subapp_app_extension.js'
232--- test/test_subapp_app_extension.js 1970-01-01 00:00:00 +0000
233+++ test/test_subapp_app_extension.js 2013-02-27 17:50:27 +0000
234@@ -0,0 +1,77 @@
235+'use strict';
236+
237+describe('SubApplication App Extension', function() {
238+ var Y, juju, app, mocks, MockApp;
239+
240+ before(function(done) {
241+ Y = YUI(GlobalConfig).use(['juju-routing',
242+ 'juju-gui', 'app-subapp-extension'],
243+ function(Y) {
244+ juju = Y.namespace('juju');
245+ MockApp = Y.Base.create('mock-app', Y.App,
246+ [Y.juju.SubAppRegistration], {});
247+ done();
248+ });
249+
250+ });
251+
252+ beforeEach(function() {
253+ app = new MockApp();
254+
255+ Y.namespace('mock');
256+
257+ mocks = {
258+ subAppRoutes: [
259+ { path: '/', callbacks: 'showRootView', namespace: 'charmStore' },
260+ { path: '/charm/:id', callbacks: 'showCharmDetailView',
261+ namespace: 'charmStore' }
262+ ],
263+ parentAppRoutes: [
264+ { path: '*', callbacks: 'check_user_credentials' },
265+ { path: '*', callbacks: 'show_notifications_view' },
266+ { path: '/charms/', callbacks: 'show_charm_collection' },
267+ { path: '/charms/*charm_store_path/', callbacks: 'show_charm' }
268+ ]
269+ };
270+
271+ function subAppMock() {}
272+ subAppMock.prototype.getSubAppRoutes = function() {
273+ return mocks.subAppRoutes;
274+ };
275+ subAppMock.prototype.get = function(attribute) {
276+ if (attribute === 'urlNamespace') { return 'charmStore'; }
277+ };
278+ Y.mock.subapp = subAppMock;
279+
280+ mocks.subAppProperty = {
281+ type: Y.mock.subapp,
282+ config: {}
283+ };
284+ });
285+
286+ it('should add subapps to the parent app', function() {
287+ app.set('routes', mocks.parentAppRoutes);
288+ app.addSubApp(mocks.subAppProperty.type, mocks.subAppProperty.config);
289+ var subapps = app.get('subApps');
290+ assert(typeof subapps.charmStore === 'object');
291+ });
292+
293+ it('should extract the routes from the subapp', function() {
294+ app.set('subApps', [new Y.mock.subapp()]);
295+ assert.deepEqual(app._extractRoutes(), mocks.subAppRoutes,
296+ 'Routes do not match');
297+ });
298+
299+ it('should augment the parent routes with the subapp routes', function() {
300+ var augmentedRoutes, numberOfRoutes;
301+
302+ numberOfRoutes = mocks.subAppRoutes.length + mocks.parentAppRoutes.length;
303+ app.set('routes', mocks.parentAppRoutes);
304+
305+ augmentedRoutes = app._augmentRoutes(mocks.subAppRoutes),
306+
307+ assert.equal(augmentedRoutes.length, numberOfRoutes,
308+ 'Number of routes does not match');
309+ });
310+
311+});

Subscribers

People subscribed via source and target branches