Views#find returns unexpected view in certain cases

[ericyhwang]

The Views#find(name, namespace) function returns an unexpected view in certain cases, due to odd lookup precedence and/or looking up some odd combinations of view segments.

derby-templates/lib/templates.js

Line 336 in 04a266a

Views.prototype.find = function(name, namespace) {

Details and example

Say that you have these views registered:

icons:app-logo
icons:star
mail:Body
mail:icons:open-envelope  // Add new icon only for 'mail' subtree
mail:icons:star  // 'mail' subtree wants to override the default star icon
mail:inbox:list

Now you try to load the page on the "mail:inbox:list" view. Derby tries to look up the "mail:inbox:list:Body" view from the root namespace. I would expect this lookup order, looking up "Body" in each parent successively:

- mail:inbox:list:Body
- mail:inbox:Body
- mail:Body
- Body

However, the actual lookup order looks like this:

derby-templates/lib/templates.js

Line 336 in 04a266a

Views.prototype.find = function(name, namespace) {

"exact match"
- mail:inbox:list:Body
segmentsDepth 3
- inbox:list:Body
segmentsDepth 2
- mail:list:Body
- list:Body
segmentsDepth 1
- mail:inbox:Body
- mail:Body
- Body

The parent-lookups I expected are actually the last set of lookups. The buggy behavior is that, if you have a view whose absolute path is "inbox:list:Body", that will take precedence over the immediate parent's "mail:inbox:Body", which is really unexpected. In fact, given an input of "mail:inbox:list:Body", I wouldn't expect it to try "inbox:list:Body" at all.

Historical context

After doing some code spelunking, it appears that, in early 2014, the lookup actually did work the way I expect. The lookup code changed in #2 to what it is today, with a couple variable name changes in the meantime.

That PR tried to fix derbyjs/derby#381 - translating the report to the example above, it was trying to look up "icons:app-logo" from inside "mail:inbox:list" and was failing, as the code joins the current namespace and the view argument and does the lookup based on that. So it was trying this lookup order, which failed:

- mail:inbox:list:icons:app-logo
- mail:inbox:list:app-logo
- mail:inbox:app-logo
- mail:app-logo
- app-logo

The current code uses this lookup order for find('icons:app-logo', 'mail:inbox:list'):

"exact match"
- mail:inbox:list:icons:app-logo
segmentsDepth 4
- inbox:list:icons:app-logo
segmentsDepth 3
- mail:list:icons:app-logo
- list:icons:app-logo
segmentsDepth 2  // After the exact check, I'd expect the lookup to just be this section
- mail:inbox:icons:app-logo
- mail:icons:app-logo
- icons:app-logo  // Code finds a match and finishes here
segmentsDepth 1
- mail:inbox:list:app-logo
- mail:inbox:app-logo
- mail:app-logo
- app-logo

I'd expect the lookup to just be the "exact match" check and then the checks under "segmentDepth 2".

Proposed solution

Have find(name, namespace) take advantage of the fact that it gets both pieces of information instead of just concatenating them.

If it gets both parameters, then I propose that it find('icons:app-logo', 'mail:inbox:list') treat "icons:app-logo" as an indivisible unit and do lookups in the namespace's parents successively:

- mail:inbox:list:icons:app-logo
- mail:inbox:icons:app-logo
- mail:icons:app-logo
- icons:app-logo

If it gets only a single parameter, then it should be treated as a lookup from the root. However, it looks like Derby renders "BodyElement" by looking up <view is="{{$render.prefix}}Body"></view>:
https://github.com/derbyjs/derby/blob/6bdd0d1834344c9560b29abc1a53c7f5971b6dd6/lib/App.server.js#L62

So for the single-parameter case, I propose that it go with the early-2014 behavior of doing lookups by "moving" the last segment of the view name up each parent successively. find('mail:inbox:list:Body') would then have this lookup order:

- mail:inbox:list:Body
- mail:inbox:Body
- mail:Body
- Body

This would technically be a breaking API change to Derby. I'll do some local manual testing of this proposed change with the Lever app, to see if it introduces any regressions there.

If that works out, I'll send a PR with the change and a bunch of tests for the various view lookup cases I describe. If not, then I'll update this issue with examples of what breaks and we can go from there.