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

feat: throw if parent and child routes have the same name #2267

Merged
merged 2 commits into from
Oct 29, 2024

Conversation

skirtles-code
Copy link
Contributor

#2266 demonstrated an edge case that can lead to inconsistent internal state in the matcher.

The problem arises when using nested routes. If the child (or deeper descendant) has the same name as its parent, the parent is removed at the same time that the child is added.

Removing a parent route should also remove its children, but in this case that removal happens just before the child is added to the matcher. The child still gets added to the matcher, and its parent property still points to the parent route, even though that has now been removed. The parent can no longer be resolved directly, but when resolving the child, the matched array will still contain the removed parent.

This problem can occur during the initial router creation:

createRouter({
  // ...
  routes: [{
      name: 'abc',
      // ...
      children: [{
        name: 'abc',
        // ...      
      }]
  }]
})

It can also happen when adding a route dynamically:

const router = createRouter({
  // ...
  routes: [{
      name: 'abc',
      // ...
  }]
})

router.addRoute('abc', {
  name: 'abc',
  // ...      
})

This PR introduces a dev-only check for ancestors with the same name and throws an error if a duplicate is found.

Copy link

netlify bot commented Jun 11, 2024

Deploy Preview for vue-router canceled.

Name Link
🔨 Latest commit abc8313
🔍 Latest deploy log https://app.netlify.com/sites/vue-router/deploys/67210c887f06600008b1e9a8

Copy link

codecov bot commented Jun 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.01%. Comparing base (e8791aa) to head (3f6f633).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2267      +/-   ##
==========================================
+ Coverage   90.96%   91.01%   +0.04%     
==========================================
  Files          24       24              
  Lines        1151     1157       +6     
  Branches      355      358       +3     
==========================================
+ Hits         1047     1053       +6     
  Misses         64       64              
  Partials       40       40              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sherleysong
Copy link

How can i make / to Home , make /foo to Foo , while all the left "/:pathMatch(.*)" to PAGE_NOT_FOUND ?
{ path: '/', name: 'home', component: Main, redirect: '/home', children: [ { path: '/home', name: 'home', component: Home }, { path: '/:pathMatch(.*)', component: () => Error } ] }

@skirtles-code
Copy link
Contributor Author

@sherleysong I don't fully understand your question, but you need to remove the name: 'home' from the parent route.

@sherleysong
Copy link

@sherleysong I don't fully understand your question, but you need to remove the name: 'home' from the parent route.

It worked , thank you !

The running code was { path: '/', name: 'home', component: Main, redirect: '/home', children: [ { path: '/home', name: 'home', component: Home }, { path: '/:pathMatch(.*)', component: () => Error } ] }

@[email protected], when we visit www.xxx.cn , it will redirect to Home.
@[email protected], when we visit www.xxx.cn , it will redirect to Error. (which was wrong yesterday)

as you recommended , i removed the name: 'home' from the parent route , then
@[email protected], when we visit www.xxx.cn , it will redirect to Home.

Anyway , it worked for me , thank you again!

@posva
Copy link
Member

posva commented Oct 29, 2024

Thanks for this. I really didn't expect people to fall into this but I was wrong

@posva posva merged commit 8c73877 into vuejs:main Oct 29, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants