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

[no current rule] rule proposal to forbid async lifecyclehooks #1515

Closed
2 tasks done
mhombach opened this issue Sep 5, 2023 · 3 comments · Fixed by #1643
Closed
2 tasks done

[no current rule] rule proposal to forbid async lifecyclehooks #1515

mhombach opened this issue Sep 5, 2023 · 3 comments · Fixed by #1643
Labels
package: eslint-plugin Angular-specific TypeScript rules PRs Welcome If a high-quality PR is created for this it will be accepted

Comments

@mhombach
Copy link

mhombach commented Sep 5, 2023

I propose a rule that forbids marking lifecyclehooks async (could be named no-async-lifecyclehooks ).

For example:

async ngOnInit() {
  // ...
}

The reason should be obvious: Angular does not wait for async lifecyclehooks but the code suggests it does. I regularly see something like this:

async ngOnInit() {
  const result = await fetchOnlineData(...);
  this.myData = result.filter(...);
}

afterViewInit() {
  // do something with this.myData
}

This code reads as that the ngOnInit lifecyclehook is fetching some data and that the whole lifecycle of the component is basically "pausing" until the data has arrived. Which is not true, as we know.

Disallowing the async notation here will make developers instantly aware of that they are now working with a state of the component where it will finish all init cycles first and THEN get the data:

ngOnInit() {
  fetchOnlineData(...).subscribe() {
    // uh oh... this is a callback, so it will fire at "some time" but after all hooks have finished
  }
}

afterViewInit() {
  // hmm, can't access the fetched data now since it hasn't arrived yet
}

Writing the rule should be very easy since I even can create it with the no-restricted-syntax myself:

"no-restricted-syntax": [
  "error",
  {
    "selector": "MethodDefinition[value.async=true][key.name=/^ng[A-Z]/]",
    "message": "Angular lifecycle hooks should not be 'async' since they will not be awaited for"
  }
]

I still feel this rule would definitely belong into an official angular-esling package like this one here. In this case I would not go the [key.name=/^ng[A-Z]/]-approach but explicitly add the lifecycle names.

For rule-options... I'm not sure if there is any option that would make sense. Excluding certain lifecyclehooks from this rule doesn't make any sense for the reasons explained above.

Would be glad to get some feedback and maybe have it added quickly :)

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest supported version of the packages and checked my ng version output per the instructions given here.
@mhombach mhombach added package: eslint-plugin Angular-specific TypeScript rules triage This issue needs to be looked at and categorized by a maintainer labels Sep 5, 2023
@JamesHenry
Copy link
Member

Thanks @mhombach, this makes sense as something that users shouldn't ever do, would you like to submit a PR for this?

@JamesHenry JamesHenry added PRs Welcome If a high-quality PR is created for this it will be accepted and removed triage This issue needs to be looked at and categorized by a maintainer labels Nov 8, 2023
@chimurai
Copy link
Contributor

chimurai commented Dec 2, 2023

Implemented the rule and named it no-async-lifecycle-method.

(to keep the naming consistent with the existing rules, which seem to follow the /.+lifecycle(-method)?$/ pattern)

Happy to hear your feedback and hopefully the PR can be accepted.

@mhombach
Copy link
Author

mhombach commented Dec 2, 2023

@chimurai big thanks for your PR, I just didn't have time yet but still had in mind to add the rule myself :)

Had a look at your PR and it looks very nice, I hope it will be merged soon =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: eslint-plugin Angular-specific TypeScript rules PRs Welcome If a high-quality PR is created for this it will be accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants