vuetify icon indicating copy to clipboard operation
vuetify copied to clipboard

[Bug Report][3.1.14] Components built with @/composables/router can crash when routes are missing required params

Open MatthewAry opened this issue 1 year ago • 24 comments

Environment

Vuetify Version: 3.1.14 Vue Version: 3.2.47 Browsers: Chrome 112.0.0.0 OS: Mac OS 10.15.7

Steps to reproduce

This is a really dirty reproduction. Clone, and npm i and run. There are two buttons, click on "VBtn Flaw" and then click on the home button in the top right corner.

Expected Behavior

Click on "Router Link" and then click on the home button on the top right corner. And see the expected behavior.

I expect that when you use a named route which is a child to a parent(s) with param(s) that it will inherit the params implicitly from it's parent, you shouldn't need to declare the params that are missing.

Actual Behavior

VBtn in the example crashes when you navigate back to home.

Reproduction Link

https://github.com/MatthewAry/vuetify-route-link-bug

Other comments

This reproduction shows the behavior with RouterLink and with VBtn the to params on the elements shown on those pages are basically the same. I think behaviorally when it comes to routing, there should be no difference between RouterLink and VBtn or any other component that uses @/composables/router

Update: https://github.com/vuejs/router/issues/845 has to do with this exact problem however the issue was closed because the team believed it was resolved. If you build your application in production mode you won't see this issue. See: https://github.com/vuejs/router/issues/845#issuecomment-806762085 Which suggests that ~~this issue may be upstream~~ or that the useLink method in @/composables/router needs to change ~~in some manner to be more similar to what the RouterLink component is doing~~ to avoid this issue. 🤷

Update: See https://github.com/vuetifyjs/vuetify/issues/17176#issuecomment-1517843304

MatthewAry avatar Apr 18 '23 15:04 MatthewAry

May be related to https://github.com/vuejs/router/issues/845

MatthewAry avatar Apr 18 '23 16:04 MatthewAry

This issue doesn't seem to be caused by the code related to https://github.com/vuejs/router/commit/41088143c62244fe7b198e0907f4f6f98852df62 as commenting out the current version of the dev tools code and running the application does not resolve this issue. Instead it seems that the buttons are getting re-rendered when the route changes causing it to re-resolve the route link which becomes impossible because at the time of re-rendering the routable buttons, the router has already changed its state to not have the relevant params.

MatthewAry avatar Apr 18 '23 19:04 MatthewAry

So after quite a bit of investigation this is what I've learned. Components in Vuetify that make use of the useLink function from Vue Router's RouterLink component, utilize a few ComputedRef properties that useLink returns which are bound to the router's current state. The reactive properties used are:

  • route the current RouteLocation & { href: string }
  • isActive tells us if the specified route for the link is included in the list of matched routes for the current, "visiting" route
  • isExactActive tells us if the current route being visited is the exact same route as the one specified for the link
  • href the path to the route specified for the router link. This is extracted from the same route object detailed above.

Vuetify's useLink composable function utilizes the reactive properties listed above (with the exception of href, it gets that from route instead of using the one returned from Vue Router's own useLink function) to turn it's own components into proper links using <a> tags. This functionally similar in many ways to what Vue Router does when you use its RouterLink component. However, for some reason the behavior is different.

What I am seeing is that every time the current route changes, Vuetify's router enabled components get an update, causing it to re-resolve the route for every link. And when the new route is missing params required for the links' named route, the resolution of the href property fails causing the crash. You would expect components to unmount gracefully when you leave a route that loads and renders router link components using named routes which require params, what happens is that this route re-resolve operation takes place BEFORE the components get unmounted. This is basically a race condition.

I have come up with a workaround, but it should be considered a hack and not a solution. This hack works for onRouteEnter but not for when your params change onRouteUpdate, so you'll need to figure out how to handle onRouteUpdate because that will require more considerations depending on your use case.

In the parent component, you need to obtain the current router params and store them in a const.

const routeParams = { ...useRoute().params };

routeParams will end up being assigned the route parameters that were present the moment the parent component was rendered and will be a non-reactive object.

Then in your template you will want to do something like this.

<VBtn :to="{ name: 'RouteName', params: routeParams }">Your Router Link Button</VBtn>

This way you'll be passing in the necessary params to make your link properly resolve even if your route changes to something that doesn't contain the necessary params.

MatthewAry avatar Apr 21 '23 13:04 MatthewAry

Awesome work @MatthewAry ! I can confirm your workaround. I also pass params to v-tab (in my case).

villageseo avatar Apr 21 '23 14:04 villageseo

I asked @posva about this and he said:

Once you have a boiled down reproduction only with vue router, open a new issue. What you are describing is unlikely caused by the router as that hasn't changed in a long while. we have seen however similar problems when using watchers on the routes and they have been reported in vue core in the past. Maybe that helps you reproduce it https://discord.com/channels/325477692906536972/325479452773580800/1098990698549743710

MatthewAry avatar Apr 21 '23 15:04 MatthewAry

Confirming this issue on my side as well 🙏🏻

Oleksii14 avatar Apr 21 '23 19:04 Oleksii14

I have created hack that should satisfy most use-cases but I have not done comprehensive testing of it. My hope is that it will make sure the required params are fed to the routable components. Also, please note that this solution has room for optimization. If you moved the routes map out of the SafelyResolveRouterParams class into a global, and you properly update it when you perform removeRoute and addRoute operations, it should reduce memory and compute for the components that have their own class instance.

https://gist.github.com/MatthewAry/7129cfe405f93bb5827e7cbd7928dd40

Update: safeNamedLocation gets called a lot. For example, Just hovering over an element that is using this method can trigger three function calls.

MatthewAry avatar Apr 21 '23 20:04 MatthewAry

Maybe I don't understand the actual issue or I don't know how to use vue-router "correctly", but isn't the problem, that the params: { pid: '...' } for v-btn :to within the views/example/Example.vue file aren't defined? Like it is in components/HelloWorld.vue.

1Luc1 avatar Apr 28 '23 08:04 1Luc1

It should be implicitly inherited.

On Fri, Apr 28, 2023, 1:29 AM 1Luc1 @.***> wrote:

Maybe I don't understand the actual issue or I don't know how to use vue-router "correctly", but isn't the problem, that the params: { pid: '...' } for v-btn :to within the views/example/Example.vue file aren't defined? Like it is in components/HelloWorld.vue.

— Reply to this email directly, view it on GitHub https://github.com/vuetifyjs/vuetify/issues/17176#issuecomment-1527184837, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABGMIJJ3AOJ7VJ6RP57WA3XDN5VRANCNFSM6AAAAAAXCZ66A4 . You are receiving this because you were mentioned.Message ID: @.***>

MatthewAry avatar Apr 29 '23 21:04 MatthewAry

Playing around a bit, i got the same problem with <v-list> and props property on the items, as you already mentioned. Nevertheless, the error seems a little bit different. I post my findings here; maybe it can help to solve this problem.

I set up an quick example about the problem here: https://github.com/1Luc1/list-props-to-bug

Basically there are two Layouts. One Layout holds a child component. Switching from one layout to the other causes the following browser warning during development (npm run dev):

[Vue warn]: Unhandled error during execution of watcher callback 
  at <VListItem title="Right View #1" value=undefined prependIcon="$arrowRight"  ... > 
  at <VListChildren items= Array [ {…}, {…} ] > 
  at <VList items= Array [ {…}, {…} ] density="compact" active-color="primary" > 
  at <VCol cols="auto" > 
  at <VRow class="d-flex justify-center" > 
  at <VResponsive class="d-flex text-center fill-height" > 
  at <VContainer class="fill-height" > 
  at <BaseLayout onVnodeUnmounted=fn<onVnodeUnmounted> ref=Ref< Proxy { <target>: Proxy, <handler>: {…} } > > 
  at <RouterView> 
  at <App> runtime-core.esm-bundler.js:40:16

I noticed, that for the list items, the missing params: {id: 1} which causes the failure seems to be inherited from the parent layout, since the path showing in the browser is correct.

Building and running the app, leads to the following error:

Error: Missing required param "id"
...

Vuetify Version: 3.2.3 Vue Version: 3.2.47 Vue Router: 4.1.6 Browsers: FireFox 113.0 OS: Manjaro

Update: After 4 months without any reasonable solution for this problem I'm using MatthewAry's workaround for now, which works quite well for me. I updated my issue repo with his workaround and hope this issues will get resolved in the future.

1Luc1 avatar May 10 '23 08:05 1Luc1

