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

Dynamic segments for nested items not updating correctly #2800

Comments

@agnjunio
Copy link

agnjunio commented Jun 4, 2019

Version

3.0.3

Reproduction link

https://jsfiddle.net/zrh122/3v4cj0qk/

Steps to reproduce

  • Click on Item 1
  • Click on Item 2
  • Click on type link

What is expected?

We end up at /item/2/logs

What is actually happening?

We end up at /item/1/logs


This behavior happens when we don't explicitly set a parent param for a child route. Setting $route.params.id before rendering the fixes the behavior, but this shouldn't be necessary as we already navigated to /item/2, but vue-router didn't update the dynamic segment.

@zrh122
Copy link
Contributor

zrh122 commented Jun 5, 2019

I add a simpler reproduction link for it. https://jsfiddle.net/zrh122/3v4cj0qk/

@posva
Copy link
Member

posva commented Jun 5, 2019

Thanks for the smaller reproduction, I updated the original issue to be clearer.

The problem here is that the router-link isn't rendering when current route changes, so I don't think #2802 is the right fix.

At the same time, we are allowing implicit partial params to the location passed to router-link but this is bug-prone as the router link could be used in a different views that may not even be the parent of the location but still share the same param id and I don't think we should reuse the param in that case.

Take the following case:

const routes = [
	{ path: '/a/:id', name: 'a' },
	{ path: '/a/:id/:dynamic', name: 'a.dyn' },
	{ path: '/b/:id', name: 'b', children: [
	  { path: 'one', name: 'b.one' },
	  { path: 'two', name: 'b.two' },
      { path: ':dynamic', name: 'b.dyn' }
	  ]
	}
]
  • we are on /a/2, { name: 'b.one' } will point to /b/2/one but we are sharing a param across different routes
  • we are on /b/2/two, { name: 'b.one' } will point to /b/2/one
  • we are on /b/2/two, { name: 'b.dyn', params: { dynamic: 'foo' } } will point to /b/2/foo, params are partially merged
  • we are on /a/2, { name: 'b.dyn', params: { dynamic: 'foo' } } will point to /b/2/foo, params are partially merged across routes that are unrelated
  • we are on /a/2/other, { name: 'b.dyn' } will point to /b/2/other, params are taken from a different route

This makes me wonder if it's really a good idea to implicitly merge params solely based on their name. We don't do this with query and it's very easy to pass explicitly pass params with { params: {...this.$route.params, ...otherParams} } and make it easier to guess. In any case, we won't remove current behavior but I need to trace back to where we started doing that and make sure is the right choice for future versions

@zrh122
Copy link
Contributor

zrh122 commented Jun 5, 2019

At the same time, we are allowing implicit partial params to the location passed to router-link but this is bug-prone as the router link could be used in a different views that may not even be the parent of the location but still share the same param id and I don't think we should reuse the param in that case.

Yeah, i agree with it and at now merge params is very simple. But look at this https://jsfiddle.net/zrh122/Lu89js0n/ (add some print)

 to: {
     name: 'items.logs.type',
     params: { type: 'info' }
 },

be changed to:

to: {
     name: 'items.logs.type',
     params: { type: 'info', id: 1 }
 },

i think this should not happen.
The code in reproduction will work as expect if there has no modify to to object. @posva

posva pushed a commit that referenced this issue Sep 23, 2019
Closes #2800, #2938 

* fix(normalizeLocation): add a copy for params with named locations

* test: add test case for #2938
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment