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

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

Closed
amxmln opened this issue Apr 23, 2021 · 7 comments
Closed

Comments

@amxmln
Copy link

amxmln commented Apr 23, 2021

Version

4.0.6

Reproduction link

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

Steps to reproduce

  • Navigate to the About-route
  • Try to navigate away from the about route with the navigation menu, the button, or the browser back button
  • The beforeRouteLeave guard will kick in and show a toast, but not navigate away
  • Clicking the "Discard and Leave" button will actually perform the navigation, clicking close only gets rid of the "toast"
  • Clicking "Go back" multiple times will eventually unload the app since the navigation history is updated, despite next() not being called

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
Copy link
Member

posva commented Apr 26, 2021

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>

@posva posva closed this as completed Apr 26, 2021
@amxmln
Copy link
Author

amxmln commented Apr 26, 2021

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
Copy link
Member

posva commented Apr 26, 2021

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

@amxmln
Copy link
Author

amxmln commented Apr 26, 2021

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 vuejs/vue-router#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
Copy link

whjw6 commented Aug 16, 2022

@amxmln Have you solved the issue? I met the same issue with [email protected]. The prevent dialog cannot only be popped once because the url in address bar has been changed.

@amxmln
Copy link
Author

amxmln commented Aug 21, 2022

@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
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants