Skip to content
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

Should warn when params are not provided #1572

Closed
nirazul opened this issue Jul 7, 2017 · 13 comments
Closed

Should warn when params are not provided #1572

nirazul opened this issue Jul 7, 2017 · 13 comments

Comments

@nirazul
Copy link

nirazul commented Jul 7, 2017

Version

2.7.0

Reproduction link

http://jsfiddle.net/forzLu80/30/

Steps to reproduce

  1. Click on the button to generate a random param
  2. Click on the first link (param / url stays the same)
  3. Click on the second link (param / url changes)

What is expected?

Both router-link components should behave exactly the same as their data is equal.

What is actually happening?

router-link components in nested components behave differently when param values are updated.

@LinusBorg LinusBorg added the bug label Jul 7, 2017
@LinusBorg
Copy link
Member

LinusBorg commented Jul 7, 2017

This seems to be a bug indeed.

But it has nothing to do with being nested or not. The point is that the router-link component doesn't seem to reactively re-render (even though $route is a reactive dependency) when the params in $route change in this example, or it does, but $route is somehow wrong? I'm not sure yet.

It does only update in the Home component because this component re-renders due to it's dependency on $route, and that triggers a re-render for the router-link also, somehow:

<pre v-text="$route.params.nav"></pre>

Comment out the above and that component's router-link doesn't change as well:

http://jsfiddle.net/Linusborg/forzLu80/31/

A workaround would be to actually explicitly provide the nav param:

http://jsfiddle.net/Linusborg/forzLu80/32/

/ping @fnlctrl @posva

@posva
Copy link
Member

posva commented Jul 7, 2017

nav is declared as a required parameter, so creating a route link to that route without the nav parameter doesn't make sense. We should, however, display a warning.
An other workaround is to set the nav parameter as optional: /app/:nav? (I think)

@LinusBorg
Copy link
Member

@posva I agree on a philosophical level that it's better to always define required params explicitly, but technically I think this example should work nonetheless.

So why is the router-link not updating, even though $route has changed, which is a dependency of its render function?

@nirazul
Copy link
Author

nirazul commented Jul 7, 2017

I think a warning would help a lot, as this does work in some cases.
If it's an error that required params are missing in a router-link, then the bug is in my opinion, that the params are taken over when changing a route.

@posva
Copy link
Member

posva commented Dec 7, 2017

After diving a bit more on the repro, the problem comes from the fact that params that have the same name between current route and target route, are reused if not provided (plus what you explained about the component that re renders and therefore recompute the link). Which leads to another problem when two named routes have the same parameters like /u/:a and /d/:a, if you are on /u/1 the links, { name: 'u' } and { name: 'd' } will point to /u/1 and /d/1 respectively. The original idea behind this is to allow writing links like { params: { a: 1 }} or { query: { foo: 'foo' }} but I think that ultimately, we should just forbid named links without their necessary params

So here, the problem is that { name: 'app' } should warn that the params are not provided. I don't know what was the actual scenario where you found the problem @nirazul but were you combining it with query or params?

edit: actually, there's another problem as if the prop of the router-link was overwritten somewhere...

@nirazul
Copy link
Author

nirazul commented Dec 8, 2017

@posva
According to github I was working on a multi-layered navigation with: /app/:mainnav?/:subnav?

When hitting the bug I was trying to change the subnav while maintaining the mainnav parameter (I think, not sure anymore).

<router-link to="{ name: 'app' }">entry link</router-link>
<router-link to="{ name: 'app', params: { mainnav: 'main-foo' } }">mainnav link</router-link>
<router-link to="{ name: 'app', params: { mainnav: 'main-foo', subnav: 'sub-foo' } }">subnav link</router-link>

The main problem stays the same, though: Why the difference in the behaviour? In my understanding, current params and queryparams should be maintained when omitted and following a named route, right?

@posva
Copy link
Member

posva commented Dec 8, 2017

That's actually different because in /app/:mainnav?/:subnav? params are optional so going to { name: 'app' } from / is valid but if the path was /app/:main then it wouldn't (it would display a warning on the console)

@nirazul
Copy link
Author

nirazul commented Dec 8, 2017

@posva Ok sorry for the confusion. I guess while trying out how to implement this, I stumbled upon the error.

@posva
Copy link
Member

posva commented Dec 8, 2017

What? there's nothing to be sorry about! 😄
Thanks for helping out with clear bug reports and improving vue-router!!!

@lzxb
Copy link

lzxb commented Jan 11, 2018

this bug,me submitted pr,Why did no one handle it

@posva posva added this to TODO in 3.x Feb 27, 2018
@posva posva moved this from Icebox to Todo in 3.x Apr 8, 2018
@posva

This comment has been minimized.

@posva posva moved this from Todo to Ready to review/merge in 3.x Apr 17, 2018
@posva posva added improvement and removed bug labels Jul 9, 2018
@posva posva changed the title router-link in nested component behaves differently than one in a page-route Should warn when params are not provided Jul 9, 2018
@posva
Copy link
Member

posva commented Jul 9, 2018

So, good news, #2286 fixes this (it's similar to #1918).
Bad news: this shouldn't actually work, it should at least display a warning saying that you are using implicit params. But I'm not fully convinced.
I adapted the title accordingly

@posva posva added this to Doable or in refactor (low prio, low complex) in Longterm Mar 26, 2019
@posva
Copy link
Member

posva commented Apr 22, 2020

Merging params will be kept so that users can provide partial params to navigate between children routes

@posva posva closed this as completed Apr 22, 2020
@posva posva moved this from Doable or in refactor (low prio, low complex) to Done in Longterm May 9, 2020
@posva posva moved this from Ready to review/merge to Done in 3.x May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3.x
  
Done
Development

No branches or pull requests

4 participants