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

No stacktrace on NavigationDuplicated error uncaught in promise #2881

Comments

@rchl
Copy link
Contributor

rchl commented Aug 8, 2019

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:

var NavigationDuplicated = /*@__PURE__*/(function (Error) {
function NavigationDuplicated () {
Error.call(this, 'Navigating to current location is not allowed');
this.name = this._name = 'NavigationDuplicated';
}
if ( Error ) NavigationDuplicated.__proto__ = Error;
NavigationDuplicated.prototype = Object.create( Error && Error.prototype );
NavigationDuplicated.prototype.constructor = NavigationDuplicated;
return NavigationDuplicated;
}(Error));

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):

export class NavigationDuplicated extends Error {

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:

return abort(new NavigationDuplicated(route))

but error doesn't do anything with it:
constructor () {

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

@posva
Copy link
Member

posva commented Aug 9, 2019

@rchl what navigator are you seeing this on? There are some limitations regarding error stacktraces that may lead to errors being uncomplete in browsers like IE9

@rchl
Copy link
Contributor Author

rchl commented Aug 9, 2019

I'm seeing this on every browser as it's not specific to a browser but to transpiled variant of the code. Instantiating such transpiled "class" is not gonna produce a stack trace as inheritance of Error is "faked" in that case (or at least not considered as proper subclass of Error by engines).

@posva
Copy link
Member

posva commented Aug 9, 2019

Weird, I do get a stack trace on Safari, Chrome and Firefox. IE is a bit of a different story

Screen Shot 2019-08-09 at 16 29 34

What are you expecting to see? Adding the stack manually or using Error.captureStackTrace yield the same stacktrace

@francoisromain

This comment has been minimized.

@posva

This comment has been minimized.

@rchl
Copy link
Contributor Author

rchl commented Aug 9, 2019

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.

@rchl
Copy link
Contributor Author

rchl commented Aug 9, 2019

Also here is a version using esm version of VueRouter (in browsers that support ES modules): https://jsfiddle.net/L8wykjns/5/

You can see that logged error actually has stack trace in this one.

@ghost

This comment has been minimized.

@ashtonian

This comment has been minimized.

@shayneo

This comment has been minimized.

@evgeniy-legkun

This comment has been minimized.

@vuejs vuejs deleted a comment from claudivanfilho Aug 12, 2019
@vuejs vuejs deleted a comment from MrSunshyne Aug 12, 2019
@vuejs vuejs deleted a comment from dawid-kokoszka Aug 12, 2019
@vuejs vuejs deleted a comment from hlynurl Aug 12, 2019
@posva
Copy link
Member

posva commented Aug 12, 2019

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 #2872 and #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.

@shayneo
Copy link

shayneo commented Aug 12, 2019

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?

@madmoizo
Copy link

madmoizo commented Aug 13, 2019

@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

@posva
Copy link
Member

posva commented Aug 13, 2019

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

@posva
Copy link
Member

posva commented Aug 13, 2019

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

@vuejs vuejs deleted a comment from mehmetkarakamis Aug 13, 2019
@jd-solanki

This comment has been minimized.

@akao19940209

This comment has been minimized.

@burakson

This comment has been minimized.

@posva

This comment has been minimized.

@shayneo

This comment has been minimized.

@posva

This comment has been minimized.

@posva
Copy link
Member

posva commented Mar 28, 2020

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

lfarrell added a commit to UNC-Libraries/box-c that referenced this issue Apr 28, 2020
…n error, see vuejs/vue-router#2881 (comment). It seems it's not supposed to throw the error if parameters are changed, so maybe a bug in vue-router?
bbpennel pushed a commit to UNC-Libraries/box-c that referenced this issue Apr 30, 2020
* Package updates

* Update for new output names

* Update permissions editor to vue-cli 4.3.x

* Update vue-cdr-access packages

* Router push needs a catch block, as navigating to same route throws an error, see vuejs/vue-router#2881 (comment). It seems it's not supposed to throw the error if parameters are changed, so maybe a bug in vue-router?

* * Update vue-cdr-access packages
* Security fixes

* Fix issue with js build not overwriting previous build

* Update vue-test-utils and fix tests. There were breaking changes in 1.0.0-beta.30
a365344743s added a commit to a365344743s/vue-page-stack that referenced this issue Jun 11, 2020
@FEA5T

This comment has been minimized.

@posva
Copy link
Member

posva commented Jun 17, 2020

You have a big usability issue on your hands here that is being ignored.

I did not ignore it. I listened to the feedback and came up with vuejs/rfcs#150 which was well received. It addresses your concern too.

And to answer your question, no, I don't think a duplicated navigation it's an error.

Locking this thread as it's outdated after the RFC and comments like yours, which are negative and clearly a rant with sentences like Because I can give you a million cases and I have about a million of these buttons live in production, it's not something I need to deal with.

@vuejs vuejs locked as resolved and limited conversation to collaborators Jun 17, 2020
@vuejs vuejs deleted a comment from SisodiyaAshwini Nov 10, 2020
@posva posva changed the title No stacktrace on NavigationDuplicated error No stacktrace on NavigationDuplicated error uncaught in promise Nov 10, 2020
@posva posva pinned this issue Nov 10, 2020
@posva posva unpinned this issue Jul 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.