fix(rule): fix bugs in no-life-cycle-call rule #575
Conversation
|
@kevinphelps thanks for the fix of #454. There seems to be a failure in Travis. |
|
I think you should only fix the |
|
I'm going to remove build change to see if it fixes the Travis build. It's in this PR simply because I needed for local development and I pushed it along with my fixes. |
src/noLifeCycleCallRule.ts
Outdated
| @@ -14,7 +14,8 @@ export class Rule extends Lint.Rules.AbstractRule { | |||
| typescriptOnly: true, | |||
| }; | |||
|
|
|||
| static FAILURE_STRING: string = 'Avoid explicitly calls to lifecycle hooks in class "%s"'; | |||
| static FAILURE_STRING_FOR_CLASS: string = 'Avoid explicit calls to lifecycle hooks in class "%s".'; | |||
| static FAILURE_STRING: string = 'Avoid explicit calls to lifecycle hooks.'; | |||
There was a problem hiding this comment.
I think you can only have one FAILURE_STRING (and let's remove the unnecessary :string):
static FAILURE_STRING = 'Avoid explicit calls to lifecycle hooks.';
There was a problem hiding this comment.
You can have more than one.
There was a problem hiding this comment.
I didn't mention that it's impossible. I'm suggesting that you remove and keep only one failure. :)
There was a problem hiding this comment.
I can remove the type annotation. I kept it because it was already there.
There was a problem hiding this comment.
Sorry, I misread. But I wanted to keep the class name when it's available.
There was a problem hiding this comment.
I have updated this.
| @@ -49,19 +50,35 @@ export class ExpressionCallMetadataWalker extends NgWalker { | |||
| } | |||
|
|
|||
| private validateCallExpression(node: ts.CallExpression): void { | |||
| const name = (node.expression as any).name; | |||
| const name = ts.isPropertyAccessExpression(node.expression) ? node.expression.name : undefined; | |||
| const expression = ts.isPropertyAccessExpression(node.expression) ? node.expression.expression : undefined; | |||
|
|
|||
There was a problem hiding this comment.
I intentionally separated these variable declarations for readability. Source node info vs. rule logic checks.
src/noLifeCycleCallRule.ts
Outdated
| return; | ||
| } | ||
|
|
||
| let currentNode = node as any; | ||
| let failureString = Rule.FAILURE_STRING; | ||
|
|
There was a problem hiding this comment.
I intentionally separated the if block with blank lines for readability.
There was a problem hiding this comment.
I don't mark the if block, but the block between let and const variables.
There was a problem hiding this comment.
I can remove if it violates the project's code conventions.
There was a problem hiding this comment.
Nah, it's just a nit. :)
kevinbuhmann
force-pushed
the
lifecycle-call-super
branch
3 times, most recently
from
624f55e
to
f3d0f31
Compare
kevinbuhmann
force-pushed
the
lifecycle-call-super
branch
from
f3d0f31
to
3e355c4
Compare
|
Thanks! |
|
Would you look into the failing build? Looks prettier related. |
|
Yeah, I can in a bit. Just need to reformat my changes since the prettier PR was merged first. |
Changes
See #573.
super.ngOnInit(), etc. should be allowed by theno-life-cycle-callrule.The
no-life-cycle-callrule would error with a "cannot read name of undefined" error when the life cycle call was not in a class. In my case, there was test code callingfixture.componentInstance.ngOnChanges()and the rule would crash.(Original commits: e9f4d23...dd63e7f)