Skip to content

[next] Different behaviour for default subRoute when using path vs. named route #629

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jwahdatehagh opened this issue Aug 31, 2016 · 29 comments

Comments

@jwahdatehagh
Copy link

jwahdatehagh commented Aug 31, 2016

I have a nested routes with a default component:

routes: [
  { 
    path: '/home', 
    name: 'home',
    component: Home,
    children: [
      {
        path: '',
        name: 'home.index',
        component: HomeIndex
      }
    ]
  }
]

I'd expect for HomeIndex to be rendered into Home's <router-view> when i enter Home, but when i navigate to it using :to="{ name: 'home' }", only Home is rendered.

Using to="/home" directly works as expected.

Vue.js & vue-router.js version

2.0.0-rc.4, 2.0.0-rc.4

Reproduction Link

https://jsfiddle.net/jwahdatehagh/rdtgfrfb/

@LinusBorg
Copy link
Member

This is expected behaviour and intentional. If you want to render the default path via name, you have to use it's name, not the parent name.

@jwahdatehagh
Copy link
Author

jwahdatehagh commented Aug 31, 2016

Ok, thank you!

Can you explain the reasoning behind that? I think the uri should represent the state of the app. If :to="{ name: 'home' }" and to="/home" link to the same uri, why should the rendered state be different?

@LinusBorg
Copy link
Member

I think the uri should represent the state of the app.

You're right about that, we should discuss this.

It's a bit related to your other ticket, #628 also I think - there it's about the question that /home and /home/ should both be matched as active even with exact.

@fnlctrl ping

@LinusBorg
Copy link
Member

LinusBorg commented Aug 31, 2016

When using a named route-link, the route record is fetched by that name from a nameMap object.

https://github.com/vuejs/vue-router/blob/next/src/create-matcher.js#L33

When we use a path, the matched route is retrieved from a pathMap object. Here we get the defautl child route.

https://github.com/vuejs/vue-router/blob/next/src/create-matcher.js#L33

That's why it works differently I think.

Maybe the solution is as easy as this:

After getting the record by name, get the record again from the pathMap using ? record.path + '/', which should return the record for the default child route if there is one, and if there is none, it should return the same route object again.

@LinusBorg LinusBorg changed the title [next] default component in nested routes [next] Diofferent behaviour for default componentwhen using path vs. named route Aug 31, 2016
@LinusBorg LinusBorg changed the title [next] Diofferent behaviour for default componentwhen using path vs. named route [next] Different behaviour for default subRoute when using path vs. named route Aug 31, 2016
@frankdugan3
Copy link

I just ran into this today. I agree that it is pretty unintuitive to have the named route-link resolve differently than the path when a default child route is defined. @LinusBorg your solution seems like it would produce intuitive results.

@LinusBorg
Copy link
Member

LinusBorg commented Sep 1, 2016

after some discussion we will solve this with a warning: When the dev uses a named route that has a default child route, there will be a warning in the console.

Reasoning:

  • If we don't change the current behaviour, there can be different behaviour between navigating to a URL from a named link, and visiting the same URL by typing it in the browser, as demonstrated by OP.
  • But if we matched the parent route to the child default route, this may be unexpected for the developer because he expected to get the parent route object after transition, not the child route. So this would solve the issue for end users, but may introduce problems for the dev.
  • Therefore, we want to discourage using the parent named route in these cases altogether. We will show a warning encouraging the dev to use the default child's named route instead.

We will reference the commit here when it's done.

@LinusBorg
Copy link
Member

PR provided with #641 - currently in review.

@inca
Copy link

inca commented Sep 7, 2016

I'm not sure that refraining from using links to parent is the right thing to do.

Say I have item.view and its default child item.summary (among other children). I normally specify :to="{ name: 'item.view' }" in the link because I want it to stay active for all children subroutes. Basically, if child link is used, then we loose active link highlighting there (and it kinda sucks since there's params involved, too).

@jwahdatehagh
Copy link
Author

Oh lol @inca so true...

yyx990803 pushed a commit that referenced this issue Sep 8, 2016
)

* fix  #629 - add warning when named route has a default child route.

* Improve warning message.

* this fails me

* debug log

* corrected Object.keys() mistake, shrunk test case to one case, still fails

* only warn when not in production mode.

* remove the helper, solved it with a normal jasmine spy

* ad child component again

* remove msg variable, check with `toMatch()` instead.

* remove unnecessary test case.

* remove unnecessary const, create it in beforeAll instead. change testacy to revert to this.maps instead of maps

* add test case for console.warn spy

* remove debug log

* add replace plugin for rollup to define "globals" like with web packs DefinePlugin.

* add replace plugin to rollup config and set production env variable in pm build script.

* Importe test case to test warning in production and development mode.

* fix unrelated lining bug: debugger statement in a file.

* cleanup unnecessary statement from test case
@LinusBorg
Copy link
Member

@inca that's indeed a common scenario that would clash with this fix.

I will leave this issue open and think about how to deal with this.

@LinusBorg
Copy link
Member

LinusBorg commented Sep 8, 2016

@inca on second thought, this is not an issue.

You would link to the default child named route like this:

<router-link :to="{name: 'item.summary'}"></router-link>
// path: `/ítem/12/` -> link is active

now if you link to another child of that parent:

<router-link :to="{name: 'item.other-child-route'}"></router-link>
// path: `/item/12/other-child-route` -> link is active

... the first link with the default child route would also still be active, because the non-exact match is still true.

Little demo: https://jsfiddle.net/Linusborg/r0jsw888/2/

@jwahdatehagh
Copy link
Author

Ah, nice @LinusBorg.
I think in your demo { path: '/', name: 'def-child', component: Default }, the path should be empty { path: '', name: 'def-child', component: Default }, otherwise it will be at the actual root '/' right? But this solution makes sense ty...

@inca
Copy link

inca commented Sep 8, 2016

Haha, yep, that works because of paths matching, but tbh that's a bit unintuitive, too :)

I mean, just think about it: you have a link to child state which is active while the state is not technically active (b/c its sibling state is active). Makes my head spin a bit 😄

@LinusBorg
Copy link
Member

LinusBorg commented Sep 8, 2016

I think in your demo { path: '/', name: 'def-child', component: Default }, the path should be empty { path: '', name: 'def-child', component: Default }, otherwise it will be at the actual root '/' right?

No, in child routes, '' and '/' are both valid default child paths. '/' doesn't link to the root in this case.

@LinusBorg
Copy link
Member

LinusBorg commented Sep 8, 2016

@inca I admit that this is not 100% intuitive when thinking from the perspective of the named route. But the real starting point should always ne the path. The URL is the source of truth.

From that perspective, it makes sense that the parent and the default child have the same "active" behaviour - they have the same path. In that way it behaves exactly like the browser does.

As you can read further up, we thought about changing the behaviour to acutally match the parent named route to the default child, but that would be both a breaking change and also confusing in its own way.

@inca
Copy link

inca commented Sep 8, 2016

The URL is the source of truth.

Yep, unfortunately. (I can elaborate, but it's not exactly related to this issue)

Named routes are kinda inevitable if params are involved, which makes this piece of navigation a bit hard to reason about. Conceptually, one could also argue that upper-level navigation component should only point to "item" without referring to an explicit substate. In other words, only router decides which subroute is default, not router and links.

Since I'm a big fan of old Angular's ui-router I would propose adding "abstract route" feature. The idea is pretty intuitive: simply add abstract: true flag on parent — and it would mean that the parent should not be rendered on its own. If transition.to refers to abstract route it will quietly redirect to its default child (or default child's child, recursively) — or throw an error if non-abstract route not found.

@inca
Copy link

inca commented Sep 8, 2016

On a second thought, I think we can make one of the following assumptions without introducing abstract: true:

  1. if one of the child route has path '', then the parent route is considered abstract and should not be rendered directly; instead, the default child should be considered the target state every time the transition is made to parent.
  2. more general, breaking and unsafe: if route has children, then it is abstract an must contain a default route.

Reasoning: the problem obviously stems from the combination of two facts: 1) URL (pathname) is the source of truth for routing composition and 2) both parent and default child share the same URL thus introducing a degree of ambiguity into the source of truth.

So in case the first solution is preferred the fix is somewhat simple: first resolve the named route to path using nameMap => then resolve the target route using pathMap (which would be the default child) => no more ambiguity and frustration, everyone is happy 😄

@LinusBorg
Copy link
Member

LinusBorg commented Sep 8, 2016

This is exactly the solution I had in mind at first (see last paragraph here), but it is technically a breaking change, and also confusing in its own way:

When the dev explicitly wants to go to the named route parent, he will end up with $route for parent.defaultchild - which might have other $route.meta properties, other contents in $route.matched, route guards etc - namely those of the default child route config, rather than the parent's.

This sort of implicit behavior is generally not a good idea and something we try to get rid of in Vue wherever we find it.

I talked this through with Evan and the others and we came to the conclusion that we rather warn the dev of the behavior that navigating to the named parent has, and encourage to explicitly use the default child.

Then the dev wont be surprised by finding the "wrong" data in his $route.

@inca
Copy link

inca commented Sep 8, 2016

I am very sorry for being such a stubborn. But there's clearly an inconsistency there, so I don't believe that warning is an adequate solution.

Let me try to explain:

When the dev explicitly wants to go to the named route parent

This contradicts to the "URL is a source of truth" principle mentioned above. Both parent and its default child share the same URL, so going to parent (only by name!) will leave you with broken transition (unrendered ) and a warning; but refresh the page — and you land to the parent.defaultroute (b/c this time router actually uses path instead of name).

