$router.push() followed by redirect in a route's beforeEnter hook causes uncaught promise rejection

[berniegp]

Version

3.1.3

Reproduction link

https://jsfiddle.net/asu3qnry/

Steps to reproduce

To trigger the uncaught promise rejection, click on "/home" and then the button.

What is expected?

No uncaught promise rejection because there are actually no errors generated by the application. It's simply a navigation ($router.push) that generates a redirect (next('/foo')) in a beforeEnter hook.

What is actually happening?

Console reports: Uncaught (in promise) undefined

---

Using a router-link to /bar instead of the initial $router.push('/bar') does not produce the error.

Related to #2833 #2881

[posva]

I already answered this at #2833 (comment)

@berniegp unfortunately, this feature request will do nothing to help you with that. It's is related to the new promise api and errors are stil being improved (there were not before). You can find more at #2881 (comment)

Please, check the related issues, the reasoning has been verbosely explained there already

[berniegp]

@posva I did read your previous reply and the referenced issue #2881 in its entirety. Here's why I thought this issue is different in more detail:

1. NavigationDuplicated is mentioned all over the place in No stacktrace on NavigationDuplicated error uncaught in promise #2881 but it is not involved at all in my jsfiddle. For reference, here is the stack trace from my jsfiddle in Chrome:

   Uncaught (in promise) undefined
   (anonymous) @ vue-router.js:2057
   abort @ vue-router.js:2088
   (anonymous) @ vue-router.js:2137
   beforeEnter @ Inline Babel script:25
   iterator @ vue-router.js:2126
   step @ vue-router.js:1852
   step @ vue-router.js:1856
   runQueue @ vue-router.js:1860
   confirmTransition @ vue-router.js:2153
   transitionTo @ vue-router.js:2040
   push @ vue-router.js:2371
   (anonymous) @ vue-router.js:2785
   push @ vue-router.js:2784
   click @ Inline Babel script:10
   

   And the code at abort @ vue-router.js:2088:

   // base.js
   
   var abort = function (err) {
     // after merging https://github.com/vuejs/vue-router/pull/2771 we
     // When the user navigates through history through back/forward buttons
     // we do not want to throw the error. We only throw it if directly calling
     // push/replace. That's why it's not included in isError
     if (!isExtendedError(NavigationDuplicated, err) && isError(err)) {
       if (this$1.errorCbs.length) {
         this$1.errorCbs.forEach(function (cb) {
           cb(err);
         });
       } else {
         warn(false, 'uncaught error during route navigation:');
         console.error(err);
       }
     }
     onAbort && onAbort(err); // <-- Line 2088, note that err === undefined
                              // in my case so *not* a NavigationDuplicated instance
   };
   

2. My understanding is that the whole discussion in No stacktrace on NavigationDuplicated error uncaught in promise #2881 is centered around error handling. My use-case does not involve any error-generating code path. Here is where the abort() is triggered in confirmTransition():

   // base.js
   
   const iterator = (hook: NavigationGuard, next) => {
     if (this.pending !== route) {
       return abort()
     }
     try {
       hook(route, current, (to: any) => {
         if (to === false || isError(to)) {
           // next(false) -> abort navigation, ensure current URL
           this.ensureURL(true)
           abort(to)
         } else if (
           typeof to === 'string' ||
           (typeof to === 'object' &&
             (typeof to.path === 'string' || typeof to.name === 'string'))
         ) {
           // next('/') or next({ path: '/' }) -> redirect
           abort() // <----------- This causes onAbort to be called which is the Promise's
                   //              reject function passed from VueRouter.prototype.push()
           if (typeof to === 'object' && to.replace) {
             this.replace(to)
           } else {
             this.push(to)
           }
         } else {
           // confirm transition and pass on the value
           next(to)
         }
       })
     } catch (e) {
       abort(e)
     }
   }
   

3. I understood your comment in #2833 (comment) as telling me that my issue was separate from Handle Promise rejection in guards #2833.

I spent quite some time debugging, researching and reproducing this in a minimal test case and I honestly thought this issue was different from the others linked. Note also that, in an attempt to not add noise to the repo, my first step was to find another related issue and not hastily create a new one :). Of course all these issues are related to Promise rejection so maybe they will all be fixed the same way.

[berniegp]

For reference, here is the concrete use-case from my application:

// The route configuration
{
  path: '/',
  beforeEnter(to, from, next) {
    const auth = store.getters['auth/currentAuth'];
    if (auth) {
      next(getHomePageBasedOnAccountType(auth));
    } else {
      next({ name: 'login' });
    }
  },
}

// The code that redirects to the above route in a component
let formCancelPath;
// formCancelPath is set from somewhere (query param, history, etc.)
if (formCancelPath) {
  this.$router.push(formCancelPath);
} else {
  this.$router.push('/'); // <---- This path
}

[wooliet]

After reading through the other issues, I believe I'm in the same boat as @berniegp. A navigation guard that redirects results in the console error.

To confirm for my own sanity, I used vue cli to generate a project and then updated router.js with a third (valid) route that has a guard which will always redirect back to the root page:

    {
      path: '/abouttoo',
      name: 'about-too',
      component: AboutToo,
      beforeEnter: (to, from, next) => {
        next({
          path: '/',
          replace: true
        });
      }
    }

