fix(parser): disallow invalid TS parameter property patterns #11753
Conversation
|
Thanks for the PR, @ronakmaheshwari! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
❌ Deploy Preview for typescript-eslint failed.
|
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run-many --target=build --parallel --exclude... |
❌ Failed | 11s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-11-11 14:35:01 UTC
| AST_NODE_TYPES.Identifier; | ||
|
|
||
| if (!isIdentifier && !isAssignmentToIdentifier) { | ||
| this.#throwError( |
There was a problem hiding this comment.
Can this part move to check-modifiers.ts?
There was a problem hiding this comment.
We cant change the project structure and the function is used any many places
There was a problem hiding this comment.
fisker is right, I think. This newly added logic duplicates what's already in convertParameters. It'd be better to add these new modifier checks to the existing area of code that checks modifiers.
|
@ronakmaheshwari tip, you'll want to fill out PR descriptions before requesting review. https://typescript-eslint.io/contributing/pull-requests#raising-the-pr has our PR contributing process (but this is really a universal GitHub practice). Thanks for sending 🙂 |
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
This change has some build failures. Go ahead and take a look at the logs to see why it's failing. Cheers!
|
I will be fixing it |
|
I wounder if the build failure same as #11593 |
|
@fisker I feel it's the entire codebase. It's very finicky to start development |
Maybe something around that obfuscated the issue, but I don't think this PR is incorrectly failing. In https://github.com/typescript-eslint/typescript-eslint/pull/11753/files#diff-341d9ffb40c62139c7da69b49d5081cbce7eabce79b58fe569de987a91e63c14R1482,
Yeah 😞. We have found it it difficult to manage a codebase that has so many cross-dependencies by nature (ESLint, TypeScript, multiple ASTs, multiple types of packages, etc.)... |
|
Oh, shoot. I just noticed #11760: a PR that tackles the same backing issue, #11708. #11760 was sent a few days after this PR, but because this PR wasn't properly linked to #11708, it wouldn't have been straightforward for @dbarabashh to see that a PR already existed for that issue. This is a big part of why we have the task list and issue linking in the PR template 🙃. #11760 actually does apply the change suggested in #11753 (comment), putting validation in This puts us in a hard spot. I don't want to close either PR because both have good work put into them. This one came first and you're actively working with us. But that one didn't do anything wrong - and couldn't have been expected to know it was a second PR for the issue. Blugh. |
|
#11760 was merged, so closing this out. @ronakmaheshwari we appreciate you sending this PR (thanks again!). If you want to keep contributing, we'd love that - every little bit helps. Just please be sure to respect the contributing guide & templates so that we don't get into a race condition again. ❤️ |
…{bar}, ...baz)
PR Checklist
Overview