-
-
Notifications
You must be signed in to change notification settings - Fork 681
vue/no-mutating-props option: {"propProps": false} #1371
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
Comments
Actually,
So a better proposal might be to make:
In a breaking change, renaming this to |
^ Updated title/description accordingly |
I strongly agree with this issue -
IMO proposal that led to creation of this rule had a big flaw:
Is not an equivalent of this:
In this case how would parent know that |
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 I think the suggested |
I also agree this needs to change. The v3 documentation says:
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 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... 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 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 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...). 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 :
|
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. |
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 |
For the implementation, the rule should only warn about direct modification of the |
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 |
Note: the eslint rule 'vue/no-mutating-props' has been disabled. See vuejs/eslint-plugin-vue#1371
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! |
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:
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: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:)
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.
(Similar to https://simonkollross.de/posts/vuejs-using-v-model-with-objects-for-custom-components )
The text was updated successfully, but these errors were encountered: