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

Add vue directive #51

Closed
6 tasks
shadoWalker89 opened this issue Mar 31, 2018 · 20 comments
Closed
6 tasks

Add vue directive #51

shadoWalker89 opened this issue Mar 31, 2018 · 20 comments
Assignees
Labels

Comments

@shadoWalker89
Copy link

shadoWalker89 commented Mar 31, 2018

Hi,

It would be really nice if we could do something like

<div v-can="deleteSomething">Delete something</div>

Update:

Requirements:

  • create a separate Vue plugin (e.g., AbilityDirectives)
  • this plugin should provide global directive v-can
  • directive should work similar to v-if (i.e., remove/insert DOM nodes when condition changes)
  • an array as argument to directive v-can="[action, subject, field]" (e.g., v-can="['read', post, 'title']")
  • arguments as directive arguments v-can.action.field.of="subject", .of is optional, just for readability (e.g., v-can.read.title.of="post", v-can.read="post")
  • arguments as directive arguments for class checks v-can.action.field.of.subject, .of is optional, just for readability (e.g., v-can.read.title.of.Post, v-can.read.Post)
@stalniy
Copy link
Owner

stalniy commented Mar 31, 2018

You can with v-if

<button v-if=“$can(“delete”, post)”>delete</button>

@stalniy
Copy link
Owner

stalniy commented Mar 31, 2018

Could you please explain why do you want to add additional directive?

@shadoWalker89
Copy link
Author

Because of readability

@stalniy
Copy link
Owner

stalniy commented Apr 1, 2018

It was done as method because having this you can do much more. For example,

  1. Combine permissions logic with other conditions , like:
    v-if=“$can(‘read’, post) && isVisible”
  2. this allows to pass check as a property to anothe component like
<image-gallery :editable=“$can(‘update’, ‘Image’)” />
  1. I don’t see the way how I can pass 2 or 3 arguments (in case of checking per field permissions). Probably the only way is to pass array, isn’t it?

v-can can’t cover these cases, that’s why I decided not to create a separate directive. Also 3th point will push us to pass array as argument to directive, so eventually we you will see this v-can=“[‘read’, post]” and in my opinion it’s less readable. Maybe you want to suggest better way. I didn’t understood how you suggest to pass that arguments

@stalniy stalniy added the status: define requirements Need to define requirements and ensure that feature is useful label Apr 2, 2018
@stalniy
Copy link
Owner

stalniy commented Apr 3, 2018

So, do you think it makes sense to create a separate directive?

@shadoWalker89
Copy link
Author

  1. I don't know about the pseudo code that your provided in the first point, but i find most of the time that the permission definition contains a lot more then just user.isAdmin() or user.isEditor() but it checks also for something like:
  • Is the post assigned to the editor or not ?
  • Is the post published or not ? (May be that what you mean with isVisible in your code ?)
    Doing so, the vast majority of the time i find my self doing just permission checks within if conditions without any additional conditions. Which can come in handy if a v-can directive exists.
    But when the times comes and we need additional conditions along side the permission check, nothing stops us from using a v-if

2- This is passing props to a component so is has to be done throw the props it self, which why i don't think it falls within the scope of my issue. I'm talking about showing/hiding/rendering a component on the page.

3- Yep, to pass arguments we must use an array but it is not so bad isn't ?

In laravel we have blade directives @can @cannot Which is very helpful.
Nice package by the way, thank your for your effort and keep up the good work 😄

@stalniy
Copy link
Owner

stalniy commented Apr 3, 2018

Ok. I see your point.

I suggest this interface for the directive:

  • v-can.read=“post”
  • v-can=“[‘read’, post]”. array syntax for cases when action is a variable

@stalniy
Copy link
Owner

stalniy commented Apr 3, 2018

for per field check:

  • v-can.read.title.of=“post” (of is optional, just for readability)
  • v-can=“[‘read’, post, ‘title’]”

Also it will be shipped as separate plugin, this will allow to shake out it if unused.

Frankly speaking, I prefer $can syntax because it’s closer to Ability interface :) and doesn’t require additional abstraction

@shadoWalker89
Copy link
Author

Nice
Thanks

@stalniy
Copy link
Owner

stalniy commented Apr 3, 2018

ok, lets wait some time to see whether there are other people who wants such directive.

@affanshahid
Copy link

This would be an excellent addition

@stalniy
Copy link
Owner

stalniy commented Apr 17, 2018

Another thoughts on this topic:

Sometimes you may want to add css class based on user permissions:

<div :class=“{ my: $can(‘read’, ‘User’) }”>...

Or use v-show instead of v-if

<div v-show=“$can(‘read’, ‘User’)”>...

This kind of stuff works very nice with provided $can method but not with v-can

@stalniy stalniy added help wanted and removed status: define requirements Need to define requirements and ensure that feature is useful labels Apr 17, 2018
@stalniy
Copy link
Owner

stalniy commented Apr 17, 2018

@shadoWalker89, it'd be awesome if you could provide a PR for this directive with tests. I use jest for testing

@stalniy
Copy link
Owner

stalniy commented Apr 17, 2018

You can start from simple directive which accepts array and later add more readable version if you need it

@shadoWalker89
Copy link
Author

shadoWalker89 commented Apr 19, 2018

@stalniy i'm sorry but i'm already using another package.

@stalniy
Copy link
Owner

stalniy commented Apr 19, 2018

Could you please share why you decided to go with another solution? Just want to understand and make CASL to be better :)

@shadoWalker89
Copy link
Author

Well, the main reason is that i looked up packages with the vue directive.
And then i found a package that is easier when it comes to defining the permissions.
At the beginning, the way permissions are defined in your package confused me a bit.

Good luck with your package.
I want to thank you for the work that you are doing, because you are helping people with your time and effort by making this package.

@stalniy
Copy link
Owner

stalniy commented Apr 26, 2018

An interesting article https://codeburst.io/reusable-vue-directives-v-can-753bf54e563f , probably will take some design suggested there :)

@stalniy
Copy link
Owner

stalniy commented May 14, 2018

After some investigation I found that it's possible to implement directive only with hacks which in future versions of Vue may not be supported.

Taking a look at vue-browser-acl I see that the author changes vnode inside directive. However in the official documentation for directives you may find that all arguments passed into directive should be considered as read-only:

Apart from el, you should treat these arguments as read-only and never modify them. If you need to share information across hooks, it is recommended to do so through element’s dataset.

That means if I implement directive with the same hack as in vue-browser-acl I will need to abandon it if Vue 3.0 will ignore it. And everybody who used directive will need to rewrite it to v-if="$can or components.

So, the best what I can do here is to provide a custom component <can>. Directive won't be implemented, at least until Vue will allow to write custom conditional directives

@stalniy stalniy closed this as completed May 14, 2018
@stalniy
Copy link
Owner

stalniy commented May 14, 2018

keep track on progress in #63

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