Make `NavigationDuplicated` a no-op for `push` and `replace`

[dominik-bln]

What problem does this feature solve?

Since a recent change every call to push or replace that is handed the same path as is currently active is rejecting with a NavigationDuplicated error and clients that don't care about this duplication need to attach a no-op catch clause to all calls. I am aware that there was already a discussion on this topic in #2872 and can understand the viewpoints expressed by the maintainers there. However I'd still like to ask to reconsider this change and not reject for duplicated navigations.

The main arguments I would put into consideration against treating NavigationDuplicated as an error are:

• Not rejecting is more consistent with how <a> tags and <router-link> are working, i. e. I'd argue it is the API that is least surprising to users of vue-router because it is how browsers usually work.
• A client can already find out if a duplication is happening by looking at the current route and the push target if this functionality is desired. While this might be slightly more complicated for that use-case, the changed API puts a burden on all clients that don't care about the duplication, which is IMHO the more common case.
• It essentially seems not possible anymore to call push without an attached catch because otherwise we might get irrelevant error logs, i. e. the API is now always (albeit minimally) more complex.
• Introducing the rejection was effectively a breaking change of the API for a lot of clients. The observable behaviour of pushing the same navigation twice was a no-op before and now it isn't anymore.

What does the proposed API look like?

A duplicated navigation is a "no-op" or warning log in vue-router itself and doesn't require clients to handle this in any way.

[posva]

I'm happy to bring the conversation but I would like to see new points brought to the table. I did my best in explaining it at #2881 (comment) and will gladly add more precisions.

Regarding your points:

1. There is no rejection in a elements when clicking, that's why router-link behaves the same. push is however programmatic navigation and can appear in the middle of the code, therefore knowing if the navigation took place or not makes a difference
2. The navigation rejection happens because the navigation didn't take place. If we are on the url already, there isn't a navigation. This is indeed different from a regular a element because a always produce new requests and would therefore reload the page whereas router-link does not produce a full reload of the page, so having the page reloading if we are on the same page is undesirable. FYI there is an issue about forcing navigation at Force navigation when clicking on a router-link even if the route doesn't change #974 but it makes sense to not have it as a default in an SPA as the navigation happens client side.
3. and 4. were covered in the linked reply

I'm still open to suggestions and I do search things myself like always resolving and never rejecting but allowing users to detect errors by using instanceof Error. But nobody is putting up concrete examples to bring the discussion further, it always ends up being very abstract and I do read what people have to say about it in hope of advancing.
To me the point of having a rejection is being able to try catch the navigation to execute code only if the navigation succeeded and after the navigation happened.

For 3.x no changes are going to happen unless they are non-breaking, but I look forward to make things convenient and flexible for 4.x

I will open an issue to track and organize the discussion of should push reject or not and possible improvements about navigation failure

[dominik-bln]

Thank you for your quick reply.

There is no rejection in a elements when clicking,

Interesting, I was totally convinced that this would just do nothing and not reload.

I think the main opinion difference I, and probably other commenters had as well, is the way in which Promise.catch is to be used and there might just be two different camps.

I'd personally only reject for errors where it's absolutely certain that the client code must handle them (you could probably call this fatal errors, but I see that there are likely also a differences in what is to be considered inside this error class) and allow other error conditions to be handled inside the then block if the clients decide that they are important or with if-statements beforehand, e. g. something like this:

this.$router.push({ name: 'some-route'}).then((route) => {
  if (route.path === this.$route.path) {
    throw new Error('Navigation Duplicated')
  }
  // do the actual then processing
}).catch((error) => { console.error(error) })

I just remembered that the Apollo GraphQL client has a setting to switch between these approaches:

https://www.apollographql.com/docs/react/v2.5/api/react-apollo/#optionserrorpolicy

[dominik-bln]

So maybe this is a case that could benefit from configuration:

const router = new Router({
    routes: [],
    mode: 'history',
   // other options could be: 'reload', 'throw' and default to `throw` to avoid breaking changes
    duplicateNavigationPolicy: 'ignore'
})

There are three sort of concrete use cases of router.push and router.replace (while allowing genuine errors to bubble up):

1. Just get the application to the location:

router.push(location).catch(err => {
    if (err.name !== 'NavigationDuplicated') {
        throw err;
    }
});

If the router allows push to succeed for a duplicate location, this is just:

router.push(location);

2. Navigate to location and when we arrive, perform some callback but definitely don't call that callback multiple times.

router.push(location).then(raceyOperation);

If the router allows push to succeed for a duplicate location, this:

let acquiredLock = acquireLock();
router.push(location).then(route => {
    if (acquiredLock) raceyOperation(route);
});

3. Navigate to location and when we arrive, perform some callback, yes I want it done multiple times.

router.push(location).then(nonRaceyOperation).catch(err => {
    if (err.name === 'NavigationDuplicated') {
        nonRaceyOperation(route);
    } else {
        throw err;
    }
})

If the router allows push to succed for a duplicate location, this is just:

router.push(location).then(nonRaceyOperation);

I think use case 1 is probably the most common, but in all use cases but the racey operation, the code is cleaner allowing push to return the successful promise but the level of dirtiness of the racey operation is lower with manual deduplication then the messiness for the other use cases with the enforced deduplication. When performing a racey operation, the developer probably is most aware of the sensitivity of the operation and will want to guard that anyways. Then again, acquiring locks isn't that obvious to a lot of people.

I would avoid calling this navigation. The method is called push and people have intuitive ideas of what it means to navigate somewhere (you are already there being successful), but a push that doesn't modify the (navigation) stack has failed, so I am sympathetic to returning a failed promise for a push that doesn't actually modify the navigation stack. With replace it is harder to argue that this should fail, I can always redundantly replace an object with itself.

So one solution would be to add methods like navigateTo which redundantly succeeds for the same location.

I am in use case 1 all the time, so I think I am going to be replacing router.push with a function that redundantly succeeds:

router.duplicationErroringPush = router.push;
router.push = async function (location) {
  return router.duplicationErroringPush(location).catch((error) => {
    if (error.name === 'NavigationDuplicated') {
      return this.currentRoute;
    }
    throw error;
  });
};

[alexcroox]

I believe @dominik-bln suggestion of an ignore flag would be most beneficial.

Having to add a no-op catch to every single one of my $router.push/replace calls on the off chance they might link to the active page is madness.

[DarkLite1]

I came here as a newbie looking for why I got this error while already being on the page and the button got clicked again. Reading it all, I do think @dominik-bln suggestion of having an option for this would be great. In that case it's up to the consumer to decide what happens and it avoids having to add .catch on all calls

My use case:
Suppose you have a header toolbar with a profile button. When you click the profile button with your name on it and your avatar you end up on the MyProfile page. That button on top can still be clicked when you're already on the page. If that happens I feel that no error should be thrown.

Thank you for having another look at this.

[posva]

vuejs/rfcs#150 addresses this 🙂

[codeofsumit]

Landing here as I'm trying to find a solution to the NavigationDuplicated error. In our case, we simply want to remove all trailing slashes from a URL (by redirecting to the same URL without the trailing slash). But we only get NavigationDuplicated and there seems to be no solution to ignore that and simply do the navigation.

While trailingSlash: false in the nuxt config would make the original page a "different page" and therefore make the redirect work, trailingSlash: false breaks lots of pages in our app so I'm not sure if that's the path to go.

Essentially, we would like to get rid of the trailing slash (without a user error) in the URL.