Namespace aware routing

Namespace aware routing

Add namespace aware routing to app. This is forward looking. It adds support
for namespaced urls where these are defined as /:ns:/urlFragment/:ns2:/etc.

Each namespace in the url will be preserved and dispatched to accordingly.

Middleware should only fire once for a given request.

https://code.launchpad.net/~bcsaller/juju-gui/namespace-routing/+merge/148365

(do not edit description out of merge proposal)

[bcsaller, 2013-02-14 12:50:46]

Please take a look.

[bcsaller, 2013-02-14 13:19:56]

Please take a look.

[gary.poster, 2013-02-14 14:00:09]

I only got halfway through routing.js, but wanted to share what I had before I
went into calls.  No one should wait for me: please don't count me for the two
reviews today.  However, I will return to this if I can.

Gary

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routi...
File app/assets/javascripts/routing.js (right):

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routi...
app/assets/javascripts/routing.js:5: // particuallary around text
copy and paste error: please update comment to describe what this actually does
(and the old comment has a typo, "particularly," but the associated module has
been removed from trunk).

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routi...
app/assets/javascripts/routing.js:7: function _trim(s, char, leading, trailing)
{
I don't understand yet why this only trims a single character.  My suspicion is
that, as I read, I'll understand that this is all you need, and otherwise you
would have been in the "oh my gosh JS doesn't have a built-in regex excape
function" place.

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routi...
app/assets/javascripts/routing.js:20: function ltrim(s, char) { return _trim(s,
char, true, false); }
Wouldn't it be nice if the built-in trim were flexible enough for what you need!

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routi...
app/assets/javascripts/routing.js:23: function pairs(o) {
docstring would help with easy reading.

Return sorted array of object attribute pairs (name, value).

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routi...
app/assets/javascripts/routing.js:37: // Multi dimensional router (TARDIS).
lol

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routi...
app/assets/javascripts/routing.js:49: return parts[0];
trivial, but when placed next to your getQS implementation, "return
url.split('?')[0];" looks pretty reasonable.

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routi...
app/assets/javascripts/routing.js:65: var parts = url.split(this.fragment);
Might be helpful to include examples in comments:

> '/foo/bar'.split(/\/?(:\w+\:)/)
["/foo/bar"]
'/> :baz:/foo/bar'.split(/\/?(:\w+\:)/)
["", ":baz:", "/foo/bar"]

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routi...
app/assets/javascripts/routing.js:67: // This is a URL fragment w/o a namespace
Took me a second to agree, but yes, nice.

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routi...
app/assets/javascripts/routing.js:68: result.charmstore = parts[0];
wait a second, that's not general-purpose! :-)

I don't see the need for this to be general purpose.  I suggest moving it into
app/routing.js.  Alternatively, if you really want to, clean this up.

[bcsaller, 2013-02-14 15:12:53]

Made changes around your suggestions, but will wait for other reviews

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routi...
File app/assets/javascripts/routing.js (right):

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routi...
app/assets/javascripts/routing.js:37: // Multi dimensional router (TARDIS).
On 2013/02/14 14:00:09, gary.poster wrote:
> lol

I forgot I left that in there. :)

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routi...
app/assets/javascripts/routing.js:49: return parts[0];
On 2013/02/14 14:00:09, gary.poster wrote:
> trivial, but when placed next to your getQS implementation, "return
> url.split('?')[0];" looks pretty reasonable.

Done.

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routi...
app/assets/javascripts/routing.js:65: var parts = url.split(this.fragment);
On 2013/02/14 14:00:09, gary.poster wrote:
> Might be helpful to include examples in comments:
> 
> > '/foo/bar'.split(/\/?(:\w+\:)/)
> ["/foo/bar"]
> '/> :baz:/foo/bar'.split(/\/?(:\w+\:)/)
> ["", ":baz:", "/foo/bar"]

Done.

https://codereview.appspot.com/7327045/diff/3001/app/assets/javascripts/routi...
app/assets/javascripts/routing.js:68: result.charmstore = parts[0];
On 2013/02/14 14:00:09, gary.poster wrote:
> wait a second, that's not general-purpose! :-)
> 
> I don't see the need for this to be general purpose.  I suggest moving it into
> app/routing.js.  Alternatively, if you really want to, clean this up.

Added this.defaultNamespace and use that in place.

[bcsaller, 2013-02-14 15:16:20]

Please take a look.

[matthew.scott, 2013-02-14 17:58:27]

Some minors below.  The code looks fine for the most part, with the queue
addition.  However, I can't get the app to load past the "trying to connect"
screen.  I'm seeing deltas in the websocket frames, but nothing going on in the
app.  Will try uncommenting logs (so don't get rid of them yet :) to see what's
going on.

https://codereview.appspot.com/7327045/diff/3001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7327045/diff/3001/app/app.js#newcode346
app/app.js:346: //console.group('dispatch');
Remove commented code.

https://codereview.appspot.com/7327045/diff/3001/app/app.js#newcode381
app/app.js:381: // Null-queue for NS routing. The 1ms delay in the queue
presents
Very minor, should be a /*...*/ style comment - will make it easy to yuidoc down
the road.  Or it could just be yuidoc'd, if you want.

https://codereview.appspot.com/7327045/diff/3001/app/app.js#newcode395
app/app.js:395: var loc = Y.getLocation();
Very minor: could be moved to top of function and used in line 391.

https://codereview.appspot.com/7327045/diff/3001/app/app.js#newcode404
app/app.js:404: /**
We should note that this comes from App.Base, along with what has changed.

https://codereview.appspot.com/7327045/diff/3001/app/app.js#newcode996
app/app.js:996: * dispatches once per any dispatch call wrt namespace
with regards to

https://codereview.appspot.com/7327045/diff/9001/app/assets/javascripts/routi...
File app/assets/javascripts/routing.js (right):

https://codereview.appspot.com/7327045/diff/9001/app/assets/javascripts/routi...
app/assets/javascripts/routing.js:48: * normalize a url w/o its qs.
Normalize, without

https://codereview.appspot.com/7327045/diff/9001/app/assets/javascripts/routi...
app/assets/javascripts/routing.js:50: * @return {Object} {url: string, qs:
querystring}.
Should be returning just the URL, correct?

[bcsaller, 2013-02-14 20:42:24]

thanks for the review, pushing the updates.

https://codereview.appspot.com/7327045/diff/3001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7327045/diff/3001/app/app.js#newcode381
app/app.js:381: // Null-queue for NS routing. The 1ms delay in the queue
presents
On 2013/02/14 17:58:27, matthew.scott wrote:
> Very minor, should be a /*...*/ style comment - will make it easy to yuidoc
down
> the road.  Or it could just be yuidoc'd, if you want.

Something in me fights doing yuidoc for internal methods and overrides that
should never be called, but I can change it.

https://codereview.appspot.com/7327045/diff/3001/app/app.js#newcode395
app/app.js:395: var loc = Y.getLocation();
On 2013/02/14 17:58:27, matthew.scott wrote:
> Very minor: could be moved to top of function and used in line 391.

Done.

https://codereview.appspot.com/7327045/diff/3001/app/app.js#newcode404
app/app.js:404: /**
On 2013/02/14 17:58:27, matthew.scott wrote:
> We should note that this comes from App.Base, along with what has changed.

Done.

https://codereview.appspot.com/7327045/diff/9001/app/assets/javascripts/routi...
File app/assets/javascripts/routing.js (right):

https://codereview.appspot.com/7327045/diff/9001/app/assets/javascripts/routi...
app/assets/javascripts/routing.js:48: * normalize a url w/o its qs.
On 2013/02/14 17:58:27, matthew.scott wrote:
> Normalize, without

Done.

https://codereview.appspot.com/7327045/diff/9001/app/assets/javascripts/routi...
app/assets/javascripts/routing.js:50: * @return {Object} {url: string, qs:
querystring}.
On 2013/02/14 17:58:27, matthew.scott wrote:
> Should be returning just the URL, correct?

Done.

[bcsaller, 2013-02-14 20:45:47]

Please take a look.

[bcsaller, 2013-02-15 14:06:47]

Please take a look.

[bcsaller, 2013-02-15 14:10:12]

There was an issue with the login-mask renaming that I somehow missed in the
merge, resovled now.

[gary.poster, 2013-02-15 18:19:40]

Hi Ben.  This is a very impressive branch.

In terms of usability, I think we need a navigation spelling or other aid to let
one dimension easily construct a url that keeps all current aspects of the url
except one.  AFAICT, that does not exist.  I think we would need an exposed API
call to construct a url only changing a given namespace + path.  Some other
convenience functions to use that API might be nice too (like to navigate within
the app).

I'd be happy to talk through this on a call if that would help speed things up,
either to understand my point or to convince me that it is unnecessary. :-)

I'm not clear on how this prevents re-dispatching along a given dimension that
does not change between two urls--that is, /:foo:/bar/:bing:/baz ->
/:foo:/bar/:bing:/shazam should not redispatch along the :foo: namespace, if I
understand your goals.  Again, maybe that's best for a call, though I might ask
for explanatory comments somewhere afterwards.

Note that I suggest you read my per-line comments through once before responding
item by item--I eventually come to understand things (maybe?) that I question
initially.

Also, please forgive my many comments.  This is a meaty branch, and I decided to
let myself selfishly annotate as I went.  The annotations themselves may or may
not indicate room for clarity in the code.

Thank you!

Gary

https://codereview.appspot.com/7327045/diff/14001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode237
app/app.js:237: this._routeGeneration = 0;
This is not a request.  It's a mild suggestion.

These four attributes are a bit mysterious on their own.  Comments describing
their intent would help the reader along, I think.

For this particular attribute and the next, I see this is needed in
_routeStateTracker and updated elsewhere.  Obviously it is some kind of uid
mechanism but I don't understand the details yet as of this writing.

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode239
app/app.js:239: this._nsPath = null;
AIUI, this is used to help override _getPath when we are in the parent class'
dispatch, which is run for each namespace.

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode240
app/app.js:240: this._nsRouter = juju.Router();
"The router provides the logic to parse a multi-dimensional url into its
composite parts. Each dimension of the url is delineated with namespace markers,
identified by colons before and after the namespace name (e.g., :namespace:)."?

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode351
app/app.js:351: Y.each(paths, function(p, ns) {
What is ns, and why don't you use it?

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode354
app/app.js:354: // to whom it applies.
Thorough thinking.

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode365
app/app.js:365: // NS aware getpath
Might as well make it yuidoc linter friendly.  It's trivial.

"This is normally internal to the YUI app, and is called to obtain the url path
on which to dispatch.  We override it in order to have each namespaced path
handled separately (see teh dispatch method above)."

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode374
app/app.js:374: * NS aware navigate wrapper.
"This is normally internal to the YUI app, and is called when..." (sorry, I'm
not sure and am not investigating this second)

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode387
app/app.js:387: * apply url saves as they happen.
Why did the router have the 1ms delay to begin with?  Will disabling it bite us
in some other way?

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode398
app/app.js:398: * NS aware wrapper around _save to update URL.
So this normalizes URLs, right?  If so, cool and smart.  (maybe worth
clarifying)

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode423
app/app.js:423: event on every pageview) and other browsers (which do not).
ugh :-)

Is this another override, or is this unique to our machinery?  If an override,
worth noting as you have elsewhere.

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode426
app/app.js:426: @param {String} path URL path.
Just one namespaced part of it, right?

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode438
app/app.js:438: self._dispatching = self._dispatched = true;
whoa!  we are doing and we have done, simultaneously?  I'll have to ponder that
one. :-)  Possibly trivial, but that kind of seemingly impossible statement
might indicate a naming issue.  Or maybe it is just a simplification?  If so, it
doesn't smell right to me...inconsistencies like that feel like bug magnets to
me.

If I don't convince you, I won't make a fuss, but I'd prefer to have this
cleaned up.

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode448
app/app.js:448: req.next = function(err) {
why do we have to slam this on the instance every time?  Oh, because we are
mutating shared vars in the closure...worth a comment.

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode454
app/app.js:454: if (err === 'route') {
IOW, is this the path for the "I didn't find a route for this namespace" case? 
We just don't dispatch in that case, so that other url dimensions have a chance
to run?  If so, clarifying that in a comment would have helped me.

If I do understand what this code does correctly, then we don't have a parallel
for the multidimensional case, right?  IOW, if none of the dimensions have
success, we silently will do nothing with this code?  Not sure it matters, but
made me think.

[later] ah, you explain this in _routeStateTracker.  A comment up here would be
nice too (maybe just "see comments in _routeStateTracker"?)

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode455
app/app.js:455: callbacks = [];
[I am making notes for myself.  Maybe worth comments, maybe not.] ...clearing
the data structure in the closure...

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode456
app/app.js:456: req.next();
...recursively calling this same function, but now we'll fall through to
handling the next routes...

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode461
app/app.js:461: } else if ((callback = callbacks.shift())) {
Do I guess correctly that a lot of this code is copied from YUI sources,
modified to handle things like pendingRoutes?  If so, it would be nice to
indicate in comments which of this code duplicates YUI functionality, and which
is new.

[later] Oh...not even the beginning of this code block is copied, because it
handles multiple callbacks, which the original YUI code does not?  So it
duplicates and enhances throughout.  Yeah?  Do we need multiple callbacks
because each path (url dimension/namespace) can have a callback?

[yet later] oh...you needed it to be an array so that routeStateTracker could be
added, and so then you also made it possible for routes to provide multiple
callbacks themselves, because why not.

[on further reflection] This function is so long (and subtle, perhaps?) that I
wonder what we could do to make the high level view more digestible at first
reading. Perhaps if we abstracted some code into a well-named function or method
or two, just so it is easier to parse intent?  I don't see nice places to do
that yet. :-/

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode489
app/app.js:489: // Allow access tot he num of remaining routes for this request.
typo: to the

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode493
app/app.js:493: req.next();
...recursively calling, because we mutated the closure's callbacks variable
above...

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode500
app/app.js:500: self._dispatching = false;
I'd expect to see self._dispatched = true here and when we exit early, above,
myself, just for cleanliness.  <shrug>

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode501
app/app.js:501: return self._dequeue();
...not to be confused with deque :-P ...

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode1001
app/app.js:1001: // Additionally pass the route with its exteneded
typo: extended

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode1020
app/app.js:1020: if (cdId && seen[cdId] && (seen[cdId] >= gen)) {
I feel like I'm so close to understanding this. Maybe it's a delusion. :-)

You say that this function is to prevent processing the same url namespace path
multiple times.  How would that happen?  As far as I see, we iterate through the
namespaced paths in dispatch, one per.

Oh!  Is this for the following story?  I go to :foo:/bing/:bar:/baz.  That will
dispatch twice, once for each namespace.  Then I go to :foo:/bing/:bar:/shazam. 
Because of this code, only :bar: dispatches.  ...No, that's not it, because
_routeGeneration has increased so both will be called.  I really don't see why
this is needed yet. :-)

https://codereview.appspot.com/7327045/diff/14001/app/app.js#newcode1030
app/app.js:1030: seen[cdId] = gen;
This is never cleaned out.  Is that a problem?  Oh!  the set of cdIds is
constrained, because it is the registered callbacks for all of the routes.  nm.

trivial: Why is is it cdId (and not cbId or callbackId)?

https://codereview.appspot.com/7327045/diff/14001/app/assets/javascripts/rout...
File app/assets/javascripts/routing.js (right):

https://codereview.appspot.com/7327045/diff/14001/app/assets/javascripts/rout...
app/assets/javascripts/routing.js:96: console.log('URL has more than one
refernce to same namespace');
type: reference

https://codereview.appspot.com/7327045/diff/14001/app/assets/javascripts/rout...
app/assets/javascripts/routing.js:108: * @param {Object} componets is the result
of a parse(url) call.
typo: components

https://codereview.appspot.com/7327045/diff/14001/app/assets/javascripts/rout...
app/assets/javascripts/routing.js:147: }, '0.0.1', {
0.1.0 at the very least! :-)

(just kidding, do whatever you want, but nice code.)

https://codereview.appspot.com/7327045/diff/14001/undocumented
File undocumented (right):

https://codereview.appspot.com/7327045/diff/14001/undocumented#newcode1
undocumented:1: app/app.js:383 "_queue"
Please keep line count at 208 unless there's a really good reason not to.

I'm sympathetic to the yuidoc linter being more aggressive than we want or need,
but it's our tool to do good things (build docs) so I'm willing to encourage us
all to put up with the pain.  Maybe we can have time for a slack task to improve
the yuidoc linter toward the end of the cycle.

[jeff.pihach, 2013-02-18 17:12:47]

This looks great! I just have a couple small comments :-)

https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/rout...
File app/assets/javascripts/routing.js (right):

https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/rout...
app/assets/javascripts/routing.js:5: function _trim(s, char, leading, trailing)
{
This feels like a generic utility method which could be split out into a
separate utility method module and hung off Y.Lang so that it's available
application wide.

As a generic utility method it would probably be best solved using a regex. Ln
5-18 could be replaced with:

** WARNING - incoming untested code **

Y.Lang.trimString = function(str, start, end) {

  var trimRegex = new RegExp("^[" + start + "]+|[" + end + "]+$","g");
  return str.replace(trimRegex, "");

}

https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/rout...
app/assets/javascripts/routing.js:90: console.log('URL namespace without path');
If you're creating logs for debugging it's best to use Y.log() because it can
then be filtered in or out during debugging instead of always logging to the
console.

https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/rout...
app/assets/javascripts/routing.js:96: console.log('URL has more than one
refernce to same namespace');
s/refernce/reference :-) also see above comment about Y.log()

https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/rout...
app/assets/javascripts/routing.js:113: var u = '/';
u is url? Please don't manually minify - use meaningful variable names </pet
peeve> :-)

[matthew.scott, 2013-02-18 17:48:00]

Just a quick FYI.

https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/rout...
File app/assets/javascripts/routing.js (right):

https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/rout...
app/assets/javascripts/routing.js:90: console.log('URL namespace without path');
On 2013/02/18 17:12:47, jeff.pihach wrote:
> If you're creating logs for debugging it's best to use Y.log() because it can
> then be filtered in or out during debugging instead of always logging to the
> console.

For what it's worth, console is current filtered by deployment path (i.e.:
turned off for production, etc).  The reasons for using console.log() also
include the fact that one can log multiple objects, group, and nest logs.  This
is part of our coding standard decided on previously.

[benji, 2013-02-18 20:00:06]

Non-full review comments about trim function.

https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/rout...
File app/assets/javascripts/routing.js (right):

https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/rout...
app/assets/javascripts/routing.js:5: function _trim(s, char, leading, trailing)
{
On 2013/02/18 17:12:47, jeff.pihach wrote:
> This feels like a generic utility method which could be split out into a
> separate utility method module

+1  I'm surprised trimming strings isn't already available in JS or YUI.

> and hung off Y.Lang so that it's available application wide.

-1 to injecting ourselves into a third-party library

> As a generic utility method it would probably be best solved using a regex. Ln
> 5-18 could be replaced with:
> 
> ** WARNING - incoming untested code **
> 
> Y.Lang.trimString = function(str, start, end) {
> 
>   var trimRegex = new RegExp("^[" + start + "]+|[" + end + "]+$","g");
>   return str.replace(trimRegex, "");
> 
> }

Building regexes programmatically requires escaping the inputs.

For example, what if I want to trim any leading "bar" off a string and I pass in
"bar" as the start parameter and "rabid" as the str.  Instead of getting "rabid"
back (because the prefix wasn't there to get removed) I instead get back "id"
because my start string was interpreted as a set of characters not a literal
string.

[jeff.pihach, 2013-02-18 21:22:10]

> Building regexes programmatically requires escaping the inputs.
> 
> For example, what if I want to trim any leading "bar" off a string and I pass
in
> "bar" as the start parameter and "rabid" as the str.  Instead of getting
"rabid"
> back (because the prefix wasn't there to get removed) I instead get back "id"
> because my start string was interpreted as a set of characters not a literal
> string.

Agreed, sorry I just wrote that method into the comment box - it wasn't meant to
be a copy/paste solution :-)

[gary.poster, 2013-02-19 13:21:59]

Data:

* Trim is available in recent JS, but it only trims whitespace:
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/...

* JS does not have a regex escape function, but YUI does:
http://yuilibrary.com/yui/docs/api/classes/Escape.html

* To my knowledge YUI does not implement an arbitrary trim function, such as the
one Ben would need.

Opinions:

* I agree with Benji that we don't want to mutate YUI modules.

* I think Ben's current one-off functions are fine for now, as I said in my
review.  We could add trim function to a module somewhere as a slack task if
someone wanted to, or if Ben wanted to.  I do agree that using the regex might
in fact lead to somewhat more concise code, but, <shrug>.

I'd like to see this landed asap.

[bcsaller, 2013-02-20 04:18:47]

On 2013/02/15 18:19:40, gary.poster wrote:

> In terms of usability, I think we need a navigation spelling or other aid to
let
> one dimension easily construct a url that keeps all current aspects of the url
> except one.  AFAICT, that does not exist.  I think we would need an exposed
API
> call to construct a url only changing a given namespace + path.  Some other
> convenience functions to use that API might be nice too (like to navigate
within
> the app).

Thank you for this very through review. I didn't properly respond when you wrote
it as I was in the thick of dealing with the core issues you raised rather than
the finer points. I think I've addressed all the major issues. 

There is one outstanding option (that I haven't taken) at the higher level api.
Routes are dispatched currently w/o additional filtering on a fixed namespace.
By this I mean that /foo should match in one and only one namespace (even though
it will be preserved in the URL in the namespace it was found in). We may want
to change this by requiring routes to belong to a SubApp with a fixed namespace
or by having a namespace attribute in the route and changing App.match to
include this.

There are a few other minor ramifications of this branch, the most noteworth is
that URLs should end in a slash '/'. This allows for consistent addition of
other namespaces to the URL and simplifies parsing. Where possible views should
ask the application of the reverse URL mapping using app.getModelURL. Changes of
this nature were made in the codebase as well.

I expect that we will hit a remaining issue or two but that this will not happen
till we being using this branch, I think its ready for that. Because there is a
reasonable delta in the branch (the change to all the code living in
_dispatch/_navigate) I will resubmit but it should be familiar to anyone who has
reviewed this branch.

[bcsaller, 2013-02-20 04:23:10]

Thanks, I've included the minors, I didn't change to the regex because of the
escaping. If someone else needs this sort of method I'd be happy to improve
those for reuse but we made it pretty far w/o.

https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/rout...
File app/assets/javascripts/routing.js (right):

https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/rout...
app/assets/javascripts/routing.js:5: function _trim(s, char, leading, trailing)
{
On 2013/02/18 20:00:06, benji wrote:
> On 2013/02/18 17:12:47, jeff.pihach wrote:
> > This feels like a generic utility method which could be split out into a
> > separate utility method module
> 
> +1  I'm surprised trimming strings isn't already available in JS or YUI.
> 
> > and hung off Y.Lang so that it's available application wide.
> 
> -1 to injecting ourselves into a third-party library
> 
> > As a generic utility method it would probably be best solved using a regex.
Ln
> > 5-18 could be replaced with:
> > 
> > ** WARNING - incoming untested code **
> > 
> > Y.Lang.trimString = function(str, start, end) {
> > 
> >   var trimRegex = new RegExp("^[" + start + "]+|[" + end + "]+$","g");
> >   return str.replace(trimRegex, "");
> > 
> > }
> 
> Building regexes programmatically requires escaping the inputs.
> 
> For example, what if I want to trim any leading "bar" off a string and I pass
in
> "bar" as the start parameter and "rabid" as the str.  Instead of getting
"rabid"
> back (because the prefix wasn't there to get removed) I instead get back "id"
> because my start string was interpreted as a set of characters not a literal
> string.

Done.

https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/rout...
app/assets/javascripts/routing.js:96: console.log('URL has more than one
refernce to same namespace');
On 2013/02/18 17:12:47, jeff.pihach wrote:
> s/refernce/reference :-) also see above comment about Y.log()

Done.

https://codereview.appspot.com/7327045/diff/16001/app/assets/javascripts/rout...
app/assets/javascripts/routing.js:113: var u = '/';
On 2013/02/18 17:12:47, jeff.pihach wrote:
> u is url? Please don't manually minify - use meaningful variable names </pet
> peeve> :-)

Done.

[bcsaller, 2013-02-20 04:24:51]

Please take a look.

[jeff.pihach, 2013-02-20 14:39:46]

Looks good! I just wanted to add a comment for a future enhancement.

https://codereview.appspot.com/7327045/diff/26001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7327045/diff/26001/app/app.js#newcode351
app/app.js:351: _navigate: function(url, options) {
I would like to see this in a YUI extension eventually. Extensions encourage
modularity, make it a lot easier to document and, for external developers, to
see that you're overwriting core methods. This would also follow the 'YUI way'
for this type of modification. - We can do this in a separate branch later -

[matthew.scott, 2013-02-20 16:06:48]

Land as is - looks good to me and works well.  Thanks for the branch!

[bcsaller, 2013-02-20 16:36:16]

*** Submitted:

Namespace aware routing

Add namespace aware routing to app. This is forward looking. It adds support
for namespaced urls where these are defined as /:ns:/urlFragment/:ns2:/etc.

Each namespace in the url will be preserved and dispatched to accordingly.

Middleware should only fire once for a given request.

R=gary.poster, matthew.scott, jeff.pihach, benji
CC=
https://codereview.appspot.com/7327045