-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Comments
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 router.beforeEach(() => {
// `router` is already available here.
}) In the case of |
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 Router.beforeEach = myVariable; Are there any stronger reasons for keeping a NULL this for these 2 hooks ? |
As Thorsten said, can you please provide a real case scenario when using |
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. |
@LinusBorg I think it would be useful for
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:
This doesn't work as there is no way to access the I can get around this by:
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 Making |
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.
|
Seems like this can be monkey patched in with:
That makes For me it makes sense that The 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 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 👍 |
My real-world use-case for this is to get access to the Vuex store via I followed the pattern provided by the "vue-hackernews-2.0" example and returned a factory from my When I got around to integrating vue-router in my app and needed access to the store in I unfortunately had to refactor the app so that By the way, I found MANY questions of users asking "how can I access the Vuex store in |
@berniegp Well, while I understand some of the scenarios prove to be a challenge, There are solutions to this, even with a store from a factory function, e.g. exporting that instance it from // main.js
export { store }
Not exactly sexy but doesn't have any ugly side effects. iIf we follow the initial feature request though, and make
|
@LinusBorg I completely agree and understand that a component must not be created before I don't agree with 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 :
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. |
How come 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. |
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. |
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 also seems to make code unnecessarily complicated and increases the learning curve, as someone has to know that using Because of this, I already had to review some code were |
The way I got to resolve this (access
Then, on a setup file for Vue 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 |
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.
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. |
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). |
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 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 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. |
My 2c is that for |
I'm closing this in favor of #3166 |
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):
confirmTransition()
- invoke the hook with the router instance as THISbindEnterGuard()
- invoke the hook with the router instance as THISThe text was updated successfully, but these errors were encountered: