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

Warn when v-model is bound on non-existent key #5932

Closed
kay999 opened this issue Jun 20, 2017 · 26 comments
Closed

Warn when v-model is bound on non-existent key #5932

kay999 opened this issue Jun 20, 2017 · 26 comments

Comments

@kay999
Copy link

kay999 commented Jun 20, 2017

What problem does this feature solve?

Considering this binding:

<input type="text" v-model="data.val1">

This works fine if there is for example { data:{ val1:null }} defined in component-data, but if only { data:{} } is defined it doesn't work correctly because while val1 is created by v-model, it's not made reactive.

By using Vue.set for setting values, this wouldn't be a problem which would make live easiert for certain usecases.

What does the proposed API look like?

No visible change.

But to maintain the actual behaviour this could also be implemented using a modifier like 'set' for example:

<input type="text" v-model.set="data.val1">

This would use Vue.set(data, 'val1', ...) instead of simply generating data.val1 = ...

@LinusBorg
Copy link
Member

This is intentional. We think it is better to explicitly define your data strcuture, because then the code of your component is more self-explanatory.

Data mutation should preferably happen in the component's code, not the template, and having v-model create reactive properties dynamially would work against that.

Vue 1 actually did what you proposed and we removed it in 2.0 for the above reason.

@kay999
Copy link
Author

kay999 commented Jun 20, 2017

While I understand the intention, it also creates lots of boilerplate-code in forms which are used to create data. And it's easy to run into stange behaviour if the backend sends "undefined" data which is then removed by JSON.stringify and leads to the loss of reactivity.

Also for components which create "array-forms" (forms with add/delete-buttons to create multiple rows of data) it always require to providing some data-initialization-code which is often unnecessary if "undefined" should be the standard value.

So maybe providung at least an option like using the "set"-modifier would give the developer the choice how to use it.

@kay999
Copy link
Author

kay999 commented Jun 20, 2017

It's also a bit inconsistent: If "reactivify by creation" is wanted, providing Vue.set is "wrong" too. But if it exists, why not use it for v-model, too?

And if Vue really wants to enforce this, silently losing reactivity isn't a good thing IMO. In this case v-model should create code which checks for the existence of a key first and raises an error if the key doesn't exits.

@kay999 kay999 changed the title Use Vue.set for v-model data-bindung instead of a simple js assignment Use Vue.set for v-model data-binding instead of a simple js assignment Jun 20, 2017
@LinusBorg
Copy link
Member

LinusBorg commented Jun 20, 2017

It's also a bit inconsistent: If "reactivify by creation" is wanted, providing Vue.set is "wrong" too.

There are situations where it's necessary - and if it's necessary and you use Vue.set, then that behaviour is plainly visible in your code instead of being abstracted away in v-model and hidden in your template.

It's a utility function, and providing it as such is different from integrating this under the hood into one of the most-used features of the framework.

The former enables users to create reactive properties after the fact when they really have to, while the latter accepts this behaviour as the norm and hides it away.

We want to support patterns that lead to readable and maintainable code, even if that means a bit of boilerplate in some situations. Adding your proposed behaviour to v-model would not contribute to that goal, in my opinion.

@kay999
Copy link
Author

kay999 commented Jun 20, 2017

Then there should at least a check in v-model for non-existing keys, because in the moment this often leads to invisible loss of reactivity if at some point a field isn't created.

This can happen quite easily by setting a value to 'undefined'. Just try to evaluate JSON.stringify({ v1:null, v2:undefined }) to see the problem if you use Vue to edit data provided by some REST service which uses JSON.stringify.

@kay999
Copy link
Author

kay999 commented Jun 20, 2017

Also the documentation should be updated. Instead of

"Due to the limitations of modern JavaScript (and the abandonment of Object.observe), Vue cannot detect property addition or deletion. Since Vue performs the getter/setter conversion process during instance initialization, a property must be present in the data object in order for Vue to convert it and make it reactive."

there should be a paragraph explaining that it's not because of a "limitations of modern JavaScript" but in fact intended behaviour.

@LinusBorg
Copy link
Member

LinusBorg commented Jun 20, 2017

I can agree that a warning here would be helpful and we should look into how to do that or something equally helpful.

However concerning your example: if Your REST API leaves out properties like you describe, and elaves out props that it shouldn't,

  • That's a bad API design in my book
  • Your client's API lib should take care that the data is normalized.

It should not be the job of the UI (v-model) to add back properties/data that is missing from the data-model. That has to be taken care of in a central place.

@kay999
Copy link
Author

kay999 commented Jun 20, 2017

On write-back it's of course normalized in the backend (which is still necessary because the backend needs to do validation because it can't trust the data it gets from the frontend). Also sometimes it's useful to distinguish between null and undefined values which only works in Vue if I use code like { value:undefined }.

But because JSON.stringify omits fields with undefined values, it's in fact not easy to do. The only way to do it is to initialize the fields with undefined after receiving them from the backend - which requires a lot of boilerplate code.

@LinusBorg
Copy link
Member

LinusBorg commented Jun 20, 2017

Also sometimes it's useful to distinguish between null and undefined values

Agreed - somethimes for those few times:

initialize the fields with undefined after receiving them from the backend - which requires a lot of boilerplate code.

not really:

Object.assign({ valueThatcanbeUndefined: undefined/*,  other such values*/, objectFromAPI}

So generally, that's a one-liner and only a lot of boilerplate if you would deal with objects that had a lot of those kinds of properties.

@kay999
Copy link
Author

kay999 commented Jun 20, 2017

I also don't see frontend-code as as crucial to the data-modeling than the backend.

So just writing { value:{} } and letting the template define the fields of 'value' via v-model would be less boilerplatey then having to write { value:{ name:undefined, value:undefined, ... } } just because.

And after posting 'value' the backend (where the real data-models live) has to do validation anyway. So I don't see the necessity of Vue forcing me jumping through this hoop...

@kay999
Copy link
Author

kay999 commented Jun 20, 2017

And even one-liners can become lots of boilerplate if they are necessary in lots of places. It's also not DRY

@LinusBorg
Copy link
Member

LinusBorg commented Jun 20, 2017

Also the documentation should be updated. Instead of

"Due to the limitations of modern JavaScript (and the abandonment of Object.observe), Vue cannot detect property addition or deletion. Since Vue performs the getter/setter conversion process during instance initialization, a property must be present in the data object in order for Vue to convert it and make it reactive."

there should be a paragraph explaining that it's not because of a "limitations of modern JavaScript" but in fact intended behaviour.

No. Here you are mxining up two different things. The above pararaph is not about v-model, but any addition of aproperty with normal Javascript means. The limitation is that we technically can't detect when a new poperty is created by obj.newproperty = 'whatever' (no matter if that's done by v-model or you, the developer), so we can't make it reactive. That's why Vue.set() exists.

The choice that we don't use Vue.set() in the implementation of v-model is independent of the fact that we can not detect properties that are added without it and make them reactive "automatically".

@LinusBorg
Copy link
Member

LinusBorg commented Jun 20, 2017

I also don't see frontend-code as as crucial to the data-modeling than the backend.

We don't see it as trivial or unimportant, and I don't hink you do either. And our concerns are not about validation, but integrity and predictability.

If v-model always added props that were missing for you, then visiting a form could add a prop without the user doing anything, or as little as clicking into the input and then leaving again (depending on the concrete implementation we would choose).

So now you would have to keep in mind that this property could exist or not exist in various places in your app, depending on wether the user had prevously openened a form that might have added it, or not.

It could make code & data much more brittle and inconsistent, compared to code that would normalize the data that it receives from the API.

That's basically what I meant earlier - better code and data integrity at the cost of a bit of boilerplate when necessary.

@kay999
Copy link
Author

kay999 commented Jun 20, 2017

If v-model always added props that were missing for you, then visiting a form could add a prop without you doing anything, or as little as clicking into the input

I don't see a problem here, because it's clearly stated in the code by writing 'v-model'. If you look at it from a declarative point of view, using v-model would be sufficient and also DRY.

Of couse you may argue that a template is for output only and shouldn't contain code which changes data, but then having v-model would be wrong anyway. Also I can always use the "long-form"

<input :value="value.field" @input="v => { $set(value, 'field', v) }>

in a template. By allowing this, Vue already goes beyond the simple "the template only shows things" concept. I assume that's because the concept of Vue is more about pragmatism than about enforcing certain paradigms or otherwise templates and models should have been much more decoupled (as in react/flux for example).

Also the "problem" with v-model is simply the "inherited" behaviour of the described "JS limitation". So the quoted paragraph from the docs also applies to v-model. If Vue would detect field addition to all watched objects, v-model would also create new fields - which it in fact already does, it just don't makes them reactive.

@doits
Copy link

doits commented Jun 23, 2017

This bites me some times, too. I'd vote to at least generate a warning in case v-model encounters an undefined.

In my case I have to introduce the boilerplate if the form content is dynamic, e.g. multiple instances can be created by an add button. For example:

<div v-for='item in items'>
  <input type="text" v-model="item.first_name"></input>
  <input type="text" v-model="item.last_name"></input>
  ... 10 more inputs ...
</div>
<button @click.prevent='add_new_item'>Add new item</button>
...
props: {
  items: {
    handler: Array,
    default() { return [] }
  }
},
methods: {
  add_new_item() {
    // `this.items.push({})` does not work
    // instead each attribute must be provided for reactivity:
    this.items.push({
      first_name: '',
      last_name: '',
      ... 10 more keys ...
   })
  }
}
...

It often happened to me that I added a new input in the template but forgot to add the key in the add_new_item() method. So at least a warning would be nice if the property is undefined.

(Or is there a better practice for that particular use case?)

@sirlancelot
Copy link

sirlancelot commented Jun 24, 2017

For forms I like to follow a structure wherein I define (in JS) an "empty" set of fields like so:

const emptyFields = Object.freeze({
    firstName: "",
    lastName: "",
    phoneNumber: ""
})

If you want your form to allow multiple entries, then you just have to clone your "empty" object:

function getNewItem() {
    return { ...emptyFields }
}

It's still mildly inconvenient to remember to add the field to the "emptyFields" object when you add a corresponding input element to your form, but at least you've documented your code well by creating a variable called "emptyFields" which conceivably should contain all your form fields in an empty state. And as @LinusBorg points out, it actually helps you to write better code.

However, I still agree that at least during development, a warning should be emitted when attempting to assign to a non-reactive property.

@yyx990803 yyx990803 changed the title Use Vue.set for v-model data-binding instead of a simple js assignment Warn when v-model is bound on non-existent key Jun 26, 2017
@kay999
Copy link
Author

kay999 commented Jun 30, 2017

While I still think that a framework shouldn't dictate code style to much and let the developers decide about that, at least submitting a warning would be an improvement.

OTOH at least for prototyping using Vue.set would be a real time-saver for me, so I would still prefer at least having the ability to use a global option or use for example v-model.set to activate the behavior.

@kay999
Copy link
Author

kay999 commented Jun 30, 2017

Data validation needs to be done in the back-end (because a malicious user could always post arbitrary data). So forcing the developer to specify data structures on both the back-end and the front-end is unnecessary, not DRY, and leads to boilerplate.

So while it's useful for front-end-only structures, it's not for general forms where the front-end only displays data and posts it back (typical CRUD operations). In the moment Vue makes it unnecessarily cumbersome to do this.

Also I prefer to use 'undefined' as a value for unspecified values, but this don't work with Vue because JSON omits fields with undefined values. So I have to use null or '' instead (which is problematic in certain situations) or write code which initializes the data in the front-end after receiving it from the back-end.

Another example are array-based fields where each entry in an array creates a new "sub-form". But in the moment we need a solution like sirlancelot's one above instead of just seeing the template as a specification for the data. So instead of just writing the template, I also have to create (and maintain) a template-object, write a create-method and bind this method to my array-edit-component. In bigger projects all this code adds up without really making everything more readable because instead of just looking at my template to see what happening, I have also to check some boilerplate.

And from a more conceptual point of View: In Vue template are "active" because you can bind mutations to it. That's a fact and it's used all over the place. Also JS handles undefined fields similar to fields with a value of 'undefined' in many occasions. So I don't think it't plausible to draw the quite arbitrary line of allowing to change data-fields but not create them.

@noscript
Copy link

noscript commented Sep 4, 2017

In my case there has been done refactoring on the back-end, and so some of the keys were renamed. I would prefer to see a warning as well, if the key doesn't exists.

@chriscasola
Copy link
Contributor

I can open a pull request to add the warning.

chriscasola added a commit to chriscasola/vue that referenced this issue Oct 4, 2017
Warn when v-model is bound to a non-existant key since that key
will not be reactive.

Fixes vuejs#5932
@yyx990803
Copy link
Member

I just re-read the whole thread, and decided that we are going to allow v-model to create reactive properties if they don't exist in all cases, but only one level deep.

This means the following will work assuming foo is an empty object:

<input v-model="foo.bar">

But this will still throw an error (cannot read property 'baz' of undefined):

<input v-model="foo.bar.baz">

The Reasoning

Dev Experience

For forms, having to declare all nested properties can indeed be a chore. Silently failing is also obviously bad. We've also seen the question of "resetting form data" popping up many times. In terms of convenience, auto-setting would simplify it down to resetting to an empty object.

Consistency

