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

Params from the current location are not copied to the target location #189

Closed
janispritzkau opened this issue Apr 22, 2020 · 6 comments
Closed
Labels
enhancement New feature or request

Comments

@janispritzkau
Copy link

janispritzkau commented Apr 22, 2020

I've noticed that there is a todo to merge the params from the current location into the target location.

https://github.com/vuejs/vue-router-next/blob/9697134c05f0f4c6fde48a773880946074e95666/src/matcher/index.ts#L201

However, this is not the only place where you'd need to merge route params. When there is a redirect in a route, it only uses the params from the redirectedFrom location or the targetLocation. But instead, the params from targetLocation should be merged into the redirectedFrom location if it exists.

Here's what I did to get most of the old Vue Router 3 behavior back: https://github.com/janispritzkau/vue-router-next/commit/bc5d568d5412380f226e518ae6d50e14e079d2b1

I'm not sure what is meant with Should this be done by name. in the comment above.

@janispritzkau janispritzkau changed the title Params from current location not copied to target location Params from the current location are not copied to the target location Apr 22, 2020
@vuejs vuejs deleted a comment from vue-bot Apr 22, 2020
@janispritzkau
Copy link
Author

janispritzkau commented Apr 22, 2020

I forgot a thing. Only the params that exist in the target location's route definition should be copied from the previous one.

@posva
Copy link
Member

posva commented Apr 22, 2020

Feel free to do a PR if you want! I will help you get it through. As you noticed, we only need to copy params that exist at the target location. It's also not a merge: if the target location contains params, we use those instead, that way we can easily remove optional params and it's still easy to keep existing params with { ...$route.params }. I don't understand why you need to merge redirectedFrom params though, I don't think we need to do that.

About the comment by name, I probably meant by key

@posva posva added the enhancement New feature or request label Apr 22, 2020
@janispritzkau
Copy link
Author

Hmm, the redirectedFrom merge doesn't make much sense there. There is still a problem with redirects that the currentLocation is never set to the route location that has the redirect, which means that those params are not copied over.

@posva
Copy link
Member

posva commented Apr 22, 2020

I'm not sure I get what you mean, the currentLocation shouldn't be set to the route location that has the redirect because it's never a location displayed by the Router. Are you referring to automatically reusing params that were initially passed through a push to a partial redirect like redirect: { name: 'other' }? I have to check a few things but it's a behavior that caused a few issues dues to its implicit nature so it would be better to not add it at least for now.

@janispritzkau
Copy link
Author

janispritzkau commented Apr 22, 2020

To give some context, this is roughly what the router looks like in my app:

const routes = [
  {
    path: "/configuration/:configuration",
    name: "Configuration",
    redirect: { name: "Step1" }
  },
  {
    path: "/configuration/:configuration/step-1",
    name: "Step1",
    component: FirstStep
  },
  // ... other configuration steps
]

Now when I go to /configuration/2 I want it to redirect me to /configuration/2/step-1.

For all the other routes I'd just do router.push({ name: "SomeStep" }) and have the params copied over from the previous location.

@posva
Copy link
Member

posva commented Apr 22, 2020

I was giving this a spin because I thought that redirects being relative to the targetLocation instead of the currentLocation was a mistake but I changed my mind: I wanted to make next and redirect consistent with each other but that is not necessary as they play a different role when it comes to redirections

You can take a look at https://jsfiddle.net/posva/j9ts2dqp/ (Vue Router 3). Going from /a/1 to a /b/2 gives an id=2 but going from /a/1 to /b2/4 gives id=1. Also going from / to /b2 gives an id=2 but going from / to /b2/4 gives an error because the param id is missing. You can compare it with https://jsfiddle.net/posva/dqhv4y9x/ (Vue Router 4) and it's indeed consistent but I think it makes more sense for redirect to implicitly merge with the targetLocation instead of currentLocation

I will fix this

@posva posva closed this as completed in d8a6b25 Apr 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants