Created Y.SubApp class by extending Y.App

Created Y.SubApp class by extending Y.App



https://code.launchpad.net/~hatch/juju-gui/1130790-extend-app-subapp/+merge/150421

(do not edit description out of merge proposal)

[jeff.pihach, 2013-02-25 19:57:34]

Please take a look.

[gary.poster, 2013-02-25 21:47:56]

Hi Jeff.  Looks like a good direction.  It needs tests, as we've said, and I
don't yet see how the route callables are bound the way I thought we wanted them
to be.  Otherwise good.

Thanks

Gary

https://codereview.appspot.com/7384059/diff/1/app/assets/javascripts/sub-app.js
File app/assets/javascripts/sub-app.js (right):

https://codereview.appspot.com/7384059/diff/1/app/assets/javascripts/sub-app....
app/assets/javascripts/sub-app.js:6: Base class to create sub apps.
Same comment as previous branch: usually, we prefix these doc lines with "*". 
If we don't have to, great: please let us know so we don't have to do it either.
:-)

https://codereview.appspot.com/7384059/diff/1/app/assets/javascripts/sub-app....
app/assets/javascripts/sub-app.js:50: routes[i].namespace = namespace;
I thought we needed to make the callables found off of the sub-app also, yeah? 
Or does that happen elsewhere?

[jeff.pihach, 2013-02-25 21:56:09]

Thanks for the review - I added a link to where I am binding the callbacks for
the subapp

https://codereview.appspot.com/7384059/diff/1/app/assets/javascripts/sub-app.js
File app/assets/javascripts/sub-app.js (right):

https://codereview.appspot.com/7384059/diff/1/app/assets/javascripts/sub-app....
app/assets/javascripts/sub-app.js:50: routes[i].namespace = namespace;
On 2013/02/25 21:47:56, gary.poster wrote:
> I thought we needed to make the callables found off of the sub-app also, yeah?

> Or does that happen elsewhere?
I'm doing it in the dispatch method - this way we only bind callbacks that are
actually used. Now that you mention it though I should probably implement some
type of caching mechanism. 

http://bazaar.launchpad.net/~hatch/juju-gui/1130790-create-subapp/view/head:/...

[bcsaller, 2013-02-26 01:06:59]

https://codereview.appspot.com/7384059/diff/1/app/assets/javascripts/sub-app.js
File app/assets/javascripts/sub-app.js (right):

https://codereview.appspot.com/7384059/diff/1/app/assets/javascripts/sub-app....
app/assets/javascripts/sub-app.js:50: routes[i].namespace = namespace;
On 2013/02/25 21:56:09, jeff.pihach wrote:
> On 2013/02/25 21:47:56, gary.poster wrote:
> > I thought we needed to make the callables found off of the sub-app also,
yeah?
> 
> > Or does that happen elsewhere?
> I'm doing it in the dispatch method - this way we only bind callbacks that are
> actually used. Now that you mention it though I should probably implement some
> type of caching mechanism. 
> 
>
http://bazaar.launchpad.net/%7Ehatch/juju-gui/1130790-create-subapp/view/head...

The 'normal' router behavior resolves these at runtime. Its the cost of an
attribute lookup, happening at dispatch. I don't think you need to do anything
special.

[jeff.pihach, 2013-02-26 19:30:50]

Please take a look.

[jeff.pihach, 2013-02-26 19:37:04]

Please take a look.

[bcsaller, 2013-02-26 22:01:00]

Thanks for the branch, I have some concerns still but if you tell me the
prototype proved the theory to a greater extent than this show I'm happy to talk
about it.

https://codereview.appspot.com/7384059/diff/12002/app/assets/javascripts/sub-...
File app/assets/javascripts/sub-app.js (right):

https://codereview.appspot.com/7384059/diff/12002/app/assets/javascripts/sub-...
app/assets/javascripts/sub-app.js:34: options: options
I doubt handling just this is enough to make this system work, we are still
subclassing Router and PjaxBase which tie into history, add push/popState
listeners, etc. 

It seems like App should really move the bulk of its methods into a ViewManager
extension and then app-base could use that. We'd only want [View, ViewManager]
for subapps but I'm  not even sure how much of ViewManager makes sense w/o the
ability to control the URL or manage history.

https://codereview.appspot.com/7384059/diff/12002/app/assets/javascripts/sub-...
app/assets/javascripts/sub-app.js:49: for (i = 0; i < routes.length; i += 1) {
We usually use iterators for this.

https://codereview.appspot.com/7384059/diff/12002/app/assets/javascripts/sub-...
app/assets/javascripts/sub-app.js:76: index: {}
I think this is unused.

[gary.poster, 2013-02-26 22:35:45]

LGTM.  Tests are short but good.  Thanks!

Gary

https://codereview.appspot.com/7384059/diff/12002/app/assets/javascripts/sub-...
File app/assets/javascripts/sub-app.js (right):

https://codereview.appspot.com/7384059/diff/12002/app/assets/javascripts/sub-...
app/assets/javascripts/sub-app.js:34: options: options
On 2013/02/26 22:01:00, bcsaller wrote:
> I doubt handling just this is enough to make this system work, we are still
> subclassing Router and PjaxBase which tie into history, add push/popState
> listeners, etc. 

I appreciate Ben's concern, and this reflects his original desire to avoid using
Y.App.Base.  I hadn't realized that Router and PjaxBase were mixed in.  I do
like how simple this looks so far though.

I tried to investigate whether we could mitigate this trivially.  PjaxBase does
not integrate if you set html5 to false explicitly, before its initializer is
called.  Maybe that would help? 
http://yuilibrary.com/yui/docs/api/files/pjax_js_pjax-base.js.html#l101

That also turns off the html 5 router connection.
http://yuilibrary.com/yui/docs/api/files/app_js_router.js.html#l160
I suspect/hope that this is less pertinent as long as it is clear that SubApp
users are not supposed to call methods that SubApp does not specifically bless.

We could mask the non-blessed ones, I guess, but having tried that in the past,
that never felt very nice.  We could do a whitelist approach, but this moves
farther and farther into hack land at that point.

> 
> It seems like App should really move the bulk of its methods into a
ViewManager
> extension and then app-base could use that. We'd only want [View, ViewManager]
> for subapps but I'm  not even sure how much of ViewManager makes sense w/o the
> ability to control the URL or manage history.

It would be interesting to see what we actually use of the App.Base in the
expected usage of a SubApp.

https://codereview.appspot.com/7384059/diff/12002/app/assets/javascripts/sub-...
app/assets/javascripts/sub-app.js:49: for (i = 0; i < routes.length; i += 1) {
On 2013/02/26 22:01:00, bcsaller wrote:
> We usually use iterators for this.
+1, unless Jeff has a counter-argument

https://codereview.appspot.com/7384059/diff/12002/test/test_sub_app.js
File test/test_sub_app.js (right):

https://codereview.appspot.com/7384059/diff/12002/test/test_sub_app.js#newcode44
test/test_sub_app.js:44: }) === false);
You threw me for a loop there!  But I caught up at the end. :-)

[jeff.pihach, 2013-02-27 05:54:01]

Thanks a lot for the reviews! I have added some comments in reply and will be
making the noted changes.

https://codereview.appspot.com/7384059/diff/12002/app/assets/javascripts/sub-...
File app/assets/javascripts/sub-app.js (right):

https://codereview.appspot.com/7384059/diff/12002/app/assets/javascripts/sub-...
app/assets/javascripts/sub-app.js:34: options: options
On 2013/02/26 22:35:45, gary.poster wrote:
> On 2013/02/26 22:01:00, bcsaller wrote:
> > I doubt handling just this is enough to make this system work, we are still
> > subclassing Router and PjaxBase which tie into history, add push/popState
> > listeners, etc. 
> 
> I appreciate Ben's concern, and this reflects his original desire to avoid
using
> Y.App.Base.  I hadn't realized that Router and PjaxBase were mixed in.  I do
> like how simple this looks so far though.
> 
> I tried to investigate whether we could mitigate this trivially.  PjaxBase
does
> not integrate if you set html5 to false explicitly, before its initializer is
> called.  Maybe that would help? 
> http://yuilibrary.com/yui/docs/api/files/pjax_js_pjax-base.js.html#l101
> 
> That also turns off the html 5 router connection.
> http://yuilibrary.com/yui/docs/api/files/app_js_router.js.html#l160
> I suspect/hope that this is less pertinent as long as it is clear that SubApp
> users are not supposed to call methods that SubApp does not specifically
bless.
> 
> We could mask the non-blessed ones, I guess, but having tried that in the
past,
> that never felt very nice.  We could do a whitelist approach, but this moves
> farther and farther into hack land at that point.
> 
> > 
> > It seems like App should really move the bulk of its methods into a
> ViewManager
> > extension and then app-base could use that. We'd only want [View,
ViewManager]
> > for subapps but I'm  not even sure how much of ViewManager makes sense w/o
the
> > ability to control the URL or manage history.
> 
> It would be interesting to see what we actually use of the App.Base in the
> expected usage of a SubApp.
I agree that by subclassing app.base we are including a lot of unnecessary code
but it is looking like 3.10 will bring with it a new App.base which only has the
base app methods at which point we will simply be able to upgrade the version
and all of the unnecessary code will be gone. 

By extending app.base it's my goal to maintain a consistent api across the whole
application and be able to take advantage of library improvements as additional
work is done on it.

In the prototype using this new navigate method was enough to mitigate any
further issues but it's entirely possible that once people start using it we
will run into other use cases which will be easy to tackle at that time.

https://codereview.appspot.com/7384059/diff/12002/app/assets/javascripts/sub-...
app/assets/javascripts/sub-app.js:49: for (i = 0; i < routes.length; i += 1) {
On 2013/02/26 22:35:45, gary.poster wrote:
> On 2013/02/26 22:01:00, bcsaller wrote:
> > We usually use iterators for this.
> +1, unless Jeff has a counter-argument
No counter here - I'll be switching this over

https://codereview.appspot.com/7384059/diff/12002/app/assets/javascripts/sub-...
app/assets/javascripts/sub-app.js:76: index: {}
On 2013/02/26 22:01:00, bcsaller wrote:
> I think this is unused.
Yes it appears your right, consider it gone!

https://codereview.appspot.com/7384059/diff/12002/test/test_sub_app.js
File test/test_sub_app.js (right):

https://codereview.appspot.com/7384059/diff/12002/test/test_sub_app.js#newcode44
test/test_sub_app.js:44: }) === false);
On 2013/02/26 22:35:45, gary.poster wrote:
> You threw me for a loop there!  But I caught up at the end. :-)
Sorry about that :-)

[jeff.pihach, 2013-02-27 14:51:53]

*** Submitted:

Created Y.SubApp class by extending Y.App

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