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

How to make the directives react on state changes? #35

Open
thapar opened this issue Apr 6, 2020 · 12 comments
Open

How to make the directives react on state changes? #35

thapar opened this issue Apr 6, 2020 · 12 comments

Comments

@thapar
Copy link

thapar commented Apr 6, 2020

I am using vue-browser-acl's custom directive as such:

<template>
  <div>
      <BaseSideNavigationMenu v-role:authenticated />
      <PageContent />
  </div>
</template>

With the following setup and rule:

Vue.use(Acl, store.state.currentUser, (acl) => acl.rule("authenticated", () => store.getters.isAuthenticated));

And the following in my store:

export default new Vuex.Store({
  state: {
    currentUser: JSON.parse(window.localStorage.getItem("currentUser") || "{}"),
  },
  mutations: {
    SET_CURRENT_USER(state, user) {
      state.currentUser = user;
      window.localStorage.setItem("currentUser", JSON.stringify(user));
    },
    SET_NO_CURRENT_USER(state) {
      state.currentUser = {};
      window.localStorage.setItem("currentUser", "{}");
    },
  },
  actions: {
    async login({ commit }, login_data) {
      try {
        const response = await axios.post("/api/login", login_data);
        const currentUser = response.data.currentUser
        commit("SET_CURRENT_USER", currentUser);
        return response;
      }
    },
    async logout({ commit }) {
      const response = await axios.post("/api/logout");
      commit("SET_NO_CURRENT_USER");
    },
  },
  getters: {
    isAuthenticated(state) {
      return Object.keys(state.currentUser).length > 0;
    }
  },

However, when a user does a login, the <BaseSideNavigationMenu v-role:authenticated /> element does not show unless the page is refreshed. Likewise, upon logout, the BaseSideNavigationMenu element remains visible until the page is refreshed.

How can vue-browser-acl's directive be made to react on state changes?

@mblarsen
Copy link
Owner

mblarsen commented Apr 6, 2020

Check the index file in the example folder.

The user parameter must be a function if it is loaded dynamically.

// Since the user is asynchronously fetch we
// provide a function to fetch it in this case
// we access it through the Vuex store.
const user = () => {
  return store.getters.user
}

Vue.use(Acl, user, (acl) => { ... })

@thapar
Copy link
Author

thapar commented Apr 6, 2020

@mblarsen Upon further tinkering, I noticed that certain elements are reactive with v-role:authenticated tacked on, and others are not. I made the following changes, as per your suggestion, but the outcome of which elements are reactive still hasn't changed:

Vue.use(Acl, () => store.getters.currentUser, (acl) => acl.rule("authenticated", () => store.getters.isAuthenticated));

And I updated the getters portion of the store to include the following:

getters: {
    isAuthenticated(state) {
      return Object.keys(state.currentUser).length > 0;
    },
    currentUser(state) {
      return Object.keys(state.currentUser).length > 0 ? state.currentUser : {};
    }
}

The following elements are responsive as expected:

<span v-role:authenticated.not>Login</span>
<span v-role:authenticated>{{ currentUser.name }}</span>

But the following elements are not responsive:

<LoginForm v-role:authenticated.not />
<LoggedinMenu v-role:authenticated />

The full project code can be seen by following the links above or here.

@mblarsen
Copy link
Owner

mblarsen commented Apr 7, 2020

Can you please test if this works:

<LoginForm v-if="$can('authenticated')" />

@mblarsen
Copy link
Owner

mblarsen commented Apr 7, 2020

I looked at the repo you shared.

All of the following isn't really about the issue you've raised. Just some observations. Hope you don't mind :)


When you add () => store.getters.currentUser you provide a function that Acl uses to pass as the first parameter to the rules you define.

So your rule should ideally be:

-  acl.rule("authenticated", () => store.getters.isAuthenticated)
+  acl.rule("authenticated", (user) => Boolean(user))

If the user is present you are authenticated. Although your currentUser-getter is returning an "empty" object if the user is not there. Maybe null would be better?

What you've done isn't wrong. Just a bit confusing. They idea of the Acl is to ask "can the current user do x" or "what is true about the current user" - so ideally you should resolve your logic from the user's perspective as I've shown above.

Also:

<LoginForm v-role:authenticated.not />

Is kind of misuse of the semantics. The authenticated isn't really a role. You could perhaps instead create a rule called login.

<LoginForm v-can:login />

But really when it comes to UI flow I'd use the store getters directly in your case:

<LoginForm v-if="!authenticated" />
<LoggedinMenu v-if="authenticated" />

For me it is all about how the code reads. If "authenticated do this if not do that".

@thapar
Copy link
Author

thapar commented Apr 7, 2020

Can you please test if this works:

<LoginForm v-if="$can('authenticated')" />

This works reactively as expected. Can the v-role syntax not be reliably used reactively?

@thapar
Copy link
Author

thapar commented Apr 7, 2020

I looked at the repo you shared.

All of the following isn't really about the issue you've raised. Just some observations. Home you don't mind :)

When you add () => store.getters.currentUser you provide a function that Acl uses to pass as the first parameter to the rules you define.

So your rule should ideally be:

-  acl.rule("authenticated", () => store.getters.isAuthenticated)
+  acl.rule("authenticated", (user) => Boolean(user))

If the user is present you are authenticated. Although your currentUser-getter is returning an "empty" object if the user is not there. Maybe null would be better?

What you've done isn't wrong. Just a bit confusing. They idea of the Acl is to ask "can the current user do x" or "what is true about the current user" - so ideally you should resolve your logic from the user's perspective as I've shown above.

Also:

<LoginForm v-role:authenticated.not />

Is kind of misuse of the semantics. The authenticated isn't really a role. You could perhaps instead create a rule called login.

<LoginForm v-can:login />

But really when it comes to UI flow I'd use the store getters directly in your case:

<LoginForm v-if="!authenticated" />
<LoggedinMenu v-if="authenticated" />

For me it is all about how the code reads. If "authenticated do this if not do that".

"authenticated" is only a simplified example. In the final application, there will be "moderator", "admin", "superadmin", etc. But as per your last example with v-if="authenticated" it seems like you're saying not to use vue-browser-acl 😭. I like the syntax of separating ACL permissions for components and routes from the business logic (the code will have plenty of other v-ifs to deal with that aren't related to ACL permissions). Also, this blogpost makes excellent points for using your vue-browser-acl library (and I like the syntax and flexibility of this library).

@mblarsen
Copy link
Owner

mblarsen commented Apr 7, 2020

This works reactively as expected. Can the v-role syntax not be reliably used reactively?

I haven't had any issues with it previously. When you work Vuex and async in general things change a bit. Anyway, I'll have to look more into to your issue. Thanks for testing the $can part.

Can you try this too:

+<div v-if="authenticated || !authenticated">
    <LoginForm v-role:authenticated.not />
    <LoggedinMenu v-role:authenticated />
+</div>

Where authenticated is the getter from your store.

Never mind the logic. This is just to see what happens if it works after being forced to re-render by a parent. This is to exclude the issue being something else.

@thapar
Copy link
Author

thapar commented Apr 7, 2020

This works reactively as expected. Can the v-role syntax not be reliably used reactively?

I haven't had any issues with it previously. When you work Vuex and async in general things change a bit. Anyway, I'll have to look more into to your issue. Thanks for testing the $can part.

Can you try this too:

+<div v-if="authenticated || !authenticated">
    <LoginForm v-role:authenticated.not />
    <LoggedinMenu v-role:authenticated />
+</div>

Where authenticated is the getter from your store.

Never mind the logic. This is just to see what happens if it works after being forced to re-render by a parent. This is to exclude the issue being something else.

Yes, it is reactive with the v-if wrapping.

@thapar
Copy link
Author

thapar commented Apr 8, 2020

Hey @mblarsen, just wondering if you may have had a chance to look into the reactivity issue?

@mblarsen
Copy link
Owner

mblarsen commented Apr 8, 2020

Hey @mblarsen, just wondering if you may have had a chance to look into the reactivity issue?

I have not. Don't expect any quick response. I cannot put so much time into it at the moment.

@josefsabl
Copy link

Can you please test if this works:

<LoginForm v-if="$can('authenticated')" />

This works reactively as expected. Can the v-role syntax not be reliably used reactively?

I am having exactly the same issue. Thanks for raising this up and coming to this workaround. I would however also appreciate if the better syntax would work reactively ;-)

@mblarsen
Copy link
Owner

mblarsen commented Sep 19, 2020

Hi @thapar @josefsabl

I've spend a little time updating the example (changes have been pushed).

Would you mind checking it out and try this flow:

  1. Log in as 'jane' in 'admin' group
  2. See the Admin paragraph appear at the top
  3. Log out and see
  4. See the Admin paragraph disappear again

I'm wondering how this (working) example is different from yours. When I log in and out the user object in the state is replace completely. Could that be what makes the difference? That you don't replace your state object.

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

No branches or pull requests

3 participants