feat(eslint-plugin): [no-implicit-takeuntil-destroyed] add rule#2803
Conversation
rznn7
force-pushed
the
feat/add-rule-no-implicit-takeuntil-destroyed
branch
2 times, most recently
from
3c093be to
e691df7
Compare
rznn7
force-pushed
the
feat/add-rule-no-implicit-takeuntil-destroyed
branch
from
e691df7 to
29561f9
Compare
|
Hi ! May I ask a review for this PR? |
|
View your CI Pipeline Execution ↗ for commit 585e278
☁️ Nx Cloud last updated this comment at |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
JamesHenry
left a comment
JamesHenry
left a comment
There was a problem hiding this comment.
Sorry for the delay and the need for your to rebase @rznn7. You will also please need to address the CI feedback.
Worth noting it would not report takeUntilDestroyed() usage in a constructor that is not in a @component or @Injectable, but I assumed it was out of scope for this rule.
I'm not seeing anything in your code or tests that backs this up, why do you feel this would be the case?
I think we should link to https://angular.dev/guide/di/dependency-injection-context where you currently have (constructor or field initializer) in the error message
|
No worries, thank for your answer!
I meant that it would not report an error in the constructor of a class that is not an edit: added failing test case for this: #2803 (comment) |
rznn7
force-pushed
the
feat/add-rule-no-implicit-takeuntil-destroyed
branch
from
29561f9 to
e492e31
Compare
rznn7
force-pushed
the
feat/add-rule-no-implicit-takeuntil-destroyed
branch
from
e492e31 to
ba0e5b5
Compare
But it WILL though?? You haven't added any code to prevent that, nor tests to prove that, that's what I'm saying. You are saying it would not do that, but it will? |
|
Maybe this is a language barrier, did you mean that it should not? And that you haven't implemented it yet? |
rznn7
force-pushed
the
feat/add-rule-no-implicit-takeuntil-destroyed
branch
2 times, most recently
from
bbfa5a5 to
4724e4d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Failing CI |
|
Otherwise LGTM |
rznn7
force-pushed
the
feat/add-rule-no-implicit-takeuntil-destroyed
branch
from
926f4a2 to
cd0bce3
Compare
|
Hope it works now 👍 |
There was a problem hiding this comment.
Important
Nx Cloud has identified a possible root cause for your failed CI:
Our E2E tests are failing due to a pre-existing infrastructure issue where @angular-eslint/template-parser distribution files are missing from the test registry. The similar-task-failure-detector confirmed this same error exists in the main branch. Since this PR only adds a new ESLint rule and doesn't modify the template-parser package or build configuration, these failures are unrelated to the PR changes and require infrastructure fixes to the template-parser build/packaging pipeline.
No code changes were suggested for this issue.
If the issue was transient, you can trigger a rerun by pushing an empty commit:
git commit --allow-empty -m "chore: trigger rerun"
git push
🎓 Learn more about Self-Healing CI on nx.dev
2 participants
Hi!
Fixes #2672
First time I am working on a lint rule, hope it fits! I tried to avoid false positives as much as possible.
Worth noting it would not report
takeUntilDestroyed()usage in a constructor that is not in a@Componentor@Injectable, but I assumed it was out of scope for this rule.Working on this also made me realize that a rule for the
inject()function would also be coherent.