Feature: add vue component/master #71

jackiecookie · 2018-05-29T08:00:20Z

first, I want to thanks the casl team for providing the awesome package.
I'm now using casl to my Vue project for permissions. and I've implemented a Can component with Vue provide/inject which #64 mentioned.if you guys have good advice. please feel free to tell me.

stalniy · 2018-05-29T09:11:35Z

First of all, thank you for all that work! it's really hard to review because there are a lot of files :)

P.S.: remove yarn.lock please

stalniy · 2018-05-29T08:49:28Z

packages/casl-vue/spec/can.spec.js

+import { abilitiesPlugin } from '../src';
+
+chai.use(spies);
+const should = chai.should();


use expect because all packages in repo uses expect not should

stalniy · 2018-05-29T08:50:47Z

packages/casl-vue/package.json

@@ -46,6 +46,8 @@
    "@casl/ability": "^2.0.0",
    "@vue/test-utils": "^1.0.0-beta.12",
    "vue": "^2.5.13",
-    "vue-template-compiler": "^2.5.13"
+    "vue-template-compiler": "^2.5.13",
+    "chai": "^4.1.0",


this should be removed. chai and chai-spies are installed on the top level. So, you can run tests with 2 commands:

npm test -- --scope @casl/vue

or from package root: PATH=$PATH:../../node_modules/.bin npm test

stalniy · 2018-05-29T08:53:15Z

packages/casl-vue/spec/plugin.spec.js

@@ -1,86 +1,91 @@
-import { createLocalVue } from '@vue/test-utils'


remove all semicolons from tests. I prefer to keep tests clean and I'm not using semicolons in tests )) Probably need to add eslint rules for this

stalniy · 2018-05-29T08:58:19Z

