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(experimental-utils): support immutable members #3844
Conversation
Thanks for the PR, @rafaelss95! 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. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
PS 1: I tried to change
to: export type Range = readonly [number, number]; but it didn't work as it actually needs to be mutated in places like:
So in this case I opted out for just typing the necessary functions as
|
Codecov Report
@@ Coverage Diff @@
## main #3844 +/- ##
==========================================
+ Coverage 93.33% 93.49% +0.15%
==========================================
Files 152 192 +40
Lines 8014 8672 +658
Branches 2568 2683 +115
==========================================
+ Hits 7480 8108 +628
- Misses 180 351 +171
+ Partials 354 213 -141
Flags with carried forward coverage won't be shown. Click here to find out more.
|
7625ee6
to
5b7c952
Compare
This PR was accidentally closed earlier and I had forgotten to reopen it. |
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.
One change - otherwise LGTM
Thanks for your work!
@@ -209,13 +209,13 @@ interface RuleContext< | |||
* the root of the AST and continuing through the direct parent of the current node. | |||
* This array does not include the currently-traversed node itself. | |||
*/ | |||
getAncestors(): TSESTree.Node[]; | |||
getAncestors(): readonly TSESTree.Node[]; |
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.
This would be a breaking change because it's not uncommon for people to do getAncestors().reverse()
Also - it isn't necessary because getAncestors
returns a new array each invocation:
https://github.com/eslint/eslint/blob/d867f8100737bb82742debee2b5dc853c5f07c91/lib/linter/linter.js#L789-L797
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 for this!
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.
the lint error is actually legit.
repro
Found this TS issue: microsoft/TypeScript#17002
It's a TS bug :(
Which I see you already found and mentioned above!!
❌ Deploy Preview for typescript-eslint failed. 🔨 Explore the source changes: 0606673 🔍 Inspect the deploy log: https://app.netlify.com/sites/typescript-eslint/deploys/61b996f7e97b010008cc27a8 |
This PR is basically to provide support for immutable in some other members/functions besides of #3830.