New rule: Report usages of class methods besides render returning JSX #1580
Conversation
| @@ -303,6 +303,16 @@ function componentRule(rule, context) { | |||
| return calledOnReact; | |||
| }, | |||
|
|
|||
| isReturningArrayMap(node) { | |||
There was a problem hiding this comment.
i'm concerned about this approach; there's other things that have a .map method. How can we know this is an array?
| '}' | ||
| ].join('\n'), | ||
| errors: [{ | ||
| message: '\'names\' is missing in props validation' |
There was a problem hiding this comment.
this is a good fix; but this should be able to go in a separate PR
There was a problem hiding this comment.
It's a by-product of checking map. Considering this request and your concern above, should I remove the map support for now with it being added later as an enhancement?
There was a problem hiding this comment.
In the case of the prop-types rule, names isn't inside a map, so it should be caught regardless.
There was a problem hiding this comment.
It doesn't have to do with the JSX being returned in map. The reason it's being caught now is because isReturningJSX wasn't considering functions with return statements that are calling Array#map. So, in order to have this test case have an error, it required the same change that was made to check for returning Array#map in a component class method.
There was a problem hiding this comment.
Right - I'm saying that the bug fix for prop-types can be to consider these return statements, even if it ignores the code inside the .map callback.
There was a problem hiding this comment.
Okay, so in order to still support the Array#map support for this rule, you want me to find a solution that makes it work while also not resolving the issue with that particular test case?
There was a problem hiding this comment.
I'm thinking that the prop-types issue can be solved without having to guess that .map is "array map".
Then separately, this rule may be able to avoid making the same guess.
|
I like this rule also. For reference, I have a PR that adds similar functionality to the existing |
ljharb
force-pushed
the
master
branch
6 times, most recently
from
59af733 to
865ed16
Compare
ljharb
force-pushed
the
master
branch
4 times, most recently
from
069314a to
181c68f
Compare
ljharb
force-pushed
the
master
branch
2 times, most recently
from
380e32c to
51d342b
Compare
I saw #578 and thought that the requested rule was useful. I personally don't like components that define extra render* methods that return JSX. It is a way to clean up the main render method, but the better choice is to move the logic to separate components. This rule will try to enforce that.
I wasn't sure about the rule name, so I'm open to a better name if it's offered.
Closes #578