vue-router: No stacktrace on NavigationDuplicated error uncaught in promise

Version

3.1.1

Reproduction link

https://jsfiddle.net/Lom7hxn5/

Steps to reproduce

  1. Call $router.replace or $router.push twice with same path

What is expected?

Error object provides stacktrace (stack property)

What is actually happening?

No stack trace in the error object


When NavigationDuplicated error is thrown, there is no stack trace in the error itself so when error is reported through error reporting service like Sentry, there is absolutely no context to go with the error and so it’s hard to figure out where that error originated.

Transpiled code for the NavigationDuplicated class looks like this: https://github.com/vuejs/vue-router/blob/5141def3a21da479c2858c0d70db3cb438c3b7d0/dist/vue-router.common.js#L1935-L1946

which is problematic because instantiating such function won’t create a stacktrace. It would work fine with non-transpiled code (although I’m not suggesting to not transpile it): https://github.com/vuejs/vue-router/blob/44c63a963f05ede229d8658a45b2388e244ce00b/src/history/errors.js#L1

One possible solution would be to manually set stack trace with:

this.stack = (new Error()).stack;

but I’m not sure how that would be done if transpilation is done by webpack/babel…

There is also one extra bug in that code. The line that instantiates error passes a route object to the error: https://github.com/vuejs/vue-router/blob/fc42d9cf8e1d381b885c9af37003198dce6f1161/src/history/base.js#L125 but error doesn’t do anything with it: https://github.com/vuejs/vue-router/blob/44c63a963f05ede229d8658a45b2388e244ce00b/src/history/errors.js#L2

Saving route properties on the error object would also help in deciphering source of the error.

About this issue

  • Original URL
  • State: closed
  • Created 5 years ago
  • Reactions: 25
  • Comments: 35 (15 by maintainers)

Commits related to this issue

Most upvoted comments

I understand it may be a bit confusing to see the UncaughRejection error in the console, so let me summarize what it’s all about:

The error you see in the console is part of the new promise api: before, if no callbacks were supplied to router.push, errors were only sent to the global router error handler. Now, because both push and replace return a promise, if the navigation failure (anything that cancels a navigation like a next(false) or next('/other') also counts) is not caught, you will see an error in the console because that promise rejection is not caught. However, the failure was always there because trying to navigate to same location as the current one fails. It’s now visible because of the promise being rejected but not caught.

This behavior doesn’t happen with router-link: no errors are emitted (except globally). This is because it’s catching the error by adding a noop callback, so you don’t need to catch anything when using router-link. You can catch the rejection when doing programmatic navigation and ignore it:

router.push('/location', () => {})

But if you are using router.push in your code and you don’t care about the navigation failing, you should catch it by using catch:

router.push('/location').catch(err => {})

The last version makes more sense as the Promise api is likely to become the default and the callback version to become deprecated.

This isn’t a breaking change because the code still works the same it was before. The only exceptions are if you were doing one of these:

return router.push('/location') // this is now returning the promise
await router.push('/location') // this explicitly awaits for the navigation to be finished

However, router.push never returned anything before 3.1, so both cases were invalid usage. Even worse, if you were waiting for the resolution of router.push, it never worked because the function wasn’t returning a promise, so the navigation wasn’t even resolved nor failed after calling push. Now that it returns a Promise, it is possibly exposing bugs that were not visible before.

If you want to handle this globally, you can replace Router’s push/replace functions to silence the rejection and make the promise resolve with the error instead:

import Router from 'vue-router'

const originalPush = Router.prototype.push
Router.prototype.push = function push(location, onResolve, onReject) {
  if (onResolve || onReject) return originalPush.call(this, location, onResolve, onReject)
  return originalPush.call(this, location).catch(err => err)
}

I discourage from swallowing the errors like this because doing await router.push() will always resolve even when the navigation fails.

With the new navigation failures, you can replicate vue-router-next behavior:

import Router from 'vue-router'

const originalPush = Router.prototype.push
Router.prototype.push = function push(location, onResolve, onReject) {
  if (onResolve || onReject)
    return originalPush.call(this, location, onResolve, onReject)
  return originalPush.call(this, location).catch((err) => {
    if (Router.isNavigationFailure(err)) {
      // resolve err
      return err
    }
    // rethrow error
    return Promise.reject(err)
  })
}

This was reported already at https://github.com/vuejs/vue-router/issues/2872 and https://github.com/vuejs/vue-router/issues/2873. This issue is about improving the stack trace of the Rejection so if you could keep the conversation about that, it would help me a lot! I hope that this answer, being more detailed brings clarity to the topic. I’ve also been looking at how this impacts Nuxt with their core team so the error doesn’t appear if it shouldn’t.

hey @posva thanks so much for the thorough breakdown. I think the confusion is coming into play regarding what you described when you said:

“However, the failure was always there because trying to navigate to same location as the current one fails. It’s now visible because of the promise being rejected but not caught.”

While I understand that the above is true, I don’t think it was unreasonable for anyone to assume that the router’s default behavior would be to handle the NavigationDuplicated usecase “out of the box”… I’ve been using vue-router for several years, and I thought this was just default functionality all along.

I’m absolutely a fan of the promise API and see some immediate benefits, but I’ve had to rollback vue-router to a pre 3.1 version because I don’t have a way of cleaning up the errors that are being thrown without adding catch to all of my router.push’s.

Perhaps we can spin up a separate “feature request” issue for handling NavigationDuplicated out of the box?

I’m glad you could find it useful @shayneo !

While I understand that the above is true, I don’t think it was unreasonable for anyone to assume that the router’s default behavior would be to handle the NavigationDuplicated usecase “out of the box”

I want to be clear about this: it wasn’t handled out of the box before, the navigation failure was ignored. It was a behavior that could cause bugs (as explained in the comment) and we are now making it visible.

I’ve had to rollback vue-router to a pre 3.1 version because I don’t have a way of cleaning up the errors that are being thrown without adding catch to all of my router.push’s.

Yes, you do have a way to do it, I added a code snippet to override the function if the errors in the console are too noisy. You can also include a console.warn(err) inside to keep track of them and progressively migrate. Let me know if there are improvements to apply or cases it doesn’t handle.

Having an option to silence that specific navigation failure doesn’t seem like a good api choice imo, as it’s a failure like others and I think people just got used to not having to handle it. It’s achievable in user land with the snippet above though. It’s something that could evolve in future major versions, but this behavior is consistent with the callback version of navigation functions. For it to evolve it will be necessary to redefine what a navigation success means (everything that isn’t a success is a failure and makes the Promise rejection). Right now, navigation succeeds if the navigation resolves correctly to what we pass as the first argument (the target location). Any guard navigation aborting or redirecting, not navigating (same location) as well as errors, abort the navigation, making it reject. If you think this isn’t the right way, it would help to discuss it in an RFC with pros/cons and other alternatives

Compatible with version 3.1 below, no catch error is read

const originalPush = Router.prototype.push
Router.prototype.push = function push (location, onResolve, onReject) {
  if (onResolve || onReject) return originalPush.call(this, location, onResolve, onReject)
  try {
    return originalPush.call(this, location).catch(err => err)
  } catch (error) {
    console.log(error)
  }
}

@frlinw Errors are being improved, there were no proper errors before so some rejections still need to be improved

I’m just wondering… am I supposed to guard every route to prevent clicks if the route matches the current route? — Or is this just an issue with router?

@posva It’s not related to NavigationDuplicated but to stacktrace. on a this.$router.replace({ name: 'name' }) I have an error and I would love to know why but the false is not really helpful here image

If someone is interested in TypeScript version you could use the following snippet:

/*
* Preventing "NavigationDuplicated" errors in console in Vue-router >= 3.1.0
* https://github.com/vuejs/vue-router/issues/2881#issuecomment-520554378
* */

const routerMethods = ['push', 'replace'];

routerMethods.forEach((method: string) => {
  const originalCall = (Router.prototype as any)[method];
  (Router.prototype as any)[method] = function(location: any, onResolve: any, onReject: any): Promise<any> {
    if (onResolve || onReject) {
      return originalCall.call(this, location, onResolve, onReject);
    }
    return (originalCall.call(this, location) as any).catch((err: any) => err);
  };
});

The code above also modifies .replace method

This error sounds semantically wrong. In theory, you should be able to redirect (a.k.a push) route to be anything at anytime. Say I am in /route and upon clicking on the let’s say app logo (which redirects to/ again), this is not an error - it’s a very common user behaviour. I don’t know what was the use case for such feature but I should not implement any router logic in my application, plus suppressing errors currently sounds more dangerous then downgrading the version - which I am doing atm.

upgraded to most recent and I now I get this exception. This was introduced as part of the 3.1 new promise api. This seems to be a breaking change as I have to change code to accommodate for the new version to keep the same behavior as before. Because the exception makes the router or page act erratic, this would seem to break semvers “add functionality in a backwards compatible manner”.

Related: https://github.com/vuejs/vue-router/issues/2880 https://github.com/vuejs/vue-router/issues/2878 https://github.com/vuejs/vue-router/issues/2872

Api change - https://github.com/vuejs/vue-router/issues/2873

This RFC improves navigation failures and should address some of the problems people were facing regarding the uncaught promise rejections: https://github.com/vuejs/rfcs/pull/150

Like @LanFeusT23 I’ve also had issues where NavigationDuplicated error is thrown when trying to push with a new query param.

Is there a recommended path for this use case? It’s often desired for things like pagination for example if I’m currently at myapp.com/things?offset=0 I might want to…

this.$router.push({ name: this.$route.name, query: { offset: 10 }})

This issue is still occurring when trying to programmatically navigate to the same route with a different query parameters. For example, if I have a method as follows:

navigateToUser(userId) {
    this.$router.push({ name: "UserRoute", query: { id: userId } })
}

Clicking once works fine, as there is no id set in the query parameters yet. However upon 2nd click the route will throw an error of NavigationDuplicated.

Not only that but putting a catch(err => {}) to the end of the push method will stop the error from being shown but also prevent the url from updating.

The only solution we found is to first replace the url with empty query parameters and then push the correct query params, but that completely breaks the user experience of being able to go back to previous URLs

the navigation failure was ignored. It was a behavior that could cause bugs (as explained in the comment) and we are now making it visible.

Having an option to silence that specific navigation failure doesn’t seem like a good api choice imo, as it’s a failure like others and I think people just got used to not having to handle it.

@posva

This is about usability.

I have about a million of these buttons live in production:

Html: <footer @click="navigate">Help</footer> Js:

methods: {
    navigate() {
        this.$router.push("/help");
    }
}

This footer is visible and clickable on hundreds of pages, and users often click footer links to a page they are already on (in this case the help page). A user can come and click a link in this footer a million times, even when they are already on the page the link routes to. In fact my production error logs are 99% this NavigationDuplicated error.

There have been 5+ threads about this issue. The user/developer/customer is telling you that this a problem. You have a big usability issue on your hands here that is being ignored.

Do you really think that the user navigating to a route they are already on is an error?

It seems to me - and apparently a lot of other vocal people - that this is a normal (successful even) navigation and not an error. Navigating to a route you are already should not log an “error”. It might not be a perfect “success” but it shouldn’t reject the promise as a failure. Rejection is for when the thing you expected to happen did not happen. In this case, the thing I expected to happen (stay on the same page) - successfully happened. It’s a success, with a warning. I really don’t care how the vue-router currently defines “success” and “failure” as a result of a promise - because the way you currently are doing it defies your userbase’s (a bunch of angry entitled developers) perception of what a success/failure is - and that is why so many threads on this exist. You would think that the router should be smart enough to properly announce (warn) NavigationDuplicate instead of clogging my production error logs with an error that is actually just normal, expected behavior.

Can you give me a use case where anyone would actually want to catch navigation duplicated in production?

Because I can give you a million cases where no one would want to catch it in production. Having a link on a page - to a page that you are already on - is not an error.

Do you really think the proposed solutions are acceptable?

  1. Do you really think anyone wants to use router-link tags everywhere when an @click will do?
  2. Do you really think anyone wants to write custom code to wrap the push/replace calls with a custom catch just to filter out/silence this exception? Isn’t this why we have libraries?
  3. Do you really think anyone wants write .catch() after every push call this.$router.push("/help").catch(err => err); and have and lose the ability to catch real errors or have a million .catch() functions sprinkled throughout our codebases?
  4. Do you really think anyone wants to let this error pollute their server side production logs of all client errors?

This was a bad design decision. Usability comes first. You are going to fix this by redefining what a success and failure is. And I am about 20 seconds away from forking the whole vue-router over this and making a “not-sh*t-vue-router” lib.

For now, I’m using a workaround like this: this.$router.currentRoute.name !== 'Home' && this.$router.replace({ name: 'Home' }) It will avoid the NavigationDuplicated

The problem only happens on Sentry

Technically that’s not exactly true. Exception is stack-less regardless if running with sentry or not but when having devtools open, engine logs some extra stuff so you can actually see a (separate) stacktrace. But if you have devtools closed and trigger the error then you won’t have that.

But that’s a detail. Thanks for fixing.

So, do we have to handle catch from now on or this is something which may get completely different? Should I update router.push and other affected everywhere in my app?

How to handle “NavigationDuplicated” error globally? I could not swallow this error using onError hook. I use push (replace) function in lots of places and I would not want to guard every route too.

After many iterations I’ve created better testcase at https://jsfiddle.net/Lom7hxn5/ (also updated original comment).

Notice that in your screenshot you have two chevrons to expand. The one you’ve expanded is not actually the error object itself but something else (I think promise rejection exception). The error itself doesn’t have a stack trace.

My new testcase just catches promise rejection and console.log’s the error. That should also show stack trace but doesn’t currently. Note: If using console.error instead of console.log, there will again be second chevron with stack trace but that’s not from the error object itself.