Redesigning Navigation failures and global handlers

[posva]

Rendered

[ducmerida]

@posva
Maybe it would better to use a NavigationResult instead of NavigationFailure, with this you can handle all the navigation result cases with the same logic:

• Cancelled
• Duplicated
• Redirected
• Succeeded (or another word like Completed/Performed/Done to make a clear difference with Redirected that is also considered as a success state)

I say it because I think that is not very natural to check if the result of a function is undefined to check if its execution success, I think that is better to handle all with the same logic and always return a result with a context data, in the case of a failure you can include the Error with the stacktrace and in the case of a redirect you can include some information about the final route.

[posva]

Using a NavigationResult is indeed another possibility. The pattern I'm using comes from Node with error
first callbacks. I think it being natural or not varies on backgrounds but it's definitely very present in JS because of Node.

The reason I prefer using this approach is because there is no distinction in the succeeding cases: when navigation succeeds, a new currentRoute is set. For Redirections, there is redirectedFrom that appears to be able to treat it

[StefanFeederle]

Does this mean pushing the current route again will resolve the promise with a NavigationFailureType 'duplicated' instead of rejecting?

The old behaviour encouraged adding .catch((err) => {}) to all router.push() to get rid of duplicated rejections. This seems like an antipattern. With the proposed changes, all empty errorHandling can be deleted if no further logic in route.push().then() is needed. This is awesome!

I prefer the approach as suggested by @ducmerida to always return a NavigationResult.
This is easy to understand for beginners and handling the different outcomes can be done with the same logic. Is this possible while still providing error stacks on failed attempts?

[posva]

Does this mean pushing the current route again will resolve the promise with a NavigationFailureType 'duplicated' instead of rejecting?

Yes.

I prefer the approach as suggested by @ducmerida to always return a NavigationResult. This is easy to understand for beginners and handling the different outcomes can be done with the same logic.

That's not really true. A NavigationResult would be redundant as it would contain the previous location and current location:

const from = {...useRoute()} // create a copy of the current route
await router.push('/...')
const currentLocation = useRoute()

You still need extra logic to differentiate a failure from a success whereas with only failures, you can simply do:

const failed = await router.push('/...')

if (failed) {
  // handle failure
} else {
  // handle success
}

compared to

const result = await router.push('/...')

if (result instanceof Error) {
  // handle failure
} else {
  // handle success
}

It's also important to note that the most common use case is waiting for the navigation to be finished without failure, so making that path easy to check is valuable:

if (!await router.push('/...')) // after navigation

compared to

if (!((await router.push('/...') instanceof Error)) // after navigation
// which would probably be written as
const result = await router.push('/..')
if (result) // after navigation

Failures are usually useful for debugging and tracing back what blocked the navigation, that's why them being Error instances is so useful

Is this possible while still providing error stacks on failed attempts?

That depends on how a NavigationResult would be implemented

[trainiac]

A problem I've found is there is no way to achieve certain functionality with vue router when writing a universal web app without throwing an error. However throwing errors now causes unhandled rejections for push and replace.

Here are a couple use cases:

• Sending users to a not found page when visiting /users/:userId for a userId that doesn't exist. To handle this, in the beforeRouteEnter hook I throw an error. The server side routing handles the error by responding with response code 404 and the not found page html. The client side router will listen to the router.onError and if the error is a "not found" type the page will reload the url which allows server side to show the not found page.
• Redirecting users as a permanent redirect vs temporary redirect. To handle this, in the beforeRouteEnter hook I throw an error that contains the information required for the server side to respond appropriately.

My question about all of these is how do I achieve the same thing without throwing an error?
It feels like there should to be a way to communicate more information to your global handlers for what you want to happen without having to throw an error. Maybe there is some way to set your own NavigationResult? next accepts a NavigationResult? I dunno just spit balling here.

[posva]

@trainiac that seems to be a case of redirection and you can handle it by checking the property redirectedFrom. You don't need to throw an error. As for how to achieve it, there is an example at https://github.com/posva/rfcs/blob/router/navigation-failures/active-rfcs/0000-router-navigation-failures.md#redirections

[trainiac]

@posva Thanks for responding. Forgive me if I'm missing something but I don't see how this answers my question. How can one tell from redirectedFrom whether the desired redirect should issue a 301 or a 302? This seems like an important piece of a server side navigation result.

[posva]

Deciding of its a 301 or 302 is up to you. From the router perspective it’s just a redirection

[posva]

This RFC is now in final comments stage. An RFC in final comments stage means that:

The core team has reviewed the feedback and reached consensus about the general direction of the RFC and believes that this RFC is a worthwhile addition to the framework.
Final comments stage does not mean the RFC's design details are final - we may still tweak the details as we implement it and discover new technical insights or constraints. It may even be further adjusted based on user feedback after it lands in an alpha/beta release.
If no major objections with solid supporting arguments have been presented after a week, the RFC will be merged and become an active RFC.

[snoozbuster]

Proposal seems good to me (have run into both NavigationDuplicated and "Promise rejected with undefined" errors recently after updating vue-router). I see that this is targeted for v4 - is there a chance this can instead be released with a minor version update of v3 (as such an update introduced the issues with promises in the first place)? would like to adopt this in my company's app ASAP and would prefer not to worry about larger breaking changes in v3 -> v4.

[posva]

@snoozbuster It's great you are supporting this change but because it is a breaking change, it can only be released as a major bump

[snoozbuster]

@posva all well and good, but where was that mentality when promisifying router.push and router.replace was pushed as a minor bump? that was also a breaking change - I find it hard to believe that the solution for a breaking change introduced to consumers of this library by a minor bump is to absorb even more breaking changes caused by a major bump. What is the path to resolution for this issue for consumers which cannot upgrade to v4 (which unless I am mistaken, also requires an upgrade to vue 3.0)?

[posva]

@snoozbuster It wasn't a breaking change as described at vuejs/vue-router#2881 (comment). There are temporary solutions to prevent Promise rejections for navigation failures and the NavigationFailureType can be exposed in vue-router as well.
Introducing a different behavior would have been inconsistent with onError and afterEach and the existing callbacks. This had to be done with a breaking change to prevent further confusion.
The comments and reports on that regard helped me to create this RFC, which solves the biggest problem of uncaught promise rejections for navigation failures and makes global hooks consistent with push.

[snoozbuster]

@posva ahh, I had seen that issue but only got as far as the link to this RFC, not all the way up to that comment, so I was expecting this RFC to resolve the issue.

I don't strictly agree with the statement that "it wasn't a breaking change" because I was neither returning nor awaiting router.replace() before the Promise change, but I wasn't getting errors logged until the API changed from under me which caused our global unhandled Promise rejection handler to start logging (a huge volume of) errors. But, I also don't really agree with the idea that a duplicated navigation is a failure, either... in my eyes that's a no-op resulting in success, so I was surprised to see errors where I wasn't expecting any.

All of our duplicate navigation issues are tied to one terribly janky piece of code anyway (and seeing all these errors made me realize it's even more janky than I thought it was), so I will handle them there for now and wait in anticipation for this new API change. Thanks for explaining - feel free to mark these last couple comments as off-topic.

[snoozbuster]

ahh, this will help too. is there a better way to check for navigation failures right now than e && e.name === 'NavigationDuplicated'?

[posva]

There is no documented way to check for duplicated navigations right now (it's private api)

[champkeh]

@posva
Hi posva, after read the rfc and router's doc, I have some confuse
in rfc, example is this:

import { NavigationFailureType, isNavigationFailure } from 'vue-router'

router.push('/').then((failure) => {
  if (failure) {
    // Having an Error instance allows us to have a Stacktrace and trace back
    // where the navigation was cancelled. This will, in many cases, lead to a
    // Navigation Guard and the corresponding `next()` call that cancelled the
    // navigation
    failure instanceof Error // true
    if (isNavigationFailure(failure, NavigationFailureType.canceled)) {
      // ...
    }
  }
})

(link: https://github.com/vuejs/rfcs/blob/master/active-rfcs/0033-router-navigation-failures.md#changes-to-the-promise-resolution-and-rejection)

But, in router's doc, example is this:

const { isNavigationFailure, NavigationFailureType } = VueRouter

// trying to access the admin page
router.push('/admin').catch(failure => {
  if (isNavigationFailure(failure, NavigationFailureType.redirected)) {
    // show a small notification to the user
    showToast('Login in order to access the admin panel')
  }
})

(link: https://router.vuejs.org/guide/advanced/navigation-failures.html#detecting-navigation-failures)

I confuse that one is then and another is catch which capture the failure, so which is right place to get failure?

[posva]

The catch is for Vue Router 3. I fixed the typo on vue router 4 docs

[mariusa]

In Vue 3, how can one navigate to the same route, but with different query parameters?
None of these answers work: https://stackoverflow.com/questions/59428227/how-to-use-router-push-for-same-path-different-query-parameter-in-vue-js

Thanks

[iamandrewluca]

@mariusa I think the navigation is done. Your component data/derived data, should be reactive maybe 🤔 it should change when the query changes 👀

https://codesandbox.io/s/summer-rgb-zo4him?file=/src/App.vue

[mariusa]

You're right, I've added a watch on props.param and it triggers! Thanks :)