Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: disallow explicit calls to lifecycle hooks #427

Closed
lazarljubenovic opened this issue Oct 8, 2017 · 8 comments
Closed

Comments

@lazarljubenovic
Copy link
Contributor

Disallow code like

method() {
  this.ngOnDestroy()
}
ngOnDestroy() { /* cleanup code here */}

Instead, we should write something like

cleanup() { /* cleanup code here */}
method() {
  this.cleanup()
}
ngOnDestroy() {
  this.cleanup()
}

As Rob put it, "don't call your own hooks, because one day, you'll be debugging something and you'll be really confused why hooks are firing unexpectedly/out of order/whatever".

Thought it would be a nice addition to codelyzer.

@mgechev
Copy link
Owner

mgechev commented Oct 9, 2017

I like this proposal. Lets implement it.

@mgechev
Copy link
Owner

mgechev commented Nov 1, 2017

Btw, this is not very trivial to solve in the generic case. We need to check if given object instance implements a lifecycle hook interface and later, it's method is being invoked somewhere.

However, I think a basic check, such as:

  • Find all method invocations in a class.
  • Look for method names reserved by lifecycle hooks.
  • Report a warning if find one.

Seems good enough.

@chaosmonster
Copy link

chaosmonster commented Jul 24, 2018

How can I disable this rule for tests?

I can see the benefit for application code, but within specs I actually do want to call lifecycle hooks to test them.

@michaeljota
Copy link

@mgechev I second the question. How can this be disabled just for testing? (Aside for the obvious solution of turning it off in a comment)

@mgechev
Copy link
Owner

mgechev commented Aug 17, 2018

There are two options:

  • Comments in the beginning of the file
  • Two tslint config files - one for tests which does not include this rule and another for your app, which includes the rule and extends the original one

@michaeljota
Copy link

@mgechev Just for you to notice, that Angular CLI seems to ignore the linterOptions options setting of tslint. I know that this is also an Angular CLI problem, but the solution of having two tslint config files won't work. Thanks any way.

@mgechev
Copy link
Owner

mgechev commented Aug 20, 2018

If there's no issue for this in the Angular CLI repo, it might be worth opening.

@michaeljota
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants