feat(eslint-plugin): [array-type] detect Readonly<string[]> case
#8752
Conversation
|
Thanks for the PR, @developer-bandi! 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 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 1535f1f. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8752 +/- ##
=========================================
+ Coverage 0 87.03% +87.03%
=========================================
Files 0 254 +254
Lines 0 12492 +12492
Branches 0 3923 +3923
=========================================
+ Hits 0 10873 +10873
- Misses 0 1335 +1335
- Partials 0 284 +284
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
I'll defer to @auvred as they already started reviewing. Just fly-by leaving a couple of requests. Cheers!
| Always use `Array<T>` or `ReadonlyArray<T>` or `Readonly<T[]>` for all array types. | ||
| However, `readonly T[]` is modified to `ReadonlyArray<T>`. |
There was a problem hiding this comment.
[Docs] A bit of clarity ("however" implies contrast with the previous statement)
| Always use `Array<T>` or `ReadonlyArray<T>` or `Readonly<T[]>` for all array types. | |
| However, `readonly T[]` is modified to `ReadonlyArray<T>`. | |
| Always use `Array<T>`, `ReadonlyArray<T>`, or `Readonly<T[]>` for all array types. | |
| `readonly T[]` will be modified to `ReadonlyArray<T>`. |
| function fooFunction(foo: Array<ArrayClass<string>>) { | ||
| return foo.map(e => e.foo); | ||
| } | ||
| `, |
There was a problem hiding this comment.
[Style] Sorry if I missed this before - could you please revert any unrelated changes, such as differences in indentation levels? It's harder to review the PR when there's a lot of irrelevant noise in the diff.
| const isReadonlyWithGenericArrayType = | ||
| node.typeName.name === 'Readonly' && | ||
| node.typeArguments?.params[0].type === AST_NODE_TYPES.TSArrayType; |
There was a problem hiding this comment.
[Bug] If the type name is Readonly and first type argument is not an array, we should return from the function. Because otherwise any Readonly<...> type will be reported.
const x: Readonly<string> = 'a'| line: 3, | ||
| column: 8, | ||
| column: 12, | ||
| }, |
There was a problem hiding this comment.
[Tests] Could you revert these changed columns? These changes don't seem to be related to this PR.
…eslint into feature/array-type-allow-readonly
|
Some of the current CI test cases are failing. Since it is not failing locally, could you please help me resolve it? |
| @@ -58,6 +59,7 @@ const y: readonly string[] = ['a', 'b']; | |||
| ```ts option='{ "default": "generic" }' | |||
| const x: Array<string> = ['a', 'b']; | |||
| const y: ReadonlyArray<string> = ['a', 'b']; | |||
| const z: Readonly<string[]> = ['a', 'b']; | |||
There was a problem hiding this comment.
re #8752 (comment)
(One of) the CI failure(s) is due to this line being flagged, despite being in the "Correct" tab. (you can try it for yourself in the deploy preview by clicking the "Open in playground" button)
FAIL tests/docs.test.ts (25.146 s, 827 MB heap size)
● Validating rule docs › array-type.mdx › code examples ESLint output
assert(received)
Expected value to be equal to:
true
Received:
false
Message:
Expected not to contain lint errors:
const x: Array<string> = ['a', 'b'];
const y: ReadonlyArray<string> = ['a', 'b'];
const z: Readonly<string[]> = ['a', 'b'];
Shoutout to @auvred's for his work in #8382 / https://github.com/typescript-eslint/typescript-eslint/pull/8497/files#diff-62842d0740c234c02c26a4ccd3b3fe920b872cff1479f0703c6ec44f9cfb2f3dR407 that caught this!
There was a problem hiding this comment.
thank you for your advise!
but there is one problem. i use incorrect exampleconst z: Readonly<string[]> = ['a', 'b']; but lint cannot use this type. Is there no way?
There was a problem hiding this comment.
Hi, sorry for the slow response.
I'm not really familiar with this change, and, I apologize, but I'm having a bit of a hard time understanding what you mean in the question...
I'm wondering if the issue is that, now that the snapshot has been set, the snapshot tests are failing due to the examples being changed? If so, you may just need to run cd eslint-plugin; yarn test schemas -u (or similar); see #8997.
Let me know if that resolves your issue or if the problem is something else! 🙂
There was a problem hiding this comment.
BTW - looking closer at the screenshot, i think you might also need to run yarn lint-fix to fix the existing violations throughout the repo of the new cases that this rule reports.
There was a problem hiding this comment.
The problem was that I had to apply the changed lint rule(array-type) to another file. Thank you!
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
This will sadly false-negative on a case like this:
type T = string[];
type U = Readonly<T>;However that seems like it is okay for two reasons:
- you can't convert
Readonly<T>toreadonly T- you would need to doreadonly T[number][]which is just ew. - we would need type info to even begin to catch it - defs not worth adding that to the rule.
This LGTM - thanks for adding this!
This has been a pet peeve of mine for a while.
9dba021
into
typescript-eslint:main
PR Checklist
Readonly<string[]>#7755Overview
In the case of
Readonly<string[]>, the same asReadonly Array<string>, an error is displayed and then changed toreadonly string[].However, on the contrary,
readonly string[]is converted toReadonlyArray<string>as before.