Add no-mutation-props rule #1145
Conversation
|
I'm not super familiar with MobX, but it may be worth thinking about how this rule may impact those users? |
lib/rules/no-mutation-props.js
Outdated
| docs: { | ||
| description: 'Prevent direct mutation of this.props', | ||
| category: 'Possible Errors', | ||
| recommended: true |
There was a problem hiding this comment.
Changing which rules are recommended is semver-major, so that definitely shouldn't be done in this PR.
lib/rules/no-mutation-props.js
Outdated
| * @returns {Boolean} True if the component is valid, false if not. | ||
| */ | ||
| function isValid(component) { | ||
| return Boolean(component && !component.mutateProps); |
There was a problem hiding this comment.
let's use ‼x instead of Boolean(x) to coerce to booleans - and in this case, ‼component && !component.mutateProps is sufficient
lib/rules/no-mutation-props.js
Outdated
| function reportMutations(component) { | ||
| var mutation; | ||
| for (var i = 0, j = component.mutations.length; i < j; i++) { | ||
| mutation = component.mutations[i]; |
There was a problem hiding this comment.
can this use component.mutations.forEach instead of a loop?
lib/rules/no-mutation-props.js
Outdated
| mutation = component.mutations[i]; | ||
| context.report({ | ||
| node: mutation, | ||
| message: 'Do not mutate props.' |
There was a problem hiding this comment.
it might be nice if this error message held more information - in the message, or by providing source locations.
lib/rules/no-mutation-props.js
Outdated
| } | ||
|
|
||
| item = node.left.object; | ||
| while (item.object.property) { |
There was a problem hiding this comment.
what if item doesn't have a .object property?
lib/rules/no-mutation-props.js
Outdated
|
|
||
| 'Program:exit': function () { | ||
| var list = components.list(); | ||
| for (var component in list) { |
There was a problem hiding this comment.
let's use Object.keys(list).forEach if it's an object, and if it's an array, list.forEach - for..in should be avoided.
index.js
Outdated
| @@ -117,6 +118,7 @@ module.exports = { | |||
| 'react/jsx-uses-vars': 2, | |||
| 'react/no-deprecated': 2, | |||
| 'react/no-direct-mutation-state': 2, | |||
| 'react/no-mutation-props': 2, | |||
There was a problem hiding this comment.
(same here - adding rules to the recommended set is semver-major, so let's remove this)
Fix change requests mentioned in jsx-eslint#1145
|
Thanks for the quick review @ljharb! I've made the changes you requested. |
| ```jsx | ||
| var Hello = React.createClass({ | ||
| render: function() { | ||
| this.props.name = this.props.name.toUpperCase(); |
There was a problem hiding this comment.
What about things like this.props.list.push('foo'), or other array mutators? What about delete this.props.name, or Object.defineProperty(this.props, something), or Object.assign(this.props, something)?
There was a problem hiding this comment.
^^ Further to that, you probably also want to catch this:
const {list} = this.props;
list.push('foo');
There was a problem hiding this comment.
I've got a PR submitted that builds on @ianschmitz's work and catches this and many other things as well :) #1416
| @@ -0,0 +1,19 @@ | |||
| # Prevent mutation of this.props (no-mutation-props) | |||
|
|
|||
| NEVER mutate `this.props`, as all React components must act like pure functions with respect to their props. | |||
There was a problem hiding this comment.
"NEVER" in all-caps is a bit strong, maybe change it to "Never"? :)
There was a problem hiding this comment.
I would be fine with making it stronger, but there's no such thing as "super caps"
Fix change requests mentioned in jsx-eslint#1145
|
@ianschmitz I hope you don't mind. I used your fork as a base and added a bunch more fixes. #1416 @ljharb @Daniel15 I'd love to get that PR looked at, and if it's good, we can port nearly all of it to the |
|
Not at all @joeybaker. Sorry, I've fell out of touch with this PR 😢 Thanks for helping out everyone! |
|
Closing in favor of the updated PR over at #1416. Thanks @joeybaker ! |
|
In the future it'd be preferable to ask maintainers to add your commits to the original PR; replacement PRs are clutter. |
|
@ljharb Sounds good. I'm happy for you to do that and close the new PR :) |
|
Nah at this point it's open, we'll continue with that :-) |
PR for #1113.
Note the change in /index.js to the recommended rules. Not sure if this rule should be part of the recommended set or not?
@ljharb This does not currently warn any time the entire props object is passed to a function. It currently has the same checks as the
no-direct-mutation-staterule.