Skip to content

Commit

Permalink
feat(matcher): avoid empty records to be reached
Browse files Browse the repository at this point in the history 
Resolves posva/unplugin-vue-router#81
  • Loading branch information
@posva
posva committed Oct 24, 2022
1 parent d218cca commit 756f755
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 1 deletion.
38 changes: 38 additions & 0 deletions packages/router/__tests__/matcher/resolve.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1004,6 +1004,44 @@ describe('RouterMatcher.resolve', () => {
)
).toMatchSnapshot()
})

it('avoids records with children without a component nor name', () => {
assertErrorMatch(
{
path: '/articles',
children: [{ path: ':id', components }],
},
{ path: '/articles' }
)
})

it('avoids nested records with children without a component nor name', () => {
assertErrorMatch(
{
path: '/app',
components,
children: [
{
path: '/articles',
children: [{ path: ':id', components }],
},
],
},
{ path: '/articles' }
)
})

it('can reach a named route with children and no component if named', () => {
assertRecordMatch(
{
path: '/articles',
name: 'ArticlesParent',
children: [{ path: ':id', components }],
},
{ name: 'ArticlesParent' },
{ name: 'ArticlesParent', path: '/articles' }
)
})
})

describe('children', () => {
Expand Down
11 changes: 10 additions & 1 deletion packages/router/src/matcher/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,16 @@ export function createRouterMatcher(
// parent.children.push(originalRecord)
// }

insertMatcher(matcher)
// Avoid adding a record that doesn't display anything. This allows passing through records without a component to
// not be reached and pass through the catch all route
if (
(matcher.record.components &&
Object.keys(matcher.record.components).length) ||
matcher.record.name ||
matcher.record.redirect
) {
insertMatcher(matcher)
}
}

return originalMatcher
Expand Down

6 comments on commit 756f755

@doutatsu
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just dropping by, as this commit has broke a lot of routes on my site - specifically it broke ability to provide external URL redirects in routes:

  {
    path: '/google',
    beforeEnter() {
      window.location.replace('https://google.com');
      return false;
    },
  },

I found this to be a much simpler way to handle external linking, rather than creating a custom router-link. Any chance to fix this regression? I would imagine I am not the only one doing this. Otherwise the workaround is to provide a random component, like so:

  {
    path: '/google',
    component: PrivacyPolicy,
    beforeEnter() {
      window.location.replace('https://google.com');
      return false;
    },
  },

@posva
Copy link
Member Author

@posva posva commented on 756f755 Oct 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, thanks for sharing! That kind of route record is nonvalid: it will report an error in TS, and that's why it didn't prevent this change from happening.

Other solutions such as global navigation guards and a simple switch case inside to handle a redirection table or even the one you mentioned about a custom router link are the expected ways of doing things rather than a beforeEnter() guard. I recommend you migrate to one of those instead.

router.beforeEach(to => {
  if (to.path === '/google') {
    window.location.replace(...)
  }
})

@franciscohermida
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is relevant, but this update broke this use case where I'm using "empty" routes to structure a hierarchy of metadata. Example:

  {
    path: "/",
    component: () => import("@/layouts/MainLayout.vue"),
    children: [
      {
        path: "",
        meta: { category: "Projects" },
        children: [
          {
            name: "Projects",
            path: "/projects",
            meta: { name: "Overview" },
            component: () => import("@/views/Projects.vue"),
          },
        ],
      },
     }

In the component I use it to dynamically generate a side menu using router.getRoutes() and parsing the metadata. With this update the method doesn't return the "empty" routes, breaking this use case.

@posva
Copy link
Member Author

@posva posva commented on 756f755 Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's relevant. Either add a name or a component to make it considered as a regular route and appear in router.getRoutes()

@Kirsten-G
Copy link

@Kirsten-G Kirsten-G commented on 756f755 Nov 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure if my use case is related to this. I used the following structure to get the top level navigation highlighted when a sub-menu item is active using the linkActiveClass: 'active',

routes: [

{
      path: '/products',
      children: [
        {path: '/products/list', name: 'ProductsList', component: ProductsList},
        {path: '/products/families', name: 'ProductFamilies', component: ProductFamilies},
         ...
       ],
   }, 
]

Since the update the linkActiveClass doesn't bubble up anymore.

@posva
Copy link
Member Author

@posva posva commented on 756f755 Nov 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not that it doesn't bubble up anymore, the route record is now (correctly) hidden from route list as it shouldn't be reachable. This avoids reaching that page via a link (e.g. /products) by accident and finding yourself with a half empty page (missing the nested content). If the page can be reached, adding a name will make it reachable and should fix your issue. In most cases, the desired behavior is adding a child route with an empty path { path: '', component: ... }, you can even name that route and make your router link point to it

Please sign in to comment.