Add new rule: jsx-no-bind #85
Conversation
|
Thanks for your interest in palantir/tslint-react, @lvillani! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
There was a problem hiding this comment.
nice, this looks pretty good as a simple syntactic check. it's prone to some rare false positives because it doesn't use the type checker, but I'm ok with that.
if you have time to refactor the walker for performance, that would be great. if not, we could clean it up in a future PR.
src/rules/jsxNoBindRule.ts
Outdated
| } | ||
|
|
||
| private reportFailure(node: ts.Node) { | ||
| this.addFailure(this.createFailure(node.getStart(), node.getWidth(), Rule.FAILURE_STRING)); |
There was a problem hiding this comment.
can you use this.addFailureAtNode instead?
src/rules/jsxNoBindRule.ts
Outdated
| } | ||
| } | ||
|
|
||
| class JsxNoBindWalker extends Lint.RuleWalker { |
There was a problem hiding this comment.
would you mind refactoring to use WalkContext and applyWithFunction? see https://palantir.github.io/tslint/develop/custom-rules/performance-tips.html and https://palantir.github.io/tslint/develop/custom-rules/walker-design.html. the script in this repo (./scripts/newRule.js) ought to scaffold this up for you.
src/rules/jsxNoBindRule.ts
Outdated
| private propertyAccessCount = 0; | ||
|
|
||
| protected visitNode(node: ts.Node) { | ||
| console.log("kind:", ts.SyntaxKind[node.kind], "text:", node.getText()); |
There was a problem hiding this comment.
remove console statements please
|
also please enable CircleCI for your fork so that status can be reported here. |
src/rules/jsxNoBindRule.ts
Outdated
| return; | ||
| } | ||
|
|
||
| if (!expression.getText(ctx.sourceFile).includes(".bind")) { |
There was a problem hiding this comment.
@adidahiya I reworked the check to use a walk function as you suggested. I'm not really sure about this line though.
It seems to work in our project without false positives, but it looks brittle to me. Is there a better way to do this?
There was a problem hiding this comment.
minor: can you use node.text instead of getText()? perhaps using some part of the expression subtree here?
overall this seems fine; we can only really hope to do better if we used the type checker (I think its costs outweigh its benefits here)
There was a problem hiding this comment.
also maybe we should limit ourselves to only linting .bind(this)? are there any other cases this rule should lint for?
There was a problem hiding this comment.
I addressed the latter request by changing the code to search for .bind(this) but I can't seem to be able to use node.text or expression.text anywhere since I get a compilation error.
References #44
References #44