vuejs / rfcs

RFCs for substantial changes / feature additions to Vue core
4.87k stars 546 forks source link

Should v-model-checkbox mutate its array? #178

Open jods4 opened 4 years ago

jods4 commented 4 years ago

If the v-model of a checkbox is an array, currently Vue creates a new copy of that array for each change and assigns it (if possible...).

This comes with some drawbacks that make me wonder whether mutating the array would be a better idea.

You can play with and try to fix this fiddle to get a feel for the situations where it's not ideal. https://jsfiddle.net/3g89mswr/7/

The fact that you need to be able to assign is restrictive in some situations. In the fiddle I try to use a prop from a stateless (could be functional) component but props are not assignable.

Not only must the target be assignable but it must also be reactive. Even if you don't intend on reacting to the changes (in the example above, I only read back the array when clicking on a button) a non-reactive property holding this array is buggy because of the way v-model-checkbox work. If the property is not reactive any change will always be applied to the initial array (and previous changes are lost).

One argument could be made that similar approach would also fail for a string prop with v-model on a textbox. I would agree with that, but I'm not sure if the comparison is really obvious. I intuitively thought the array would mutate and I was surprised when it didn't. Also the text input version would work even with a non-reactive source property.

Using mutations would IMHO make Vue work in more situations and reduce friction.

aztalbot commented 4 years ago

This seems related to https://github.com/vuejs/rfcs/pull/140 and https://github.com/vuejs/rfcs/pull/175 (which I see you've mentioned already in https://github.com/vuejs/rfcs/issues/156). I might think about this differently, as I wouldn't intuitively think an array should mutate like that. Generally, I don't expect to pass an object down to have it mutated, I'd expect the parent to be notified about the update. So to make the fiddle work, I would add a computed in Child { get: () => props.selected, set: val => emit('update:selected', val) } and pass that to the checkbox v-models, and then have App do v-model:selected, which makes it explicit that list will be updated. The rfcs I mentioned would provide mechanisms to make that cleaner, which would alleviate some of the friction you mention. One weird thing about mutation is that mutating in v-model would mean you can probably pass a readonly computed list to v-model and it would modify it and mostly work even though it shouldn't, and that would seem odd to me.

The above wouldn't address the fact that the data would need to be reactive. I do agree on your reactivity point. It does seem like you need to add reactivity in a use case where you maybe don't need it. But for most people in most use cases, I'd expect they'd just make it reactive to ensure the list stays in sync and continues to work properly if they end up using that list in more than one way (i.e. in a way that needs it to be reactive). Not sure what the solution is there because I would find v-model mutation confusing (something like v-model.inplace would make it more explicit but I'm still not sure there would be appetite for it).

jods4 commented 4 years ago

About those other issues: yes and no. Yes, because they try to use a v-model inside a component with data from a parent, like I do here. No, because those other issues want to make props "mutable" (kind-of), which I don't think is a good idea. Also no because I'm looking specifically at the inner workings of v-model-checkbox, not v-model in general.

I would add a computed in Child

Yes. This (or a variation) is certainly the easiest way to make it work currently. Note that it rules out a functional component, which is not dramatic but a bit unfortunate.

you can probably pass a readonly computed list to v-model

Passing a readonly thing to a v-model feels weird to me, but I see how you could make it work.

I don't think there's a right/wrong design in the absolute here and there's previous art for mutating an array bound to list of checkboxes, e.g. Knockout. To be honest I wouldn't really care how it works if I didn't have that weird issue at the moment :/

The only nice thing about mutating the array is that you can then bind a list of checkboxes to a non-assignable array and still get the correct results out of it.

CyberAP commented 4 years ago

That'll be a breaking change for computed models with marginal gains in very specific scenarios, so for me it doesn't seem to be worth going the breaking path. Instead, you could create a computed model that will mutate array if that's desired.

const model = {
  get() { return this.modelValue },
  set(value) {
    mutateArray(this.modelValue, value)
  }
}