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

Improve terminalable directive #2272

Merged
merged 1 commit into from
Mar 21, 2016

Conversation

kazupon
Copy link
Member

@kazupon kazupon commented Jan 31, 2016

There are times when we want to use the arg and modifires in custom terminal directive, and control the priority.

e.g.

<input type="text" v-validate:feild1.input-off="['required']">

Recently, in the following related issues or PR, we became able to define the terminal directive.

However, we can not yet.

I fixed this issues.
I hope that this PR are merged with you.

Please check the codes.

@yyx990803
Copy link
Member

Thanks for the PR! This one takes some time to review, but I'll do it as soon as I can 👍

@kazupon
Copy link
Member Author

kazupon commented Feb 3, 2016

Thanks!! 😺

linker(vm, el)
expect(vm._bindDir.calls.count()).toBe(1)
var args = vm._bindDir.calls.argsFor(0)
expect(args[0].name).toBe('term')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps assign the args[0] to a variable.

@kazupon
Copy link
Member Author

kazupon commented Mar 12, 2016

ping @yyx990803

if (value != null) {
return makeTerminalNodeLinkFn(el, dirName, value, options)

var terminals = terminalDirectives.map(function (terminal) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These change seems pretty expensive since it's now map + sort + nested for loop for every single element compiled. Ideally we want to move as much work out of this function as possible.

@yyx990803
Copy link
Member

Here's what I think what should be the ideal API: the custom directive should just declare terminal: true instead of monkey patching the internal terminalDirectives array. We should also change v-if and v-for to use the same terminal: true contract.

With this, the checkTerminalDirectives function would largely be the same with compileDirectives - pseudo code:

let A = "current terminal directive found with highest priority"
- for each attribute on current element:
  - if it's a directive, and has `terminal: true`:
    - if A doesn't exist, or it has higher priority than A:
      - assign it to A
- If A exists, return a terminalLinkFn for it.

Does this make sense? I may have overlooked something but I think this could make it more efficient and more consistent.

@simplesmiler
Copy link
Member

custom directive should just declare terminal: true

Do want. This will allow to make things like vue-transfer-dom truly reusable (i.e. without Vue.use(...)).

@blake-newman
Copy link
Member

I totally agree with the approach not to monkey patch parts of the core API. This clearly has a use case that makes sense to easily implement, with future extentions this will become ever more requested.

@kazupon
Copy link
Member Author

kazupon commented Mar 17, 2016

@yyx990803
Thank you for your code review , new API proposal for terminal directive, and pseudo code.

When I'm implementing the patch of this PR, I felt that terminalDirectives array is monkey patch. I think that your API proposal make sense.

I'll try to challenge based on your feedback. 😸

@kazupon kazupon force-pushed the improve/terminal-directive branch 2 times, most recently from 3171645 to d7316c0 Compare March 19, 2016 14:15
@kazupon
Copy link
Member Author

kazupon commented Mar 19, 2016

@yyx990803 updated

}
}

if (termDef) {
return makeTerminalNodeLinkFn(el, dirName, value, options, termDef, name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can already extract arg as matched[2] here, and can just use that and pass it to makeTerminalNodeLinkFn instead of using argRE.

Support the followings:
- Directive instance properties: `arg`, `modifiers`
- Priority for terminal directive

BREAKING CHANGES:
- API: Change internal `terminalDirectives` array to `terminal: true` directive options
@kazupon kazupon force-pushed the improve/terminal-directive branch from d7316c0 to 9236191 Compare March 21, 2016 16:30
@kazupon
Copy link
Member Author

kazupon commented Mar 21, 2016

@yyx990803
Updated Done.

This PR is breaking change of custom terminal directives.
We need to write the document terminal directive feature.

@yyx990803
Copy link
Member

@kazupon yes. Custom terminal directives were not publicly documented though, so technically this is a new feature. One case I can think of is https://github.com/rhyzx/vue-transfer-dom may need to be updated to use the new API. /cc @rhyzx

yyx990803 added a commit that referenced this pull request Mar 21, 2016
@yyx990803 yyx990803 merged commit c748bb3 into vuejs:dev Mar 21, 2016
@yyx990803
Copy link
Member

Merged! 🎉
I think this is the first PR that makes non-trivial updates to the compiler mechanism, great job @kazupon !

@kazupon
Copy link
Member Author

kazupon commented Mar 21, 2016

@yyx990803
Thanks !!

As next step, I'll try to write the documentation.

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

Successfully merging this pull request may close these issues.

4 participants