@MatthewAry Could you please explain how your hack/fix applies to the bug i reported earlier ( #17614 ), As described in this bug , we are unable to navigate to the /applications route from /applications/1/about ( or any other specific application page ) , and vuetify is complaining about missing params ( and your solution shows how we can pass the params ).

However i don't understand why we would need params to navigate to /applications. The missing route param id should only be needed when navigating to /applications/1/about or /applications/1 ( specific application ). I am successfully able to open a specific application and navigate to a tab. The missing param error happens when trying to move back to list of applications. ( /applications ).

Also once you try navigating to /applications and this error occurs, it completely breaks routing in the app. You can't even navigate to / home page. Would appreciate if you could fork the sandox with your hack.

rachitpant avatar Jun 15 '23 14:06 rachitpant

I was able to do this and your patch resolves the issue in my codebase also. Thanks.

rachitpant avatar Jun 16 '23 17:06 rachitpant

https://github.com/vuejs/vue-test-utils/issues/2069

Linking this here since it seems relevant.

cadriel avatar Jun 27 '23 21:06 cadriel

@johnleider can we expect this to be fixed in blackguard?

MatthewAry avatar Jul 19 '23 17:07 MatthewAry

@johnleider can we expect this to be fixed in blackguard?

We don't have someone specifically on it atm. Would you be interested in taking the reigns on this?

johnleider avatar Jul 19 '23 18:07 johnleider

I might have the confidence to take this on in the future, but right now I don't because this bug is weird and the actual cause of the problem is unknown. It would be better to figure out the cause then to just make a workaround. With that said, I think a workaround solution is possible.

We can probably use RegEx, Matched Routes, and Resolve methods in the router to solve this; but extensive tests would need to be written to ensure that all functionality detailed https://router.vuejs.org/guide/essentials/route-matching-syntax.html is maintained.

This code might help but I'm not that great with RegEx so there might be bugs:

function resolvePath(path, params) {
    // Handle optional params
    path = path.replace(/\?/g, "\\?")

    // Handle repeating params
    path = path.replace(/\*/g, "(\\/[^/]+)*")
    path = path.replace(/\+/g, "(\\/[^/]+)+")

    // Handle custom regex
    path = path.replace(/:\w+\([^\)]*\)/g, "([^/]+)")

    // Extract all dynamic params
    const paramMatches = path.match(/:[^/()]+/g)

    if (!paramMatches) return path

    // Replace each param with its value
    let resolvedPath = path
    for (const param of paramMatches) {
      const name = param.slice(1)
      if (name in params) {
        resolvedPath = resolvedPath.replace(param, params[name])
      }
    }

    return resolvedPath
}

Using the router's resolve method could return a path for a named route but without any of the params applied. The function above can then take the params provided in an object and try to apply them to the path by matching against the path's keys. This solution however leaves the actual router out of a part of the route resolution process. Honestly it would be better if the router would allow us to provide an excessive amount of params and let it handle it instead of doing something like this.

MatthewAry avatar Aug 07 '23 19:08 MatthewAry

Hello,

we had the same issue and we resolved it by upgrading some dependencies (with yarn upgrade-interactive) :

  • vueuse/core, 10.5.0 ❯ 10.7.2
  • vue-router, 4.2.4 ❯ 4.2.5
  • vue-tsc, 1.8.4 ❯ 1.8.27
  • vuetify, 3.4.9 ❯ 3.4.11

AlexianMasson avatar Jan 29 '24 10:01 AlexianMasson

Hello,

we had the same issue and we resolved it by upgrading some dependencies (with yarn upgrade-interactive) :

  • vueuse/core, 10.5.0 ❯ 10.7.2
  • vue-router, 4.2.4 ❯ 4.2.5
  • vue-tsc, 1.8.4 ❯ 1.8.27
  • vuetify, 3.4.9 ❯ 3.4.11

Sadly, for us this did not work out. We were already on the mentioned versions and still experience the bug. Hoping for a timely resolution of this issue.

HYPE-Thomas avatar Jan 30 '24 10:01 HYPE-Thomas

We're looking for help on this issue.

johnleider avatar Jan 30 '24 16:01 johnleider

We experienced such problems too. I reported https://github.com/vuejs/router/issues/2229 because for us, it currently looks like there is a problem with the router.

markusheiden avatar May 07 '24 07:05 markusheiden

We observed that this bug caused the router callbacks to be no longer called when such an error occurred once.

markusheiden avatar May 07 '24 14:05 markusheiden

After some debugging, I have found that this issue happens when you have your route definitions in vuetify (VListItem, VBtn, VTab etc. to param) refers to a route that needs param but it is not defined.

For example with this structure:

// redacted for simplicity
const routes = {
  name: 'mainEntity',
  path: 'entity/:id',
  children: [
    {
       name: 'entityItems',
       path: 'sub-entity'
    }
  ]
}

Inside the component/page of mainEntity route if you have a route definition with the name of child component it throws error when you are leaving that page.

<v-tab :to="{ name: 'entityItems' }" >

If you define your parent props the error is gone:

  <v-tab :to="{ name: 'entityItems', params: { id } }" >

While navigating to that page it doesn't throw error because I believe it inherits the props from the parent or currentroute. After you try to leave the page containing that false route definition the reactivity works (computed, refs etc.) and because of the different currentRoute which does not contain the prop it throws error.

I hope this helps someone.

ayZagen avatar May 11 '24 14:05 ayZagen

Updated reproduction. https://github.com/MatthewAry/vuetify-17176 Also, I think I might have found another bug of some kind having to do with the router.

MatthewAry avatar Jun 03 '24 19:06 MatthewAry

If I could find a way to avoid throwing an error and just log a warning when parameters are missing, it would make things much easier!

The issue is particularly problematic when navigating to a different page (like from a product page to the products list page) where an inherited parameter, such as product_id, is missing. The problem arises when using Vuetify's VBtn or Vue Router, as they trigger router.match.stringify before the component is actually destroyed (even though it shouldn't, since it's within a computed property). This causes the app to crash in development mode.

pajuhaan avatar Aug 17 '24 08:08 pajuhaan