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

Allow guards to return a promise instead of calling the next callback #177

Closed
ycmjason opened this issue Jun 5, 2020 · 9 comments
Closed
Labels

Comments

@ycmjason
Copy link

ycmjason commented Jun 5, 2020

Currently, vue router guards rely on the next callback function to identify the decision of the guard. E.g. next(error) to throw an error; next(false) to abort; next() to continue; next(path) to redirect to other paths.

I argue that this next callback style is quite an old fashioned way of doing things in JS and no longer idiomatic. I believe the main reason for this next callback was for asynchronous operations, just like most other next callbacks in other lib and apis.

Before filing an RFC, I would like to collect some opinion on replacing next with return (sync or async) and throw.

image

Screenshot from https://router.vuejs.org/guide/advanced/navigation-guards.html#global-before-guards

We currently have 4 variations of next. and I believe we can replace the first 3 with return and the last one with throw.

For example (copied from https://css-tricks.com/protecting-vue-routes-with-navigation-guards/):

async beforeEnter(to, from, next) {
  try {
    var hasPermission = await store.dispatch("auth/hasPermission");
    if (hasPermission) {
      next()
    } else {
      next({
        name: "login", // back to safety route //
        query: { redirectFrom: to.fullPath }
      })
    }
  } catch (e) {
    next(e)
  }
}

Can become

async beforeEnter(to) {
  try {
    var hasPermission = await store.dispatch("auth/hasPermission");
    if (!hasPermission) {
      return {
        name: "login", // back to safety route //
        query: { redirectFrom: to.fullPath }
      }
    }
    return;
  } catch (e) {
    // unneeded catch, but put here for demonstration.
    throw e;
  }
}

The benefits I can think of right now:

  1. We can remove the caveat of next() should be called only once. As shown in the image above.
  2. More idiomatic Javascript
  3. You don't have to take in all 3 arguments if you don't need them all.

I can't wait to hear your thoughts.

@aztalbot
Copy link

aztalbot commented Jun 5, 2020

Love this idea, particularly since I have tripped up using next in the past. This feels much more idiomatic and intuitive (assuming it's workable without sacrificing some essential router feature).

Would a void return (or any falsey return aside from maybe empty string) be equivalent to next(false)?

@posva posva added breaking change This RFC contains breaking changes or deprecations of old API. router labels Jun 5, 2020
@posva posva changed the title [vue-router] Use async/await instead of next in guards Allow guards to return a promise instead of calling the next callback Jun 5, 2020
@ycmjason
Copy link
Author

ycmjason commented Jun 5, 2020

Would a void return (or any falsey return aside from maybe empty string) be equivalent to next(false)?

That's a very very good point. I personally think falsey return should mean abort. But i can also see it being annoying to do return true every single guard. I will leave this an open question to encourage discussion.

@posva
Copy link
Member

posva commented Jun 5, 2020

I don't think falsy returns should abort because by default, no return is return undefined. Navigation abortion should be explicit, so return false is much safer and allows a navigation guard to not return anything to just pass

@posva
Copy link
Member

posva commented Jul 1, 2020

I was wrong with this being a breaking change, we can have both syntaxes. Here is the implementation: vuejs/router#343
beforeRouteEnter's next(vm => {}) hasn't been implemented yet, so it might require other changes

@posva posva removed the breaking change This RFC contains breaking changes or deprecations of old API. label Jul 1, 2020
@jods4
Copy link

jods4 commented Jul 1, 2020

@posva That's great!
For async Promises feel much more natural and "modern" JS than old-school callbacks.

Being able to simply return a value (sync or async) also looks and feels simpler than callbacks, it's a nice improvement! ❤️

@posva
Copy link
Member

posva commented Jul 1, 2020

Thanks!
Yes, I agree and it was @ycmjason 's point at the beginning. I was reluctant to do it because I wanted to introduce an extra information argument with navigation direction and type, but that can be added on to maybe

@ycmjason
Copy link
Author

ycmjason commented Jul 1, 2020

yay! how exciting!

@posva
Copy link
Member

posva commented Jul 8, 2020

@ycmjason I created #187 to replace this, let me know if I missed anything 😉

@posva posva closed this as completed Jul 8, 2020
@ycmjason
Copy link
Author

ycmjason commented Jul 8, 2020

@posva haha thanks! sorry for not writing up an rfc for this. I'm lazy.

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

No branches or pull requests

4 participants