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
Fix: no-useless-constructor (#13830) #13842
Fix: no-useless-constructor (#13830) #13842
Conversation
Hi @AriPerkkio!, thanks for the Pull Request The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.
Read more about contributing to ESLint here |
012a7b4
to
d8a957e
Compare
Hi @AriPerkkio!, thanks for the Pull Request The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.
Read more about contributing to ESLint here |
d8a957e
to
b944314
Compare
Makes sense to me, I support this since it's a small change in the code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Not crashing is always good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Thanks for contributing! |
This is because there is an extension version of this rule which adds support for TS features (including fixing this crash): Base rules that are broken for TS code is our number one FAQ: https://github.com/typescript-eslint/typescript-eslint/blob/master/docs/getting-started/linting/FAQ.md#i-am-using-a-rule-from-eslint-core-and-it-doesnt-work-correctly-with-typescript-code I'm commenting here as a bit of an FYI for the eslint maintainers - feel free to forward people to our FAQ or to file issues with us. |
We're always doing that (actually, forwarding people directly to the extension rule if it exists, or to open an issue if it doesn't; thanks for the FAQ link!). This was just a small fix to at least not crash the whole linting. |
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
Fixes #13830
What changes did you make? (Give an overview)
Old implementation relies that class constructor has body. Added a check to verify this before accessing the object in order to prevent null pointer.
Is there anything you'd like reviewers to focus on?
As discussed on the ticket, it seems to be common practice to disable this rule when using typescript-parser. However I would rather avoid linter crash when this rule is enabled with specific parsers. Currently users will not see other erroneous cases in the same file due to linter crash.