packages/casl-vue/src/plugin.js

    methods: {
      $can(...args) {
        watcher.rules = watcher.rules; // create dependency
        return this.$ability.can(...args);
+      },
+      $defineAbility(fn) {
+        const watcherForComponent = new Vue({


you don't need a separate watcher. Also I'd like to not add this function into Vue components. People can use regular AbilityBuilder.define instead.

You can use parent.$can() from component, instead of adding additional watcher

return parent.$can(action, subject, field) ? children : null

stalniy · 2018-05-29T08:58:30Z

packages/casl-vue/src/plugin.js

    methods: {
      $can(...args) {
        watcher.rules = watcher.rules; // create dependency
        return this.$ability.can(...args);
+      },
+      $defineAbility(fn) {


please remove this function

stalniy · 2018-05-29T09:01:36Z

packages/casl-vue/src/plugin.js

      }
    }
  });
+  Vue.component('Can', Can);


this should be removed and I'd like people (i.e., clients of CASL) to do it manually in case if they want to use Can component. In case if not, component can be shaked out from the build by webpack or rollup.

So, @casl/vue should just export Can component and later in the app if you want to use it you will do this:

import { Can, abilitiesPlugin } from '@casl/vue' import Vue from 'vue' Vue.use(abilitiesPlugin) Vue.component('Can', Can) new Vue(...)

stalniy · 2018-05-29T09:02:56Z

packages/casl-vue/src/component/can.js

+    props, children, parent, injections
+  }) {
+    validateProps(props);
+    if (injections.ability) {


remove this. I'd prefer to implement inject/provide in a separate PR after dicussion

stalniy · 2018-05-29T09:03:22Z

packages/casl-vue/src/component/can.js

+
+export default {
+  name: 'Can',
+  inject: ['ability'],


remove this, explanations below

stalniy · 2018-05-29T09:04:06Z

packages/casl-vue/src/component/can.js

+  props: {
+    I: String,
+    do: String,
+    a: [String, Object],


can be only string or Function (i.e., class)

stalniy · 2018-05-29T09:10:25Z

packages/casl-vue/src/component/can.js

+    on: [String, Object]
+  },
+  render(createElement, {
+    props, children, parent, injections


I wanted to use slots. What is the difference between slots and children?

stalniy · 2018-05-29T09:13:13Z

there is #63 task with subtasks for this. You can look for details there

codecov-io · 2018-05-30T00:41:24Z

Codecov Report

Merging #71 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #71      +/-   ##
==========================================
+ Coverage   99.62%   99.63%   +<.01%     
==========================================
  Files          15       16       +1     
  Lines         267      274       +7     
==========================================
+ Hits          266      273       +7     
  Misses          1        1

Impacted Files	Coverage Δ
packages/casl-vue/src/component/can.js	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b12eb80...40d2e19. Read the comment docs.

jackiecookie · 2018-05-30T01:10:09Z

the slot between children on functional component reference: https://vuejs.org/v2/guide/render-function.html#slots-vs-children on this Can component children equals slot.default.

AFAIK the ability.update will reassign rules which hard maintain if I have a lot of authority to manage.so, in this case, every Can component can use the separate ability provide by its own vm instance or parent instance maybe is better(like casl-react abilityProvider).

stalniy · 2018-05-30T04:34:36Z

Awesome! Thanks for explanation.

Frankly speaking, I don’t think that it make sense to have more than 1 instance of Ability in application.

However your point is valid. In some edge cases it may be required (I don’t know such). But for simplicity I’d like to proceed with smaller steps (I wrote article about CR).

Let’s add support for provide/inject in a separate PR.

stalniy

Now it's better. I will change tests a bit and merge it. Thanks!

stalniy · 2018-05-30T04:18:08Z

packages/casl-react/spec/Can.spec.js

@@ -149,4 +149,4 @@ describe('`Can` component', () => {
  function validateProps(Component, props, propName) {
    assertPropTypes(Component.propTypes, props, 'prop', Component.name)
  }
-})
+})


Revert this change please as it doesn’t relates to the purpose of this commit :)

stalniy · 2018-05-30T08:42:35Z

created #72 for provide/inject functionality

jackiecookie · 2018-05-30T14:04:34Z

just have one instance of ability also has another issue at this time.let's say, I have one App have 3 components. like: https://jsfiddle.net/saL0ocLj/ , when getAnotherAbility called, the Post component cause rerendered, its performance waste. if Post component and Blog component can provide Ability by own components,like: https://jsfiddle.net/bfdvr5k8/. hope that's can make sense. Thanks!

stalniy · 2018-05-30T15:36:55Z

Ok. I’ll take a look at that tomorrow. Thanks!

stalniy · 2018-05-30T16:03:10Z

I see your point. You want to not rerender component if ability.can didn’t change its value. To do this can needs to be converted to stateful component. Functional ones are rerendered on each tick

The main idea of CASL is to keep all permissions in one place. So I don’t think it make sense to create separate abilities (I.e. instances of Ability)

Another point is that user permissions are not changed very often. Thus performance shouldn’t be a big issue here

jackiecookie · 2018-05-31T00:45:54Z

Thanks for the explanation, It's helpful!

stalniy · 2018-05-31T04:51:27Z

An interesting reading about functional components: vuejs/vue#4037 (comment)

https://blog.webf.zone/vue-js-considerations-and-tricks-fa7e0e4bb7bb

stalniy and others added 12 commits May 29, 2018 14:24

test(angular): fix test of spying can pipe

3af6410

test(casl-vue):add test for abilityProvider

04e9103

test(casl-vue):add test.

fe02b14

fix(casl-vue):add default provide to fix warn.

b7c967a

test(casl-vue):add test.

4e8b0b9

fix(casl-vue):fix bugs.

3fccf50

fix(casl-vue):fix bugs.

05bd256

test(casl-vue):add test to vaildate props.

119acbc

chore(casl-vue):rm .vscode

9e5b04f

chore(casl-vue):rm abilityProvide

86a4a6c

fix(casl-vue):fix conflict

8962181

fix(casl-vue):fix pageage-lock conflict

98e1c11

stalniy requested changes May 29, 2018

View reviewed changes

jackiecookie force-pushed the feature-add-vue-component/master branch from 1c2a1ad to 38b6a82 Compare May 30, 2018 00:41

Merge branch 'master' into feature-add-vue-component/master

07f17a6

jackiecookie force-pushed the feature-add-vue-component/master branch from 38b6a82 to 07f17a6 Compare May 30, 2018 00:44

stalniy added 3 commits May 30, 2018 08:24

fix

f3cbc37

fix tests

5514f81

chore(eslint): allows to use XOR

a52bf75

stalniy approved these changes May 30, 2018

View reviewed changes

chore(package): updates lock file

40d2e19

stalniy merged commit 42ee540 into stalniy:master May 30, 2018

jackiecookie deleted the feature-add-vue-component/master branch May 31, 2018 00:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: add vue component/master #71

Feature: add vue component/master #71

jackiecookie commented May 29, 2018

stalniy commented May 29, 2018

stalniy May 29, 2018

stalniy May 29, 2018

stalniy May 29, 2018

stalniy May 29, 2018

stalniy May 29, 2018

stalniy May 29, 2018

stalniy May 29, 2018

stalniy May 29, 2018

stalniy May 29, 2018

stalniy May 29, 2018

stalniy May 29, 2018

stalniy commented May 29, 2018

codecov-io commented May 30, 2018 •

edited

Loading

jackiecookie commented May 30, 2018 •

edited

Loading

stalniy commented May 30, 2018

stalniy left a comment

stalniy May 30, 2018

stalniy commented May 30, 2018

jackiecookie commented May 30, 2018 •

edited

Loading

stalniy commented May 30, 2018

stalniy commented May 30, 2018

jackiecookie commented May 31, 2018

stalniy commented May 31, 2018

		@@ -1,86 +1,91 @@
		import { createLocalVue } from '@vue/test-utils'

Feature: add vue component/master #71

Feature: add vue component/master #71

Conversation

jackiecookie commented May 29, 2018

stalniy commented May 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stalniy commented May 29, 2018

codecov-io commented May 30, 2018 • edited Loading

Codecov Report

jackiecookie commented May 30, 2018 • edited Loading

stalniy commented May 30, 2018

stalniy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stalniy commented May 30, 2018

jackiecookie commented May 30, 2018 • edited Loading

stalniy commented May 30, 2018

stalniy commented May 30, 2018

jackiecookie commented May 31, 2018

stalniy commented May 31, 2018

codecov-io commented May 30, 2018 •

edited

Loading

jackiecookie commented May 30, 2018 •

edited

Loading

jackiecookie commented May 30, 2018 •

edited

Loading