About.vue component is updated with a button that, when clicked, sends you to the new component:

<script>
export default {
  methods: {
    goToOtherPage() {
      this.$router.push({
        name: 'about-too'
      });
    }
  },
};
</script>

The call to goToOtherPage results in the "Uncaught (in promise) undefined" error.

---

Am I correct in understanding the proposed solution is to have all calls to push updated to look something like:

      this.$router.push({
        name: 'about-too'
      }, () => {});

That does suppress any console error. But is that the desired behavior?

[posva]

Any navigation failure is a promise rejection. I updated my comment at #2881 (comment) that explains everything. Let me know if there is anything to add

Some errors haven't been changed yet and that's why the uncaught error may be undefined or false in some scenarios

[wooliet]

Just to be clear and to make sure I'm not misunderstanding anything:

Are you saying that what we see now is a known "navigation error", generated by a defect in vue router, that was just never exposed?

In other words, my example above is a redirect from one valid path to another valid path with nothing else happening. There are no errors except the one reported by the router, which itself seems to be an error.

[wooliet]

if the navigation failure (anything that cancels a navigation like a next(false) or next('/other') also counts)

I see. This is a case of my misunderstanding what counts as a "navigation failure". The language used seems a bit off IMO, but I understand now.

[berniegp]

@posva Thanks for the clarification! However, I would argue that a redirection that cancels a navigation should not be a Promise rejection. Since rejection becomes a throw in the async/await model, it should only occur on actual errors. In my opinion, an expected control flow with a redirection should not have to add error handlers to avoid errors. While reading #2881 and the relevant documentation again, I could not find a description of what $router.push() actually resolves to; only that it should resolve (or reject) when navigation is complete.

The example code you gave in #2881 inspired me to create this global push() override:

const router = new VueRouter({ /* ... */ });

const originalPush = router.push;
router.push = function push(location, onResolve, onReject) {
  if (onResolve || onReject) {
    return originalPush.call(this, location, onResolve, onReject);
  }
  return originalPush.call(this, location).catch((err) => {
    if (err) {
      // If there really is an error, throw it
      // Should probably be more sophisticated based on the type of err
      return Promise.reject(err);
    }
    // Otherwise resolve to false to indicate the original push call didn't go to its original
    // destination.
    // TODO: return the final route instead of false?
    return Promise.resolve(false);
  });
};

Since the precise resolve/reject behavior of push() doesn't seem set in stone, would it be possible to resolve to the final route (or false if navigation aborted) instead of rejecting when there is no error?

[richterdennis]

If got this Error in my console:
Uncaught (in promise) TypeError: Cannot read property 'toString' of undefined

After a while of debugging, this is due to the abort call described above with undefined as error.

Maybe a better solution is throwing a real error like NavigationCanceled or something.

Calling router push with empty function fixed it magically: $router.push('/', () => {})

[websitevirtuoso]

Have the same problem. I have another question I have big project and now I should
implement this $router.push('/', () => {}) everywhere why?
@berniegp good solution but why it is not in core package?

[estani]

Why is this issue still closed? The "supposedly" related issues where closed but this is still valid (which show they weren't the same)
The proposed solution of @berniegp works, so please reopen the issue and review the proposal to either allow a PR or reject it with at least some feedback.

[andreasvirkus]

My main question is that if the router throws an exception, does it mean the users (me/us) are doing something wrong? If not, then I think it's unwarranted.

For example, @richterdennis proposed a NavigationCancelled error - but if we conditionally cancel a navigation in a hook, then why would we consider it an error? It's not actionable in any way.

[lichaozhy]

v3.0.7 looks good! I also think this bug is another.

[KCMertens]

I was still getting uncaught promise rejection errors in the chrome debugger, even when calling router.push('/').then(e => {}), and I think I figured out the (subtle!) source of the bug.

vue-router/dist/vue-router.esm.js

Line 2786 in 0c2b1aa

this$1.history.push(location, resolve, reject);

This line here creates a new promise that immediately runs.
The result of that is that any .catch() you may have on the outside hasn't attached yet, as the push() call has not returned yet.

Following the call tree, I end up with a stack looking like this (the fourth frame, router.ts:133, is a beforeEach() that performs a redirect using next()):

Looking at the frame in question

vue-router/dist/vue-router.esm.js

Line 2060 in 0c2b1aa

onAbort(err);

the onAbort being called here (synchronously) is the default reject from the promise all the way back in the first snippet!

---

Wrapping the push call with a timeout like so solves the unhandled rejection, as the push() call now has time to return the promise, and the caller can actually register catch() before the promise rejects.

VueRouter.prototype.push = function push (location, onComplete, onAbort) {
    var this$1 = this;

  // $flow-disable-line
  if (!onComplete && !onAbort && typeof Promise !== 'undefined') {
    return new Promise(function (resolve, reject) {
      setTimeout(() => this$1.history.push(location, resolve, reject), 0);
    })
  } else {
    this.history.push(location, onComplete, onAbort);
  }
};