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

Should v-model work on components using both v-bind="$attrs" and v-on="$listeners"? #6216

Closed
chrisvfritz opened this issue Jul 25, 2017 · 19 comments

Comments

@chrisvfritz
Copy link
Contributor

chrisvfritz commented Jul 25, 2017

What problem does this feature solve?

Following this discussion, given a CustomInput component with this template:

<div>
  <input v-bind="$attrs" v-on="$listeners">
<div>

The following currently does not work:

<CustomInput v-model="myValue" />

The problem is that v-model on a component expects a value on input events, but a DOM Event is passed to the listener instead.

What does the proposed API look like?

Option 1: Make v-model smarter

Since I think it's unlikely that anyone will want to use v-model with a DOM Event, perhaps when v-model is used on components, it could check if the argument passed to the listener is instanceof window.Event. Then if it is, use event.target.value instead.

Option 2: Make v-on smarter

Maybe a non-enumerable property could be added to $listeners (e.g. __$listeners__: true, to help v-on detect uses of v-on="$listeners". Then in cases where the $listeners object is passed to v-on and it's used on an element, listeners relevant to v-model could be wrapped:

function (event) {
  listener(event.target.value)
}

But now we're throwing away data. If someone wants to access a keyCode - e.g. with @input instead of v-model - they're out of luck. However, if modifiers were also supported for v-on's object syntax, we could fix this by making .native disable the automatic wrapping behavior.


Thoughts? Are there strategies or implications I'm not considering? I'm definitely open to alternatives.

@RobertBColton
Copy link

RobertBColton commented Jul 26, 2017

I would say for the first question, I don't care if it works automatically, but that it at least doesn't require another abstraction to accomplish. It should basically work close to how it would work if there was no component.

Literal Values

This is a huge part of the problem for me. I haven't been able to solve it because of a few cases where I want to use enum constants (I know these don't exist in JS, but they do in TypeScript, and I'm using a similar convention in a JS module) in option values.

$event.target.value is always a string, and thus, this solution would not work where the types of the value props should be literals/expressions:

<select>
	<option :value="someExpression">One</option>
	<option :value="someExpression">Two</option>
	<option :value="someExpression">Three</option>
</select>

My proposal

Maybe it would be possible to make v-model="$attrs" work similar to v-bind="$attrs" and be smart enough to not warn about direct prop mutation? What's really needed is some way of getting the prop attributes, why are we only allowed to get access to the non-prop ones?

@nickmessing
Copy link
Member

@RobertBColton in your example $event.target._value would work ;) just a hint.

@RobertBColton
Copy link

RobertBColton commented Jul 26, 2017

@nickmessing Thanks so much, that does seem to work, but I can't seem to make the following work without a vue warning about directly mutating the value prop.

<template>
    <select class="combo" v-bind:value="value" @input="update"> 
      <option v-for="(item, key) in items" :value="item">{{key}}</option>
    </select>
</template>

<script>
export default {
	props: {
		items: {
			type: Object,
			required: true
		},

		value: {}
	},

	methods: {
		update(event) {
			console.log(event.target._value);
			this.$emit('input', event.target._value);
		}
	}
}
</script>

Ideally I wouldn't want to and do not want to add another value to the data section or similar as it would seem redundant. Is there a way of achieving this simple component without getting too verbose?

@nickmessing
Copy link
Member

@RobertBColton, Github issues are not for asking questions, I am most of the time online in official chat (you can try stack overflow or forum too).

@victor-am
Copy link

victor-am commented Jul 30, 2017

My first impulse when 2.4 got out was to try using v-on="$listeners" and v-bind="$props" with v-model (and got a little sad when it didn't work 😢).

It would be an amazing feature imo 👍

As for the two options, I'm more inclined to agree with the first (seems simpler as far as I understood).

@chrisvfritz
Copy link
Contributor Author

chrisvfritz commented Jul 30, 2017

@victor-am You could experiment with implementing option 2 just for your component, like in this simple example.

@LinusBorg
Copy link
Member

I'm not sure this is desirable.

First off, $attrs and $listeners were added to make it easier to pass on these things from a parent component to a child component, to improve the situation for HOC-like components.

Making this work with v-model was not a goal in this original discussion.

Secondly, If we somehow make this work - that you can use v-model on a component with v-model on an element in that component, it feels to me like we essentially recreated inter-component two-way bindings which we originally dropped in Vue 2 for good reason.

Yes, under the hood it's technically still sending events and updating in the parent, but what happened under the hood never was the problem with .sync. It was that it made data flow harder to follow.

Do we re-create this weekness by allowing for a seamless v-model chain through many components down to some nested element in a form? It feels like we would to me.

@chrisvfritz
Copy link
Contributor Author

chrisvfritz commented Jul 30, 2017

@LinusBorg I think you might misunderstand the proposal. There's no state syncing between components, as v-model is only used on the parent component. Check out the example in the issue description, which you agreed should work in another issue.

This comment suggested what you'd like to avoid, with state being maintained in both parent and child, but his use case and proposal isn't related to this feature request.

@LinusBorg
Copy link
Member

Oh right, sorry. I misunderstood completely.

@frandiox
Copy link

frandiox commented Aug 6, 2017

v-model works differently for components and native elements: https://github.com/vuejs/vue/blob/dev/src/platforms/web/compiler/directives/model.js#L47

Therefore, I guess making v-model "smarter" is not trivial. The input event probably needs to be modified manually from the inner component to the outer component @input="$emit('input', $event.target.checked)" (or $event.target.value, etc.).

In my app, I'm wrapping a third party component and just adding custom style. I thought it should work with v-on="$listeners" v-bind="$attrs", but I noticed that adding :value="value" is also required.

I think it is a bit weird that a component with a declared v-model has input in $listeners but doesn't have value in $attrs. Thinking about it twice, I guess this is because value is automatically added to the accepted props due to v-model, and thus it is not included in $attrs (non recognized props) anymore.

However, this.value does not exist unless props: ['value'] is specifically declared. Perhaps value could be added to $attrs if it is not manually specified as a prop?

@chrisvfritz
Copy link
Contributor Author

I think it is a bit weird that a component with a declared v-model has input in $listeners but doesn't have value in $attrs.

@frandiox I might have misunderstood what you mean, but I'm seeing value in $attrs.

@LinusBorg
Copy link
Member

Well, if the child properly declared the prop, I would expect to see it in $props, not $attrs, and that's what's happening.

@chrisvfritz
Copy link
Contributor Author

@LinusBorg I think @frandiox is suggesting he's seen value automatically registered as a prop on a child component due to the use of v-model on a parent. I haven't noticed that behavior personally.

@frandiox
Copy link

frandiox commented Aug 7, 2017

@LinusBorg @chrisvfritz You are passing value attribute manually so it is normal to have it in $attrs. However, I think normally you only pass v-model="foo" and it is internally expanded to :value="foo" @input="foo = $event", so there is no need to manually pass value. If you omit it, value is not included in $attrs and is not included in $props unless props: ['value'] is specified in the component (it is used internally in $vnode.data.props, though).

If it is declared as a prop, we all agree it should be included in $props (and it is). However, if it's not declared, shouldn't it be included in $attrs? -- Considering that v-model is also adding input to $listeners

@chrisvfritz
Copy link
Contributor Author

chrisvfritz commented Aug 8, 2017

@frandiox Ah, thanks for the example. I agree it's strange that value doesn't appear in $attrs there. That should be easily fixable by having v-model not assume that value is registered as a prop, adding to $vnode.data.attrs instead. That works in both cases.

@chrisvfritz
Copy link
Contributor Author

I made some time this afternoon to fix this in #6327. 🎉 Reviews welcome. 🙂

@adi518
Copy link

adi518 commented Aug 27, 2017

Definitely something I want/need. Looks like the increasing popularity of HOC has made this a necessity.

@yyx990803
Copy link
Member

So, I finally gave this proposal a proper investigation, and it ends up being more complicated than it seems. The problem lies within consistency between different input types. The current implementation in #6327 assumes a text input and ignores non-input events. While this simplifies wrapping plain text input fields, it makes the whole thing inconsistent and feels like a special-case convenience hack.

I managed to make it work with <input type="checkbox">, which requires the component to specify the model option, and then injecting the event type and prop type back into the component v-model handler. It's already messier than I hoped - but even if this works, it becomes prohibitively complex to mimic the full behavior of native v-model (e.g. checkboxes with array binding, radio, select...)

At this point I feel the implementation cost and the inconsistency outweighs the simplification this change would bring.

Taking a step back, the whole idea of component v-model is opening up finer-grained control over the two-way binding of form inputs, not simplifying it. Its job is expanding the sugar consistently into a pair of input and output, and letting you fill in the details. Mixing that up with $attrs and $listeners feels like magic where it doesn't belong.

@Hamido-san
Copy link

All we want is to be able to create "fully transparent wrapper" components and add some functionality/customization on top... to have some semblance of inheritance in Vue.
Why all these edge cases and special syntax just for that? Why is it not the default behavior already?

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

9 participants