eslint-plugin-vue
eslint-plugin-vue copied to clipboard
vue/no-mutating-props option: {"propProps": false}
What rule do you want to change? vue/no-mutating-props
Does this change cause the rule to produce more or fewer warnings?
A new, {"propProps": false}
option would cause it to produce fewer warnings.
How will the change be implemented? (New option, new default behavior, etc.)?
New option: {"propProps": false}
(i.e. Don't error when mutating prop properties)
Please provide some example code that this change will affect:
<template>
<div>
<input type="range" v-model.number="point.x" >
<input type="range" v-model.number="point.y" >
</div>
</template>
<script>
export default {
props: {
point: {
type: Object,
required: true
}
},
}
</script>
What does the rule currently do for this code? Trigger an error
What will the rule do after it's changed?
When the default option is set ({"propProps": true}
), the error will trigger. When the option is set as {"propProps": false}
, it will not trigger an error.
Additional context
(Note there are some issues in the past suggesting this rule be loosened (#1339 , #1314; cc @leosdad @pimlie @decademoon ); here I'm suggesting an option be added instead)
My main reason for wanting an option here, is that modifying the reference of a prop (e.g. this.myProp = 3
) will cause actual run-time warnings from Vue, cause unexpected behaviour, and is deprecated:
Due to the new rendering mechanism, whenever the parent component re-renders, the child component’s local changes will be overwritten. - https://vuejs.org/v2/style-guide/#Implicit-parent-child-communication-use-with-caution
Here's the warning Vue throws:
Avoid mutating a prop directly since the value will be overwritten whenever the parent component re-renders. Instead, use a data or computed property based on the prop's value. Prop being mutated: "number"
Modifying the value of a prop (but leaving the reference the same) (e.g. this.myProp.x = 3
), does not throw runtime warnings, and (I believe) does not cause any unexpected behaviour—it's just a code style decision. As long as the actual reference to the object remains unaltered, Vue should not have any issues.
Here's a sandbox with these two cases for reference: https://codesandbox.io/s/elegant-lederberg-p1ilc
(Note: This is the same way that const
behaves in JS, so shouldn't be unexpected to most developers:
const x = 3;
x = 7; // Error!
const y = {};
y.foo = 3; // This is fine; it's the _reference_ that's const, not the value.
y = 20; // Error
)
I would love it if I could configure linting to error for reference prop modifications (which I need to fix), but to ignore value prop modifications (which I often prefer using in my code). If an option is added called, say {"propProps": true}
, then I would use it instead of disabling the rule entirely.
P.S. How would one implement my code snippet above without prop value modification? The nicest way I can think of is something like this, which doesn't look particularly better to me.
<template>
<div>
<input type="range" v-model.number="x" >
<input type="range" v-model.number="y" >
</div>
</template>
<script>
export default {
props: {
value: {
type: Object,
required: true
}
},
data() {
return {
x: this.value.x,
y: this.value.y,
};
},
watch: {
// Duplicate; could create a `mounted` method and use `$watch` to watch everything
x(newVal) { this.$emit('input', { x: newVal, y: this.y } },
y(newVal) { this.$emit('input', { x: this.x, y: newVal } },
}
}
</script>
(Similar to https://simonkollross.de/posts/vuejs-using-v-model-with-objects-for-custom-components )
Actually, vue/no-mutating-props
is largely the vue-equivalent to https://eslint.org/docs/rules/no-param-reassign ; there they have:
-
no-param-reassign: ["error", { "props": false }]
as the default (modifying param props is ok) -
no-param-reassign: ["error", { "props": true }]
disable modifying param props as well
So a better proposal might be to make:
-
vue/no-mutating-props: ["error", { "propProps": true }]
as the default (modifying property props is NOT ok) -
vue/no-mutating-props: ["error", { "propProps": false }]
modifying property props is ok
In a breaking change, renaming this to vue/no-prop-reassign
might be nice to make it more eslint-y, but I don't think that's worth stalling this. It would also be nice if propProps: false
was the default like it is for eslint, but anyways :)
^ Updated title/description accordingly
I strongly agree with this issue - vue/no-mutating-props
without this practically makes v-model unusable except on root level and you end up with something like this:
:value="value.search"
@input="$emit('input', { ...value, search: $event.target.value })" />
IMO proposal that led to creation of this rule had a big flaw: This:
<input
:value="todo.text"
@input="$emit('input', $event.target.value)"
>
Is not an equivalent of this:
<input v-model="todo.text">
In this case how would parent know that todo.text
should be updated on input
? I assume that this is not weird scenario where parent is passing whole object but expects that only single property will be modified. Moreover if parent would bind todo
using v-model="todo"
then $emit('input', $event.target.value)
would override todo
with a string
value 🤦🏼♂️
I agree with this issue too.
In my case, I have several objects that have some common properties (they implement the same interface), and I want to have a common component that represents a form for updating those properties. I pass the object to that form component, and inside the component I use several <input type="text" v-model="object.property">
.
I think the suggested vue/no-mutating-props: ["error", { "propProps": false }]
configuration will work perfect for me.
I also agree this needs to change. The v3 documentation says:
In 3.x v-model on the custom component is an equivalent of passing a modelValue prop and emitting an update:modelValue event
So this rule should not be flagged when the prop is an object reference.
I also think this should be optional. There are cases where using v-model
on properties of object passed via a props greatly simplifies the code and if the behavior is properly documented (and component properly named), it should be fine.
Please, give us the choice!
Any update on this one ?
I would add that the recommendation for using props for parent→child communication and events for child→parent is only pertinent for simple values/objects like forms , date-pickers etc.
Let me present a scenario that illustrate the need for this rule modification :
Suppose you have a component that handles some complex objects. Since this object is complex, you need a big template to handle it.
At some time, you realize some sub object in this object could be rendered by themselves elsewhere independently of the main object... Or just, you think it's taking to much space in the template and you want to split it... ... That mean you want to delegate the handling of this sub-object to a dedicated component. The easiest way to do it is to pass it as a prop.
Of course (and it is the original purpose of this rule) changing the object the component operates on should be a privilege of the parent (the child cant reassign props.subobject = [...]
), 1) because it has been deprecated, 2) because it would mean the child don't work anymore on the object the parent component assigned.
However, there is nothing wrong against modifying the sub-object's props from the child since it is exactly what was done in the big component before splitting it to delegate this task !
...Yet you get this vue/no-mutating-props
non-sense.
Now, what about using event ?
Ok, so you now have to implement all the v-model stuff. Then each time any sub-prop is changed, you have to emit an event with the full object (or implement many sub-events to refresh only parts of it... That is basically re-implementing reactivity...). Once the event fired, you deep copy the new state into the state (that maybe slow since it's a complex object with possibly thousand of nested keys...) And now that you updated the global state, guess what ? ALL object that used some properties of the sub-object should now refresh (even if what they listened to was not updated... since we did a deep-copy reassigning the the objects, except if we complicated even more by adding a diff algorithm...)... And that's not all !!! What if the child component gets too complicated itself and need to be split too ? ...Then you get all this mess over again at all nesting levels.
I ask you again : What is the simplest way of implementing it ? Mutable object passed as props and let reactivity do the job or props/event stuff ?
For debugging, you can simply add watchpoint... Or use instrumentation to log the changes...
Thus, I think that :
- This rule should be changed to pop a warning about prop's props mutation with the instruction to disable it
- It should be officially stated that for complex objects, it's okay to go this way
Very much agree with this, the rule has been a thorn in our side to the point that it's straight up disabled. We have a rule that dictates we cannot mutate properties of a prop, but fails to provide examples of how we are supposed to do this. And examples I've found are... cumbersome and unweildly, to say the least
The rule as it currently exists has been one of a few examples some dev use as to why "rules are bad and just get in the way". And while that sentiment may be wrong, given the context, they are right, this rule does just get in the way because it fails to be nuanced.
I look forward to this being implemented. However, it's been over a year now, so I'm not holding my breath.
The rule as it currently exists has been one of a few examples some dev use as to why "rules are bad and just get in the way". And while that sentiment may be wrong, given the context, they are right, this rule does just get in the way because it fails to be nuanced.
Well said @douglasg14b
I made multiple implementation where this rule makes sense - working with inputs or set of inputs treated as single, complex, custom input, makes a lot of sense and move implementation on another level.
Moment when you’re working with multi step, complex forms, everything crumbles into pieces and makes your code unreadable thanks to bazylion manually handled events flying left and right, that effectively throws v-model out of the window.
I agree with @douglasg14b - this rule is good but current implementation is lacking a lot of nuances.
Also agree with this, we need the choice, while it would really not be ok in a lib, in an app, making abstraction when some blocks of components are reused a lot throughout the code, we can just copy paste the code, add some props and be done with it, but with this rule it forces us to go through an unnecessary clone of the original object and add emits which greatly complexify the abstraction process
This is a good rule to abide by but not 100% of the time. There are legitimate cases where this rule should not apply, and doing so is actually detrimental.
I’ve turned off this rule in my codebase because it’s more hassle than it’s worth.
I can help speed things up with a PR, I already have the tests and the option schema, no I just have the implementation to do (it does look a bit complex to implement correctly though)
My proposal for the option name: directOnly: true or shallowOnly: true rather than propProps: false
A PR would certainly be appreciated! I like shallowOnly
most. Another suggestion: ignoreDeepMutations
(ignore
would be consistent with other rule options).
For the implementation, the rule should only warn about direct modification of the props
object, and not deeper. As I explained in my previous post, modifying a reactive object passed as props should even be encouraged in a model/delegate paradigm
When objects and arrays are passed as props, while the child component cannot mutate the prop binding, it will be able to mutate the object or array's nested properties.
So eslint shouldn't throw an error when mutate objects or arrays' nested properties.
I have been digging through the code to make an option for this eslint rule and trying a lot of things but it's not a straightforward implementation, the code is very hard to read through, there is a lot of cases (template mutation, script mutation, this, no this, https://github.com/vuejs/eslint-plugin-vue/blame/master/lib/rules/no-mutating-props.js to get an idea) to account for and my time is quite limited so it will probably take a while until I can get this PR ready
Any news? It'd be really awesome to have it configurable and to actually see the problem when it is really there..
No news yet. If nobody works on this and opens a PR, no progress will be made. Asking for updates won't speed that up. So if anybody feels entitled to implement this option, please go ahead!