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-checkbox mutate its array? #178

Open
jods4 opened this issue Jun 10, 2020 · 3 comments
Open

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

jods4 opened this issue Jun 10, 2020 · 3 comments

Comments

@jods4
Copy link

jods4 commented Jun 10, 2020

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
Copy link

aztalbot commented Jun 10, 2020

This seems related to #140 and #175 (which I see you've mentioned already in #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
Copy link
Author

jods4 commented Jun 10, 2020

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
Copy link
Contributor

CyberAP commented Sep 9, 2020

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)
  }
}

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

No branches or pull requests

3 participants