Even with warnings the current behavior is inconsistent in a few cases: when we do a dynamic key segment, e.g. v-model="foo[bar]" we are already using $set internally because you can't statically declare a property that is only known at runtime. Another case is when binding to an array via index v-model="arr[1]" we also have to use $set to ensure the array updates.

An longer-term concern is that when we move to Proxies in 2.x-next, detecting property addition would become trivial and it would in fact be more difficult to add a property without making it reactive.

Code Quality / Framework Opinion

We originally changed this behavior in 2.0 to be a bit more opinionated on data declaration. Changing it back certainly weakens that opinion. But now I think of it as an ok middle ground because:

  • The auto-reactive-property-creation is limited to one-level deep; The declare requirements also still exist for root-level properties. This pretty much leaves us down to objects dedicated for form bindings.

  • It is still good practice to declare the properties when you can, but I can also see how being able to omit them can speed up prototyping. Luckily JavaScript type systems have now become quite mature and I think we can start to leave some of the opinions to be enforced via opt-in tools like TypeScript or Flow.

@omidkrad
Copy link

omidkrad commented Oct 13, 2017

We've also seen the question of "resetting form data" popping up many times.

For resetting form data this is what we came up with (see second code snippet), which I think is a pretty good solution. With Vue 2.5 I was wondering if there is a somehow native/suggested way to do it?

@sebwas
Copy link

sebwas commented Oct 16, 2017

This did break my application and here's why and what I did to fix it:

Problem

I am using models to represent my data entitities, such as customer or product and for that I am using all kinds of shiny "new" features, such as classes and proxies. Put simply, I am using classes for my models that have an attributes property that holds the data and the class's constructor returns a proxy with getter and setter traps. That's cool.
The getter checks if the object has the requested property and returns it if it exists. Otherwise it returns the property from the object's attributes property (defined or undefined).
The setter directly sets the attributes property since the direct manipulation of the object's properties is not supposed to happen after initialization.

However, after updating to 2.5 I started to notice some weird behavior: directly mapping the model's attributes to input fields did not work correctly any longer.
The following behavior: the user would input something, the attributes property would not get updated as intended, but instead the object itself would get new properties.

Solution

Digging into the core I discovered that Vue respects the use of defined getters and setters and makes a call to Object.getOwnPropertyDescriptor to find out if these exist. So I created another trap for getOwnPropertyDescriptor, specifying a setter for all, but a handful of properties like this:

// (simplified)

class Model {
  constructor (attributes = {}) {
    this.attributes = attributes;

    return new Proxy(this, {
      get: Model.getProxyAttribute,
      set: Model.setProxyAttribute,
      getOwnPropertyDescriptor: (model, prop) => {
        if (['__ob__', 'attributes'].indexOf(prop) < 0) {
          return { configurable: true, set: value => model.setAttribute(prop, value) };
        }

        return Object.getOwnPropertyDescriptor(model, prop);
      }
    });
  }

  static getProxyAttribute (model, attribute) {
    return model.getAttribute(attribute);
  }

  static setProxyAttribute (model, attribute, value) {
    model.setAttribute(attribute, value);

    return true;
  }

  getAttribute (attribute) {
    if (this[attribute]) {
      return this[attribute];
    }

    return this.attributes[attribute];
  }

  setAttribute (attribute, value) {
    this.attributes[attribute] = value;
  }
}

Notice the '__ob__' property exception. This is necessary for Vue to work properly.

Conclusion

This most likely is not the most elegant solution and I want to encourage everyone to enhance my solution to make it better; while this seems to work I am not entirely happy with it.

@brianjorden
Copy link

I know this is done and closed, but I wanted to say, I'm extremely happy this functionality is available now. Building a very data centric app and was having to maintain "blank" samples of data objects that otherwise were having forms for editing dynamically generated. This just simplifies so much pain I've been dealing with, so thank you for being open to loosening some of the opinions of the framework. I'm not against there being opinions, but I tend to like when there are some without being overbearing. The flexibility, listening to feedback, considering real world usage, and some of the opt in/out ability is why Vue stands out to me. So, thanks. @yyx990803

@chriszrc
Copy link

If we're using typescript, and we're only using objects with set interfaces in our templates, then shouldn't vue generate errors when the template is referencing unknown data variables or unknown object properties? This seems like it defeats the whole point of only using strongly typed objects in our templates-

@gsailland
Copy link

gsailland commented Feb 16, 2022

I found a simple way :)
td v-if="ec.hasOwnProperty('countries')" v-select multiple :options="countries" :reduce="country => country.code" label="label" v-model="ec.countries"/v-select /td td v-else{{$set(ec,'countries',[])}}/td

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

Successfully merging a pull request may close this issue.