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

Proposal for dynamic directive arguments #4

Merged
merged 5 commits into from
Jan 26, 2019

Conversation

yyx990803
Copy link
Member

@yyx990803 yyx990803 commented Jan 16, 2019

<div v-bind:[key]="value"></div>
<div v-on:[event]="handler"></div>

Full rendered proposal

@henriqemalheiros
Copy link
Contributor

henriqemalheiros commented Jan 16, 2019

How this proposal affects custom directives? Would binding.oldArg be necessary?

@yyx990803
Copy link
Member Author

@henriqemalheiros custom directive would receive the evaluated argument I think - which requires no change in implementation. Is there a valid use case where the directive would want access to the raw argument expression?

@henriqemalheiros
Copy link
Contributor

@yyx990803 not the raw argument expression. I meant, if the argument is updated, how will the custom directive know about it?

Take for instance this overly simple directive that shows an alert:

Vue.directive('alert', {
  bind(el, { arg, value }) {
    if (![ 'click', 'mouseover' ].includes(arg)) {
      throw new Error('incorrect directive argument')
    }

    el.__alertValue = value

    el.__alertCallback = () => {
      window.alert(el.__alertValue)
    };

    el.addEventListener(arg, el.__alertCallback)
  },
  update(el, { value, oldValue }) {
    if (value !== oldValue) {
      el.__alertValue = value
    }
  }
})

If the user changes the argument from click to mouseover, the directive should remove the old event listener and add a new one, hence my question about the binding.oldArg.

@znck
Copy link
Member

znck commented Jan 17, 2019

Is there a valid use case where the directive would want access to the raw argument expression?

I think above is a use case to access raw argument expression, that way a directive can choose to throw an error if a dynamic argument is used. Or dynamic: boolean should be added to disable dynamic arguments.

@posva
Copy link
Member

posva commented Jan 17, 2019

Wouldn't dynamic ones unbind and bind again?

@yyx990803
Copy link
Member Author

@henriqemalheiros I see what you mean now, yes, dynamic argument does introduce an extra change point to watch for in custom directives. I'll update the RFC to reflect that.

@laander
Copy link

laander commented Jan 17, 2019

In general, I like it. It's quite concise and applies to different directives with the same syntax.

One thing worth mentioning is that people coming from Angular might be thrown off by this, as <div [foo]="value"> ... </div> in Angular is equivalent to <div :foo="value"> in Vue.

One alternative could be using { } instead, like so:

<div v-bind:{key}="value"></div>
<div v-on:{event}="handler"></div>

The biggest benefit is that single { foo } in JSX denotes javascript expressions i.e. dynamic evaluation. This also aligns nicely with the double {{ foo }} that you usually see in most other templating languages for expressions, including Vue.

With that said, do we know how to achieve dynamic attribute names in other framework that could be used as a mental reference? I can't seem to find anything for neither Angular, Aurelia nor Ember.

@chrisvfritz
Copy link
Contributor

I like the proposal. 🙂 I don't have any strong opinions on what kinds of expressions should be allowed, but it might be useful to only officially support simple property expressions (like what v-model's value allows). That would simplify documentation and also encourage simpler templates.

@laander I agree it's good to consider people coming from other frameworks, but I think it's better to be consistent with JavaScript in general, where [xxx] is used for dynamic keys in an object (e.g. { [dynamicKey]: value }).

@Justineo
Copy link
Member

Justineo commented Jan 17, 2019

One potential downside is that this feature may lead to the assumption that any JavaScript expression can be used inside the []. People may try to write v-slot:[item + index] which will break the whole attribute. On the other hand, the old :slot="item + index" solved this nicely. But anyway this usage may be not so significant to be considered.

@laander
Copy link

laander commented Jan 18, 2019

@chrisvfritz That's the winning argument for [ ], agreed 👍

@Justineo Good point, but perhaps that's a saviour in disguise: inline interpolation of slot name should as a minimum be discouraged (use computed properties instead) and even better if the compiler won't accept it as valid. IMO opinionated decisions on fringe use cases like this are where Vue shines.

Which does not work as expected. A workaround would be:

``` html
<div :[`key${foo}`]="value"></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<div :[`key${foo}`]="value"></div>
<div :[`${key}foo`]="value"></div>

Scott

@smolinari
Copy link
Contributor

Just so I am certain I do understand, is this valid?

<!-- v-slot with dynamic name -->
<foo>
  <template v-slot:[object.propName]>
    Hello
  </template>
</foo>

Scott

@jacekkarczmarczyk
Copy link

jacekkarczmarczyk commented Jan 19, 2019

What would be the behaviour of

<div v-on:[event]="handler"></div>

with null/undefined value of event? Maybe it should ignore the handler to make conditional binding of events simpler than it's today:

<template>
  <div v-on:[conditionalClick?'click':null]="handler"></div>
  <div v-on:[conditionalClick&&'click']="handler"></div><!-- if we allow false -->
</template>

or

<template>
  <div v-on:[conditionalClick]="hander"></div>
</template>

<script>
export default {
  computed: {
    conditionalClick () { 
      return isMonday() ? 'click' : null
    }
  }
}
</script>

Of course that could apply also to v-bind directive

@yyx990803
Copy link
Member Author

@smolinari yes, that's valid. As long as the expression does not contain characters that causes the HTML parser to end parsing of an attribute name - i.e. space and quotes.

@yyx990803
Copy link
Member Author

@jacekkarczmarczyk that's an interesting point. I'm inclined to allow falsy values to indicate that the binding should be ignored/removed. The concern would be it silently lead to undesired behavior when the falsy value is passed mistakenly.

@yyx990803
Copy link
Member Author

@jacekkarczmarczyk on a second thought, unfortunately if we generate the code like the following:

on: {
  [null]: handler
}

null would have already been converted to the string "null" key in the resulting object, so Vue would not be able to tell whether the user intentionally provided a null value or a "null" string.

Generating extra code to check the dynamic value type will resulting in less efficient code, but maybe it's worth it since dynamic argument is probably not a very commonly used feature.

@yyx990803 yyx990803 added the core label Jan 21, 2019
@yyx990803
Copy link
Member Author

yyx990803 commented Jan 21, 2019

We pretty much have 3 options regarding falsy/non-string values:

  1. Silently convert to strings (vote with 🚀)
  2. Throw warnings for anything not strings (vote with 🎉)
  3. Treat null as a special value (remove binding), and warn the rest. (vote with ❤️)

@jacekkarczmarczyk
Copy link

@yyx990803 Re: 3 - Wouldn't undefined be more reasonable, or did you mean null/undefined?

@yyx990803
Copy link
Member Author

@jacekkarczmarczyk undefined is much more likely to be a mistake.

@Justineo
Copy link
Member

As mentioned in #4 (comment), I don't think we should allow arbitrary JavaScript expressions inside []. Since we can't have space inside attributes, this may lead to very error-prone code IMO (otherwise we are going to write whitespace-less JavaScript expressions which also feels strange).

I agree with @chrisvfritz that maybe we should only allow what v-model's value allows in the [] expression.

@yyx990803
Copy link
Member Author

@Justineo in the updated proposal I mentioned we can actually detect unintentional spaces/quotes in directive arguments at the parser level, so if the user accidentally cause the dynamic argument attribute to terminate early, they will get appropriate warning. Do you still think it's necessary to limit the expression in that case?

Btw the reason v-model has restrictions in its expression is because it is used in an assignment's left position (i.e. expression = xxx). Dynamic arguments don't have a similar constraint.

@Justineo
Copy link
Member

I just tend to put DSL syntax into the attribute name while leaving JavaScript expressions with full expressive power to the attribute value. In that way we won't have to write space-less JavaScript code and It's also easier for tooling.

Is there any chance that we move the dynamic part into the attribute values? I've been thinking about this but haven't come up with something neat enough yet.

@yyx990803
Copy link
Member Author

@Justineo I am in general against adding artificial restrictions when we don't have technical reasons to do so. Watch expressions are limited because we don't want to implement a full expression parser - that's a tradeoff due to technical constraint. Dynamic arguments are just placed into generated code and handled by the JS engine. The only technical restriction here is attribute name characters.

@yyx990803
Copy link
Member Author

Updated: added section regarding handling of null as a special value.

This RFC is now in final comment period.

@yyx990803 yyx990803 added the final comments This RFC is in final comments period label Jan 23, 2019
@jacekkarczmarczyk
Copy link

jacekkarczmarczyk commented Jan 23, 2019

@yyx990803 "Unresolved questions" section could be removed i guess

@smolinari
Copy link
Contributor

@yyx990803 - for my understanding once more, will this be possible too?

<foo>
  <template :slot:[name]>
    <-- stuff to fill the dynamically named slot with... -->
  </template>
</foo>

or maybe <template :slot[name]> (notice missing semicolon)?

Scott

@Justineo
Copy link
Member

@smolinari It will be <template v-slot:[name]>...</template> as slot syntax will be updated as well.

@smolinari
Copy link
Contributor

@Justineo - Ok. But, :slot is possible now in oder to make the name dynamic. I'm just asking if that can/ will stay with the new proposal?

Scott

@Justineo
Copy link
Member

As I understand, this proposal is exactly originated from the problem that dynamic slot names doesn't fit into our current directive syntax after we merged slot and slot-scope into v-slot (ref).

@smolinari
Copy link
Contributor

smolinari commented Jan 23, 2019

True, but that has nothing to do with leaving :slot a possibility with this change. So, this:

<template :slot:namedSlot="someProps">

reads

Bind as namedSlot with someProps.

And....

<template :slot[slotName]="someProps">

reads

Bind as dynamic slotName with someProps.

I'll leave it at that. I don't want to muddy the discussion further. Just a yea or nay from Evan will be fine, maybe with a short explanation why it's a bad idea or not possible or maybe it is possible? 😁

Scott

@yyx990803
Copy link
Member Author

@smolinari the main problem is consistency: :slot is technically already the shorthand for v-bind:slot, so slot is the argument for v-bind. It doesn't seem to make sense to add an argument after the argument.

@smolinari
Copy link
Contributor

@yyx990803 - Thanks. Makes sense, Is there somewhere I can DM you an idea along these lines?

Scott

@yyx990803
Copy link
Member Author

@smolinari why not make an RFC? The process is designed to help you think through rough ideas.

@smolinari
Copy link
Contributor

smolinari commented Jan 24, 2019

Nevermind. I did what you said Evan and was writing down my thoughts in an issue here and then realized I couldn't come up with a decent example to make the suggestion worth while. 😁

Scott

@yyx990803 yyx990803 merged commit 6db84d6 into master Jan 26, 2019
@yyx990803 yyx990803 added 2.6 Landed in 2.6 and removed final comments This RFC is in final comments period labels Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.6 Landed in 2.6 core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants