-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
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
Comments
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. |
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. |
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. |
There are situations where it's necessary - and if it's necessary and you use 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 |
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 |
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. |
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,
It should not be the job of the UI ( |
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. |
Agreed - somethimes for those few times:
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. |
I also don't see frontend-code as as crucial to the data-modeling than the backend. So just writing 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... |
And even one-liners can become lots of boilerplate if they are necessary in lots of places. It's also not DRY |
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 The choice that we don't use |
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 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. |
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"
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. |
This bites me some times, too. I'd vote to at least generate a warning in case In my case I have to introduce the boilerplate if the form content is dynamic, e.g. multiple instances can be created by an <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 (Or is there a better practice for that particular use case?) |
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. |
v-model
is bound on non-existent key
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. |
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. |
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. |
I can open a pull request to add the warning. |
Warn when v-model is bound to a non-existant key since that key will not be reactive. Fixes vuejs#5932
I just re-read the whole thread, and decided that we are going to allow This means the following will work assuming <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 ReasoningDev ExperienceFor 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. ConsistencyEven with warnings the current behavior is inconsistent in a few cases: when we do a dynamic key segment, e.g. An longer-term concern is that when we move to Proxies in Code Quality / Framework OpinionWe 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:
|
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? |
This did break my application and here's why and what I did to fix it: ProblemI 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 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. SolutionDigging into the core I discovered that Vue respects the use of defined getters and setters and makes a call to // (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 ConclusionThis 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. |
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 |
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- |
I found a simple way :) |
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 = ...
The text was updated successfully, but these errors were encountered: