Add new rule for preventing this in SFCs
#1509
Conversation
|
Okay, I can update the rule to report any usage of this. You mean all usages of |
|
Yes; an SFC has no reason to use |
jomasti
changed the title
this.props in SFCsthis in SFCs
|
Updated |
| ruleTester.run('no-this-in-sfc', rule, { | ||
| valid: [{ | ||
| code: ` | ||
| function Foo(props) { |
There was a problem hiding this comment.
Sorry, this is really a nit :) don't these template literals need an extra indentation?
code: `
mystring...
`,
There was a problem hiding this comment.
I think the need is up to personal preference. I can change it if that is the standard preference.
There was a problem hiding this comment.
imo the indentation of test cases in a non-indentation rule isn't that important, as long as it's readable.
lib/util/Components.js
Outdated
| return false; | ||
| } | ||
| property = 'argument'; | ||
| } |
There was a problem hiding this comment.
Is this code really needed?
no-typos rule also detects SFCs, it does it like so: ( stripped down, the original code is at: https://github.com/yannickcr/eslint-plugin-react/blob/master/lib/rules/no-typos.js#L137 )
MemberExpression: function(node) {
const relatedComponent = utils.getRelatedComponent(node);
if (
relatedComponent && utils.isReturningJSX(relatedComponent.node)
) {
// Should be an SFC at this point
}
There must be other places where SFC detection also exists.
On the other hand, perhaps this is a better approach, I don't know too much about the internals there...
There was a problem hiding this comment.
Well, when I was calling getParentStatelessComponent before the change, it was returning any parent function, which is not what I wanted. So, I updated the function to check that the function is also returning JSX. When I did that with the previous version of isReturningJSX, it would fail on an ArrowFunctionExpression with a body. This caused several tests to fail. After I updated this case to check for the body, it resolved the failures. There may be multiple rules that do that sort of check, but I would prefer the logic to be in one place.
| } | ||
| ``` | ||
|
|
||
| The following patterns are **not** considered warnings: |
There was a problem hiding this comment.
let's also add non-warning patterns (and test cases) for destructured props/context
lib/rules/no-this-in-sfc.js
Outdated
| }, | ||
|
|
||
| create: Components.detect((context, components, utils) => ({ | ||
| MemberExpression: function(node) { |
lib/util/Components.js
Outdated
| @@ -442,7 +449,7 @@ function componentRule(rule, context) { | |||
| return null; | |||
| } | |||
| // Return the node if it is a function that is not a class method and is not inside a JSX Element | |||
| if (isFunction && !isMethod && !isJSXExpressionContainer) { | |||
| if (isFunction && !isMethod && !isJSXExpressionContainer && utils.isReturningJSX(node)) { | |||
There was a problem hiding this comment.
does isReturningJSX return true when something returns null?
Also, in React 16, components can return strings or arrays, so I'm not sure this detection will be reliable moving forward.
"React 16 fragments" will probably be something we have to solve in a separate PR anyways tho, so no need to address that now (i filed #1510)
This rule will report errors when a stateless functional component is attempting to use `this`.
|
Is there anything stopping from getting this merged? Just ran into an issue with this, using |
This rule will report errors when a stateless functional component is
attempting to use
this.This attempts to address the request in #1435.