feat(ReactThisBindingIssueRule): allow decorators to bind #534
Conversation
|
|
src/reactThisBindingIssueRule.ts
Outdated
| if (!(this.allowedDecorators.length > 0 && node.decorators && node.decorators.length > 0)) { | ||
| return false; | ||
| } | ||
| const bindingDecorators = node.decorators.find((decorator) => { |
There was a problem hiding this comment.
some can be used instead of find.
Then conversion to boolean can be omitted later.
src/reactThisBindingIssueRule.ts
Outdated
| @@ -54,6 +69,9 @@ class ReactThisBindingIssueRuleWalker extends ErrorTolerantWalker { | |||
| if (typeof(opt) === 'object') { | |||
| this.allowAnonymousListeners = opt['allow-anonymous-listeners'] === true; | |||
| } | |||
| if (typeof opt === 'string') { | |||
There was a problem hiding this comment.
IMHO, it would be better to add "speaking" property to object, instead of listing all passed strings as decorators. So setup will be like
"react-this-binding-issue": [true, {
"allow-anonymous-listeners": true,
"bind-decorators": ["autobind", "custom-bind"]
}]
instead of
"react-this-binding-issue": [true, "autobind", "customBind", { "allow-anonymous-listeners": true}]
But it's up to maintainers.
There was a problem hiding this comment.
This is something I didn't find much examples of or documentation, how do I write a schema? because options is of type any
is the current one correct? tests are passing, but I'm more interested in options schema.
There was a problem hiding this comment.
AFAIK schema is used for validation/suggestions inside of tslint.json file, and not used to type options values inside rules/walkers.
For example you can check schema for rule in core TSLint repo: schema in docs and in code
src/reactThisBindingIssueRule.ts
Outdated
| this.scope = new Scope(null); | ||
| super.visitMethodDeclaration(node); | ||
| this.scope = null; | ||
| } | ||
|
|
||
| private isMethodBoundWithDecorators(node: ts.MethodDeclaration): boolean { |
There was a problem hiding this comment.
instead of using this.allowedDecorators, if we pass it to this function, then it'd become pure, will change.
|
@IllusionMH can you verify if everything is correct? Specially the schema part :) |
src/reactThisBindingIssueRule.ts
Outdated
| @@ -53,6 +71,14 @@ class ReactThisBindingIssueRuleWalker extends ErrorTolerantWalker { | |||
| this.getOptions().forEach((opt: any) => { | |||
| if (typeof(opt) === 'object') { | |||
| this.allowAnonymousListeners = opt['allow-anonymous-listeners'] === true; | |||
| if (opt['bind-decorators']) { | |||
| const allowedDecorators: any[] = opt['bind-decorators']; | |||
| if (allowedDecorators.constructor !== Array && allowedDecorators.some((decorator) => typeof decorator !== 'string')) { | |||
There was a problem hiding this comment.
Array.isArray should be more readable/convenient.
|| makes more sense here. Because second check should be performed only if opt['bind-decorators'] is an array (otherwise it will be runtime error)
|
@cyberhck at the first glance scheme looks good and I will check it more closely later. As for And it is still up to maintainers for make final review and approve everything. P.S. You can also check Hacktoberfest announcement if you haven't see it already 😃 |
| return <div | ||
| onClick={this.listener1}> | ||
| onMouseOver={this.listener2}> | ||
| onMouseDown={this.listener3}> |
There was a problem hiding this comment.
You've got a few syntax errors here in the test: > should only come after the last attribute.
There was a problem hiding this comment.
Ahh, my bad, I copied from existing one and I didn't notice that for some reason 😄 , I'll fix the original file as well :)
|
I just rebased, merged, can you please check? I did sign up for hacktoberfest before starting on this :) |
src/reactThisBindingIssueRule.ts
Outdated
| if (opt['bind-decorators']) { | ||
| const allowedDecorators: any[] = opt['bind-decorators']; | ||
| if ( | ||
| !Array.isArray(allowedDecorators.constructor) |
There was a problem hiding this comment.
Check should be for allowedDecorators not it's constructor.
Error below can be rephrase to mention that bind-decorators should be a list of strings, because now it will fail if string is passed, and not an array.
Most probably that's why test fail now, and removed string can be added back after fix here
There was a problem hiding this comment.
ahh, I missed the allowedDecorators.constructor part for some reason, my bad, will fix after I wake up now.
|
👍 |
|
Hey maintainers, is there anything I can do to make it better? or get merged? I kinda want this on another project 🙂 |
|
Heya @cyberhck sorry for the delay! Looking now. |
|
No problem, thanks for merging :) (a t-shirt awaits) |
|
I can't believe we missed this: #813 |
closes #392