vuejs / router

🚦 The official router for Vue.js
https://router.vuejs.org/
MIT License
3.93k stars 1.19k forks source link

Undocumented Regression in beforeRouteLeave Guard When next() is Used #901

Closed amxmln closed 3 years ago

amxmln commented 3 years ago

Version

4.0.6

Reproduction link

https://github.com/amxmln/vue-router-next-issue-reproduction

Steps to reproduce

What is expected?

The toast shows up, but the URL stays the same until the "Discard and Leave" button is clicked (and thus next() is called).

What is actually happening?

The URL (but not the router-view) changes, the navigation history gets updated. Closing the toast does not restore the current route.


In a Vue2 app with vue-router@3, I used to prevent navigating away when there were unsaved changes with a beforeRouteLeave-guard similar to this one:

beforeRouteLeave(to, from, next) {
    if (this.forceNavigation)  next();
    else if (this.unsavedChanges) {
      this.$store.commit('addToast', {
        action: next,
        actionLabel: 'Discard Changes',
        message: 'You have unsaved changes, do you want to discard them?',
        type: 'warning',
      });
    } else next();
  },

This successfully prevented the navigation until the button in the toast was clicked. Ignoring or closing the toast would do nothing (as expected). Even when clicking a navigation link multiple times, this would only result in more toasts being created, but no navigation (again, as expected).

This construct does no longer work in Vue3 with vue-router@4. While trying to rewrite the code to use a promise instead (and awaiting that one), I noticed that the URL changes while the promise is still pending and only changes back once false is returned in the guard. I don’t think that’s what users are expecting and it feels like a bug, which is why I’m reporting it—in case it’s not a bug, but a conscious choice, I think that should probably be mentioned in the Migration Guide somewhere, since there’s a bunch of SO and Vue Forum threads proposing similar solutions for Vue2.

Thank you for your hard work!

posva commented 3 years ago

You were not passing the right arguments in the template and not always calling the next() callback. Here is your fixed version:

<template lang="html">
  <div class="about">
    <h1>About</h1>
    <p>
      Clicking the button below or the "Home" link will "delay" the navigation and show a toast
      below instead, which would allow the user to either navigate, or cancel.
    </p>
    <p>
      This does not work as expected: the URL changes immediately and doesn’t change back. Also,
      going back multiple times will eventually unload the Vue app.
    </p>
    <button @click="$router.back()">Go Back</button>
    <ul class="toasts">
      <li v-for="(toast, index) in toasts" :key="index">
        {{ toast.message }}
        <button @click="handleToastAction(index)">{{ toast.actionLabel }}</button>
        <button @click="closeToast(index)">Close</button>
      </li>
    </ul>
  </div>
</template>

<script>
export default {
  beforeRouteLeave(to, from, next) {
    this.addToast({
      message: "You have unsaved changes, would you like to leave and discard them?",
      action: next,
      actionLabel: "Discard and leave"
    });
  },
  data() {
    return {
      toasts: []
    };
  },
  methods: {
    addToast(toast) {
      this.toasts.push(toast);
    },
    closeToast(index) {
      const toast = this.toasts.splice(index, 1)[0];
      toast.action(false);
    },
    handleToastAction( index) {
      const toast = this.toasts.splice(index, 1)[0];
      toast.action();
    }
  }
};
</script>
amxmln commented 3 years ago

Hi and thank you for looking into this. I’ve implemented your suggested fix in my example (and pushed it to the repo), but the problem persists: clicking the "Go Back" button more than once changes the path despite the toast being shown and eventually causes the application to be unloaded. I think

Your suggestion made navigating through router-link more predictable—not calling next(false) on close was an oversight on my part—but it doesn’t seem to work with $router.back(). I did some more testing in vue-router@3 and noticed a similar behaviour.

I’ve also noticed that even clicking repeatedly on a <router-link> doesn’t yield the expected behaviour: only the very first toast will allow to discard and leave, the others just close the toast, but don’t navigate.

posva commented 3 years ago

That behavior when clicking back multiple times is normal and cannot be changed

amxmln commented 3 years ago

Alright good to know, thank you. I guess the best solution would be to immediately cancel the navigation then and start a new one when the button is clicked, which would require something like https://github.com/vuejs/vue-router/issues/3453 to be implemented, so it can be properly reconstructed.

Thank you again for your time and all the hard work you put into this project! 😊

whjw6 commented 2 years ago

@amxmln Have you solved the issue? I met the same issue with vue-router@4.0.16. The prevent dialog cannot only be popped once because the url in address bar has been changed.

amxmln commented 2 years ago

@whjw6 I have not found a proper solution for this yet, sorry. I still believe that cancelling the navigation immediately and then reconstructing it would be the only way to solve this currently, however, the feature I mentioned above still hasn't been implemented, unfortunately.

teckel12 commented 1 year ago

@amxmln I hope you've found a solution already for this. But if you haven't, or others find this issue as I have, I believe this is a major problem.

Basically, guards like beforeRouteLeave work well as long as you stay within the Vue Router methods. However, when you go back() or go() Vue Router simply passes the control over to the browser to navigate and the URL changes right away (as does the route history). Therefore, the path changes regardless of the guard. As @amxmln points out, this is a huge problem in many applications (mine are enterprise-level PWAs).

To accommodate this, the first thing I did was to create a "routeHistory" plugin which uses other guards to add routes to a store. I can't share the code as it's a corporate project, but you should be able to find sample code of creating route history. I also created a few getters to do things like get the current and previous route data (which is also used extensively to know where the user came from, which is important in many cases). Also, mutations to addRoute, updateRoute, deleteRoute, etc. Basically, using the Vue Router guards to create a perfect route history. I only store the last 10 routes, as I can't think of a reason why more than even a couple would be useful. But, store more if you have the need.

Then, once there's a correctly working routeHistory store, a back() action was created to replace the Vue Router back(). All it basically does is a router.push() to the previous route in the routeHistory store. Because it's doing a push(), the Vue Router guards will trigger and NOT go back as the guard blocks it. If your guard (beforeRouteLeave) allows, in the .then() you simply clean up the routeHistory so it's all correct (I believe this is just two .shifts() on the routeHistory).

After this was built, I simply did a search/replace for everywhere in the app where we were doing a router.back() and replaced it with a routeHistory.back(). Everything works perfectly.

Soapbox: I believe Vue Router should have a routeHistory built right in (this has always annoyed me). I've yet to do a project where I didn't need to know information about the previous route. Just storing the last 10 routes would be enough. If this was done, the .back() and .go() functions could be rewritten to .push() to the desired route and then correctly adjust the routeHistory. This is an easy lift, but it would be very valuable and resolve the problem identified in this issue.