So we decide to condemn a dev for being not explicit enough by issuing him a warning. But actually it's not dev's fault, because router allows multiple routes to map to the same URL and provides no rule to tell which one should be rendered.

As for the implicit vs. explicit, I totally agree that explicit way is a good one, but 1) it shouldn't violate the separation of concerns (see my navigation-to-item-not-to-item-summary argument above) and 2) this largely depends on interpretation: for me path: '' on child state is explicit enough to make it a default substate for every possible method of transition (via go or router-link or location.href, by using path or name), while on the other hand router makes an implicit assumption that parent can be rendered standalone, which just isn't true for the majority of cases.

Bottom line is, if the rule is explicitly stated in docs and covers real usage scenarios everything should be fine.

@fnlctrl
Copy link
Member

fnlctrl commented Sep 9, 2016

@inca Some side notes:


Since I'm a big fan of old Angular's ui-router I would propose adding "abstract route" feature. The idea is pretty intuitive: simply add abstract: true flag on parent — and it would mean that the parent should not be rendered on its own. If transition.to refers to abstract route it will quietly redirect to its default child (or default child's child, recursively) — or throw an error if non-abstract route not found.

vue-router's nested route is different by design to ui-router's counterpart because:
In ui-router docs, it states:

An abstract state can have child states but cannot get activated itself.

But in vue-router, child routes are considered optional, and the parent route can get activated itself.


This contradicts to the "URL is a source of truth" principle mentioned above. Both parent and its default child share the same URL

So true... that's the exact reason I removed all the route names in my prod app, and switched to manually spliced full paths. I wonder if we can deprecate names in vue-router 3.0 since it's only bringing troubles all the way (active class, child routes, etc.), making roughly 31% of the issues here ( (14+137)/(37+448) as of today ).


To sum up, I agree with @LinusBorg that a warning is enough, because

for me path: '' on child state is explicit enough to make it a default substate

As stated in the first note, child routes are considered optional, and this implicit behavior may also bother devs who want to show empty child route.

@LinusBorg
Copy link
Member

I think there's room for different perspectives. I can see some valid arguments in what @inca explained.

I think both ways are a possibility - and neither will be a no-go.

So I think for the moment the solution we provided works, but we should further discuss weither it might be worth it to match the default child route, after all.

Thanks for the input @inca, and no worries about being stubborn, that's nessessary sometimes ;)

@inca
Copy link

inca commented Sep 9, 2016

@LinusBorg Thank you!

@fnlctrl not really getting the "child routes are considered optional" thingy... Personally I only use routes nesting only for shared stuff used by children (layouts, data fetching, beforeEnter hooks, common URL prefix, nav highlighting). The parent itself doesn't have meaningful content, only <router-view> and decorations around it.

I wonder, what's the usecase for rendering an empty parent that I've missed?

@fnlctrl
Copy link
Member

fnlctrl commented Sep 9, 2016

Parents actually can have meaningful content, other than just a router-view in its template. It's also a very common use case.

@LinusBorg
Copy link
Member

LinusBorg commented Sep 9, 2016

I wonder, what's the usecase for rendering an empty parent that I've missed?

A simple one would be:

  • /post/1: only show the post
  • /post/1/comments show comments under the post.
  • /post/1/comments/new : show form in the comments section under the post.

Of course one could solve these trivial example without routes and might even prefer to, but it seems obvious to me that there can be similar scenarios where one wants to have a route that has children, but doesn't need a default child.

@inca
Copy link

inca commented Sep 9, 2016

@LinusBorg Thanks for examples, makes sense. Seems like it's safe to assume parent + default child = abstract parent, but I'm happy to leave this up to you guys :)

@inca
Copy link

inca commented Sep 9, 2016

btw, I just discovered this: https://github.com/vuejs/vue-router/blob/next/examples/redirect/app.js#L31, so parent can explicitly redirect to its default child — works like a charm!

@fnlctrl
Copy link
Member

fnlctrl commented Sep 9, 2016

@inca That's actually a very smart solution to this issue! Explicit and intuitive. (It'a pity we didn't come up with it before... it just makes so much sense)

@posva I think we should add it to the guides!
@LinusBorg Maybe we can suggest that in the warnings?

@frankdugan3
Copy link

Having read all of this, I agree with the direction of the dev team, and the redirect suggested by @inca resolves my use case.

But I do think it would still be nice to have some kind of option, like :to="{name: 'foo', resolve: true}", which would explicitly allow the default child to be resolved (if it exists) when navigating to a named parent route. There may be use cases where the redirect method would be undesirable.

@LinusBorg
Copy link
Member

I will close this for now, as i think we have an understanding that things will stay the way they are for now. We can revisit this later if the need arises.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants