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/routing.js
File app/assets/javascripts/routing.js (right):

https://codereview.appspot.com/7327045/diff/14001/app/assets/javascripts/routing.js#newcode96
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/routing.js#newcode108
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/routing.js#newcode147
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.

https://codereview.appspot.com/7327045/