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

vue/no-mutating-props option: {"propProps": false} #1371

Closed
cdrini opened this issue Dec 9, 2020 · 20 comments · Fixed by #2135
Closed

vue/no-mutating-props option: {"propProps": false} #1371

cdrini opened this issue Dec 9, 2020 · 20 comments · Fixed by #2135

Comments

@cdrini
Copy link

cdrini commented Dec 9, 2020

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 )

@cdrini
Copy link
Author

cdrini commented Dec 12, 2020

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

@cdrini cdrini changed the title vue/no-mutating-props "reference-only" option vue/no-mutating-props option: {"propProps": false} Dec 12, 2020
@cdrini
Copy link
Author

cdrini commented Dec 12, 2020

^ Updated title/description accordingly

@mgibas
Copy link

mgibas commented Feb 15, 2021

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 🤦🏼‍♂️

@nestorrente
Copy link

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.

@mlilback
Copy link

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.

@Liwoj
Copy link

Liwoj commented Jul 26, 2021

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!

@ligne13
Copy link

ligne13 commented Oct 14, 2021

Any update on this one ?

@hl037
Copy link

hl037 commented Oct 29, 2021

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 :

  1. This rule should be changed to pop a warning about prop's props mutation with the instruction to disable it
  2. It should be officially stated that for complex objects, it's okay to go this way

@douglasg14b
Copy link

douglasg14b commented Jan 22, 2022

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.

@mgibas
Copy link

mgibas commented Jan 22, 2022

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.

@Tofandel
Copy link

Tofandel commented Mar 10, 2022

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

@decademoon
Copy link

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.

@decademoon
Copy link

I’ve turned off this rule in my codebase because it’s more hassle than it’s worth.

@Tofandel
Copy link

Tofandel commented Mar 10, 2022

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

@FloEdelmann
Copy link
Member

A PR would certainly be appreciated! I like shallowOnly most. Another suggestion: ignoreDeepMutations (ignore would be consistent with other rule options).

@hl037
Copy link

hl037 commented Mar 11, 2022

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

@ThomasKientz
Copy link

ThomasKientz commented Apr 22, 2022

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.

From Vue's documentation

So eslint shouldn't throw an error when mutate objects or arrays' nested properties.

@Tofandel
Copy link

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

leszekhanusz added a commit to leszekhanusz/diffusion-ui that referenced this issue Sep 25, 2022
Note: the eslint rule 'vue/no-mutating-props' has been disabled.
See vuejs/eslint-plugin-vue#1371
@padcom
Copy link

padcom commented Jan 31, 2023

Any news? It'd be really awesome to have it configurable and to actually see the problem when it is really there..

@FloEdelmann
Copy link
Member

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!

FlareZh added a commit to FlareZh/eslint-plugin-vue that referenced this issue Apr 20, 2023
FlareZh added a commit to FlareZh/eslint-plugin-vue that referenced this issue Apr 20, 2023
@ota-meshi ota-meshi linked a pull request Apr 30, 2023 that will close this issue
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.