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

Providing the router instance as THIS for router guards "beforeEach" and "beforeRouteEnter" #2118

Closed
tmcdos opened this issue Mar 20, 2018 · 19 comments

Comments

@tmcdos
Copy link

tmcdos commented Mar 20, 2018

What problem does this feature solve?

In the current version of the router (3.0.1) the global guard beforeEach and the component guard beforeRouteEnter do not provide a valid THIS value. Obviously for beforeEach if you want to access the app property of the router you need a valid THIS in the hook. And since the component is not yet instantiated in beforeRouteEnter you can not access the component as THIS - but at least you should be able to access the router instance (and its app property).

What does the proposed API look like?

I am proposing the following 2 simple changes in the code (file /src/history/base.js):

  1. in function confirmTransition() - invoke the hook with the router instance as THIS
    this.pending = route
    const iterator = (hook: NavigationGuard, next) => {
      if (this.pending !== route) {
        return abort()
      }
      try {
        hook.call(this, route, current, (to: any) => { /// <----- proposed change
  1. in function bindEnterGuard() - invoke the hook with the router instance as THIS
  return function routeEnterGuard (to, from, next) {
    return guard.call(this, to, from, cb => { /// <---- proposed change
      next(cb)
      if (typeof cb === 'function') { 
@LinusBorg
Copy link
Member

Just to understand why this is useful, can you explain a usecase where you need to access the app in a guard? It feels like an anti-pattern to me.

Abouit the proposed changes:

In the case of beforeEach, you already have the router instance in the scope when you add the hook:

router.beforeEach(() => {
  // `router` is already available here.
})

In the case of beforeRouteEnter i don't think it's a good idea to have its this point to the router whhile its siblings, beforeRouteUpdate and beforeRouteLeave have this point ot the component instance. While I realize that you propose this because this can't point to the component instance in beforeRouteEnter, pointing it ot the router feels like a hack.

@tmcdos
Copy link
Author

tmcdos commented Mar 22, 2018

I understand your feeling - you know the inner working better than me, I am a simple user of the library. I still think that it is better to have a valid THIS (even if it points to the Router and not the component) rather than none. Of course others may have a different opinion and probably can show some good reasons why a NULL for this is preferable.

Regarding the beforeEach - sometimes I might not want (or not able) to define the hook as a closure or even in the same source file. It is entirely possible to provide a variable holding reference to function as an argument:

Router.beforeEach = myVariable;

Are there any stronger reasons for keeping a NULL this for these 2 hooks ?

@posva
Copy link
Member

posva commented Mar 22, 2018

As Thorsten said, can you please provide a real case scenario when using this is the only possible solution to refer to the router? It would help to know what you're trying to achieve that requires the router instance

@tmcdos
Copy link
Author

tmcdos commented Mar 23, 2018

I do not think such a case (where it is the only possible solution) exists - you can always find workarounds. I am curious why the workarounds path is preferred instead of the clean solution. Can you please share your thoughts about the possible implications and potential problems that you envision ?

What I actually need is not the router instance but the Vue app instance. Yes, the app instance can be reached using other means - but this sounds too unconvincing "There are workarounds so you will not get the natural way of access, and this is a final decision".

I believe in the Open Source idea for sharing contributions - so I am trying to share mine. Everyone is free to accept or decline it.

@Worthaboutapig
Copy link

@LinusBorg I think it would be useful for this to point to the router instance during guards. To provide a use case and address this point:

router.beforeEach(() => {
  // `router` is already available here.
})

This seems to assume you have a monolithic set of routes. I'm splitting my route definitions to make them easier to manage, so I have:

// --------------------------------------
// Router creation
import Router from 'vue-router'
import { someRoutes } from './someRoutes'

const router = new Router({
  routes: someRoutes
})
// --------------------------------------

// --------------------------------------
// someRoutes file definition
import { utils } from './utils'
import { Somewhere, SomewhereElse } from './components'

export const someRoutes =  [{
  {
    path: '/somewhere',
    name: 'Somewhere',
    component: Somewhere,
    beforeEnter: async (to, from, next) => {
      if (utils.loggedIn()) {
        next()
      } else {
        this.push('/SomewhereElse')
      }
    },
    {
      path: '/somewhereElse',
      name: 'SomewhereElse',
      component: SomewhereElse
    }
  }
}]
// --------------------------------------

This doesn't work as there is no way to access the router itself inside the guard.

I can get around this by:

// --------------------------------------
// Router creation
import Router from 'vue-router'
import { getRoutes } from './someRoutes'

const router = new Router({
  routes: someRoutes
})

router.addRoutes(someRoutes(router))
// --------------------------------------

// --------------------------------------
// someRoutes file definition
import { utils } from './utils'
import { Somewhere, SomewhereElse } from './components'

export function getRoutes(router) {
  return [
    {
      path: '/somewhere',
      name: 'Somewhere',
      component: Somewhere,
      beforeEnter: async (to, from, next) => {
        if (utils.loggedIn()) {
          next()
        } else {
          router.push('/SomewhereElse')
        }
      },
      {
        path: '/somewhereElse',
        name: 'SomewhereElse',
        component: SomewhereElse
      }
    ]
  }
}
// --------------------------------------

However, this adds a function call and a level of indirection and complication that doesn't seem either useful or necessary. The route definition files now are function calls, which isn't nice. How should I handle this situation?

It seems natural to me that this inside a guard (or other method of the Router) would point to the router object. (This assumes I'm using the guard correctly, I suppose, but it works ;) )

Making this point to the router shouldn't be a breaking change either, should it? It isn't expected to point to anything at the moment, is it, though it seems to point to the list of routes.

@sullivanpt
Copy link

Just to understand why this is useful, can you explain a use case where you need to access the app in a guard? It feels like an anti-pattern to me.

Probably not the right forum for this question, but I cannot think of a use case where I would NOT want access to the app (especially the vuex store) from inside a guard function. Maybe what @LinusBorg is suggesting here is that an explicit "import" of the store (for example to get the auth object) might be more clear to the reader?

Anyway, I just stumbled across how they solve this in nuxt. It's pretty elegant. They just "bind" their beforeEach handler to their app.

router.beforeEach(render.bind(_app))

@oles
Copy link

oles commented Oct 16, 2018

Seems like this can be monkey patched in with:

router.options.routes
    .filter(route => route.component.hasOwnProperty('beforeRouteEnter'))
    .forEach(route => {
        route.component.beforeRouteEnter = route.component.beforeRouteEnter.bind({$store: store})
    })

That makes this.$store available in beforeRouteEnter from my initial exploration.

For me it makes sense that beforeRouteEnter shouldn't have access to its components properties. beforeRoute... makes it clear that this is before anything else that happens inside the component.

The this in beforeRouteEnter should however have access to the other this properties, as it seems to be quite a common use case to check for something in your Vuex store in that guard.

I used to import the store, along with all the other people having the same problem it seems, but it seemed so out of place. The mixes of this.$store and store inside the components confuses.

I have no idea if my mokey patch is an okay way to do this - I'd love some input - and even more for an official way to do this 👍

@berniegp
Copy link

My real-world use-case for this is to get access to the Vuex store via Vue.$store.

I followed the pattern provided by the "vue-hackernews-2.0" example and returned a factory from my store/index.js (see https://github.com/vuejs/vue-hackernews-2.0/blob/master/src/store/index.js).

When I got around to integrating vue-router in my app and needed access to the store in beforeRouteEnter(), I was at a loss. If the store module returns a factory function, there is no way to access the store instance used in the Vue app from the route guard. My thinking was to follow what is said here: https://router.vuejs.org/guide/advanced/data-fetching.html#fetching-before-navigation

I unfortunately had to refactor the app so that store/index.js returns a singleton store instance instead. I feel that vue-router should ideally not impose such artificial design constraints on the app. This feature request would solve this particular use-case.

By the way, I found MANY questions of users asking "how can I access the Vuex store in beforeRouteEnter()?" This feature would resolve all these questions at the same time :)

@LinusBorg
Copy link
Member

LinusBorg commented Mar 12, 2019

@berniegp Well, while I understand some of the scenarios prove to be a challenge, this as a reference to the component instance is any will continue to be unavailable in beforeRouteEnter because we will not create a component instance before the navigation has been confirmed, as we would otherwise potentially create unwanted side effects from beforeCreate and created() lifecycle hooks in the component. That's simply unacceptable side-effect.

There are solutions to this, even with a store from a factory function, e.g. exporting that instance it from main.jsand requiring it inline:

// main.js
export { store }
beforeRouteEnter() {
  const store = require('../main.js').store
}

Not exactly sexy but doesn't have any ugly side effects.

iIf we follow the initial feature request though, and make this point to the router instance, there would also be this.app.$store.

this pointing to something unexcepted isn't nice either, though.

@berniegp
Copy link

@LinusBorg I completely agree and understand that a component must not be created before beforeRouteEnter. To do so would be completely at odds with the definition of that guard. It was not my intention to imply otherwise so sorry about the confusion. I meant to talk about giving access to router.app.$store as you described.

I don't agree with this being the router instance (or for that matter anything else besides a component or undefined) in a component's beforeRouteEnter. I think this would break sound OO design principles (even if this is js) and not be in-line with the other component guards.

However, I think there is merit in giving access to the router instance in the guards. It would provide a way to access the root Vue instance, the Vuex store, the router options, routes definitions, etc without resorting to global variables or other "hacks". My limited experience with Vue doesn't show me any obvious disadvantage with giving access to that context. My understanding is that all the other guard methods can have access to the router somehow already.

How to give access to the router (and more importantly, in my case, to the Vue app) is a question of its own. I see 3 options :

  • Through this
  • Add another argument to the guard
  • Add a property to one of the existing arguments (perhaps to next) that gives access to the router.

I'm not experienced enough with the Vue codebase to know which one is better. My first instinct however would be to add another argument.

@ianwalter
Copy link

How come this in beforeRouteEnter can't just be the app instance? No one is saying that the component properties/etc need to be there, right? I think it's clear that doesn't make sense since it's not created yet. But the app instance exists when beforeRouteEnter gets called and if this was the app instance you would be able to access $router, $route, $store, etc like you do elsewhere in the component. It would also make re-using code between beforeRouteEnter and beforeRouteUpdate easier.

Even if there is a technical issue with what I said above, I think this issue deserves more attention than it's gotten in the past. I've seen it be a stumbling block for devs we've onboarded who are new to Vue over the years and it's even more challenging with a SSR setup as @berniegp mentioned above. Thanks for everyone's time.

@tmcdos
Copy link
Author

tmcdos commented Mar 26, 2019

For those of you who do not want to wait until the proposed changes are officially implemented - you can use https://www.npmjs.com/package/patch-package or https://www.npmjs.com/package/custompatch to patch Vue-Router in your projects.

@posva posva added this to Doable or in refactor (low prio, low complex) in Longterm Mar 26, 2019
@sebiniemann
Copy link

sebiniemann commented Apr 17, 2019

For me, code snippets like

<template>
  <div>Hello: {{ honorifics }} {{ $store.firstname }}</div>
</template>

<script>
import store from './store';

export default {
  async beforeRouteEnter(to, from, next) {
    await store.loadPerson();

    next();
  }

  computed: {
    honorifics() {
      switch(this.$store.gender) {
        case: 'female':
          return 'Mrs.';
        // ... you got the point ;)
      }
    },
  },
};
</script>

always looked very weird/inconsistent, for having this.$store/$store (without the need to import the store) and store(with the need to import it) in a single component file. Especially since retrieving some data and using it afterwards seems to be a very common use case.

This also seems to make code unnecessarily complicated and increases the learning curve, as someone has to know that using store in a navigation guard is fine, but this.$store is favoured within the component.

Because of this, I already had to review some code were store was added to data, to use it within the template section. Reason was that $store was not yet used in this particular component and the author was completely unaware of $store (since nothing was stopping anybody from using store instead of this.$store anywhere within the script section), as he only saw the usage of store.

@erickwilder
Copy link

erickwilder commented Jul 12, 2019

The way I got to resolve this (access router.app.$store) inside guards defined on separate modules without an explicit import of either router or store.

export function vuexBasedGuard(router) {
  return (from, to, next) => {
    if (router.app.$store.state.someting) {
       router.push('/something')
    } else { 
      router.push('/something-else')
    }
  }
}

Then, on a setup file for Vue Router:

import { vuexBasedGuard } from './guards.js'

const router = new Vue.Router(...)
router.beforeEach(vuexBasedRouter(router))

This way I can avoid the explicit import and use a mocked router + store combination to test the guards with dependency injection instead of risking leaking test state because of a globally available store variable imported by guards.

@markkimsal
Copy link

If you're trying to do the data fetch before navigation pattern as described on the vue-router page, there doesn't seem to be a way to handle errors w/o global scope.

    beforeRouteEnter: function (to, from, next) {
        loadConnections(to.params.id).then( function(data) {
            next(function(vm) {
                Vue.set(vm.$root.$data, "connectionList", data.data);
                vm.$root.busyLoading = false;
            });
        }, function() {
            //TODO: how do you reference the
            //existing app w/o global scope?
            router.app.busyLoading = false;
            next(false);
        });
    },

You can eventually get access to the component on success, but not on failure. I guess using flags to show busy loading stuff is a transition anti-pattern? I don't know how I can show a spinner and stop it on both success and failure.

@luxaritas
Copy link

I'd like to chime in with a use case of my own. I'm currently implementing SSR, and need to have access to my axios instance for data pre-fetch, which is defined within my root Vue instance $options (since it needs to be created fresh for each request, putting it on the Vue prototype is a no-go).

@bponomarenko
Copy link

From my side, I also have a use case, which doesn't really have a workaround for now. So it is a blocker for us.

In our setup we tried to utilize beforeRouteEnter to pre-load data via Vuex store module. To do so, we cannot simply import store because the same components re-used between different applications (with different store instances), but rely on the same store modules. Here is an example component to illustrate it:

export default {
  storeModules: {
    'module-namespace': {
      state: { ... },
      getters: { ... },
    },
  },
  // Usually, this registration is happening in a global mixin, but for simplicity I put it here
  beforeCreate() {
    Object.entries(this.$options.storeModules)
      .forEach(([namespace, storeModule]) => {
        this.$store.registerModule(namespace, storeModule);
      });
  },
  beforeDestroy() {
    Object.keys(this.$options.storeModules)
      .forEach(namespace => {
        this.$store.unregisterModule(namespace);
      });
  },
};

Let say, there is loadData action in store module. So, in beforeRouteEnter we would have to register module, call loadData action and call next() on success or next(err) on error and unregister store module. In order to achieve that, we are missing two things – access to the store and to component $options. If those would be available as another argument to the hook, that would be just perfect:

export default 
  ...
  async beforeRouteEnter(to, from, next, context) {
    const store = context.router.app.$store;
    // Register module from context.componentOptions.storeModules
    store.registerModule( ... );

    try {
      await store.dispatch('loadData');
    } catch (error) {
      store.unregisterModule(...);
      next(error);
    }
  },
};

I understand that main development focus right now on v4 version, but this feature request (maybe not with exact api like in example above) would really be helpful for a lot of people.

@posva posva moved this from Doable or in refactor (low prio, low complex) to To consider (do not agree with reasoning or could be exposed as plugin, needs more discussing) in Longterm Apr 6, 2020
@kierans
Copy link

kierans commented May 21, 2020

My 2c is that for beforeEnter et al, the app vm should be this so that the guard can use properties of the app to make a decision about whether to proceed or not.

@posva
Copy link
Member

posva commented Aug 3, 2020

I'm closing this in favor of #3166

@posva posva closed this as completed Aug 3, 2020
@posva posva moved this from To consider (do not agree with reasoning or could be exposed as plugin, needs more discussing) to Done in Longterm Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests