feat(eslint-plugin): [array-type] detect `Readonly<string[]>` case #8752

developer-bandi · 2024-03-22T12:53:34Z

PR Checklist

Addresses an existing open issue: fixes Enhancement: [array-type] Detect Readonly<string[]> #7755
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

In the case of Readonly<string[]>, the same as Readonly Array<string>, an error is displayed and then changed to readonly string[].

However, on the contrary, readonly string[] is converted to ReadonlyArray<string> as before.

typescript-eslint · 2024-03-22T12:53:37Z

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.

netlify · 2024-03-22T12:53:50Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`ae91e94`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/663f855470b49f00088ccfb2
😎 Deploy Preview	https://deploy-preview-8752--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 99 (🟢 up 3 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

nx-cloud · 2024-03-22T12:54:47Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit ae91e94. 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

🟥 Failed Commands
`nx test eslint-plugin --coverage=false`
`nx run-many --target=lint --parallel --exclude eslint-plugin`

✅ Successfully ran 28 targets

Sent with 💌 from NxCloud.

codecov · 2024-03-22T13:16:43Z

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 87.03%. Comparing base (f248e68) to head (76d9f36).
Report is 7 commits behind head on main.

❗ Current head 76d9f36 differs from pull request most recent head ae91e94. Consider uploading reports for the commit ae91e94 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8752      +/-   ##
==========================================
- Coverage   87.40%   87.03%   -0.37%     
==========================================
  Files         260      254       -6     
  Lines       12608    12492     -116     
  Branches     3937     3923      -14     
==========================================
- Hits        11020    10873     -147     
- Misses       1313     1335      +22     
- Partials      275      284       +9

Flag	Coverage Δ
unittest	`87.03% <50.00%> (-0.37%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
packages/eslint-plugin/src/rules/array-type.ts	`36.61% <50.00%> (-61.91%)`	⬇️

... and 42 files with indirect coverage changes

packages/eslint-plugin/tests/rules/array-type.test.ts

JoshuaKGoldberg

I'll defer to @auvred as they already started reviewing. Just fly-by leaving a couple of requests. Cheers!

JoshuaKGoldberg · 2024-04-08T17:31:43Z

packages/eslint-plugin/docs/rules/array-type.mdx

+Always use `Array<T>` or `ReadonlyArray<T>` or `Readonly<T[]>` for all array types.
+However, `readonly T[]` is modified to `ReadonlyArray<T>`.


[Docs] A bit of clarity ("however" implies contrast with the previous statement)

Suggested change

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>`.

JoshuaKGoldberg · 2024-04-23T20:45:52Z

packages/eslint-plugin/tests/rules/array-type.test.ts

-function fooFunction(foo: Array<ArrayClass<string>>) {
-  return foo.map(e => e.foo);
-}
-      `,


[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.

auvred · 2024-05-01T07:46:05Z

packages/eslint-plugin/src/rules/array-type.ts

+        const isReadonlyWithGenericArrayType =
+          node.typeName.name === 'Readonly' &&
+          node.typeArguments?.params[0].type === AST_NODE_TYPES.TSArrayType;


[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'

playground

auvred · 2024-05-01T07:46:09Z

packages/eslint-plugin/tests/rules/array-type.test.ts

          line: 3,
-          column: 8,
+          column: 12,
        },


[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

developer-bandi · 2024-05-03T14:32:13Z

Some of the current CI test cases are failing.

Since it is not failing locally, could you please help me resolve it?

kirkwaiblinger · 2024-05-08T19:27:15Z

packages/eslint-plugin/docs/rules/array-type.mdx

@@ -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'];


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!

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?

developer-bandi added 3 commits March 21, 2024 00:17

feat: add new type

1864644

fix: test case

18fff29

test: add test case

2471ff2

docs: add ReadonlyArray<T> description

2ce7956

test: add droped test case

76d9f36

auvred requested changes Apr 6, 2024

View reviewed changes

packages/eslint-plugin/tests/rules/array-type.test.ts Show resolved Hide resolved

auvred added the awaiting response Issues waiting for a reply from the OP or another party label Apr 6, 2024

fix: revert comment line and fix error position

dbcd788

developer-bandi requested a review from auvred April 7, 2024 03:31

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Apr 7, 2024

JoshuaKGoldberg requested changes Apr 23, 2024

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Apr 23, 2024

developer-bandi added 2 commits April 28, 2024 16:11

fix: fix indent, docs description

8a361fe

fix: revert indent

1a59bd0

developer-bandi requested a review from JoshuaKGoldberg April 28, 2024 07:32

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Apr 28, 2024

auvred requested changes May 1, 2024

View reviewed changes

auvred added the awaiting response Issues waiting for a reply from the OP or another party label May 1, 2024

developer-bandi added 2 commits May 3, 2024 22:51

fix: add exception and delete not related PR

8fc0b82

Merge branch 'main' of https://github.com/developer-bandi/typescript-…

7482f0d

…eslint into feature/array-type-allow-readonly

developer-bandi requested a review from auvred May 3, 2024 14:18

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label May 3, 2024

kirkwaiblinger reviewed May 8, 2024

View reviewed changes

kirkwaiblinger added the awaiting response Issues waiting for a reply from the OP or another party label May 8, 2024

docs: move no correct example to incorrect example

f3e3cff

docs: fix example document

ae91e94

developer-bandi requested a review from kirkwaiblinger May 11, 2024 14:51

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label May 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): [array-type] detect `Readonly<string[]>` case #8752

feat(eslint-plugin): [array-type] detect `Readonly<string[]>` case #8752

developer-bandi commented Mar 22, 2024

typescript-eslint bot commented Mar 22, 2024

netlify bot commented Mar 22, 2024 •

edited

nx-cloud bot commented Mar 22, 2024 •

edited

codecov bot commented Mar 22, 2024 •

edited

JoshuaKGoldberg left a comment

JoshuaKGoldberg Apr 8, 2024

JoshuaKGoldberg Apr 23, 2024

auvred May 1, 2024

auvred May 1, 2024

developer-bandi commented May 3, 2024

kirkwaiblinger May 8, 2024

developer-bandi May 11, 2024

		Always use `Array<T>` or `ReadonlyArray<T>` or `Readonly<T[]>` for all array types.
		However, `readonly T[]` is modified to `ReadonlyArray<T>`.

feat(eslint-plugin): [array-type] detect Readonly<string[]> case #8752

Are you sure you want to change the base?

feat(eslint-plugin): [array-type] detect Readonly<string[]> case #8752

Conversation

developer-bandi commented Mar 22, 2024

PR Checklist

Overview

typescript-eslint bot commented Mar 22, 2024

netlify bot commented Mar 22, 2024 • edited

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Mar 22, 2024 • edited

☁️ Nx Cloud Report

codecov bot commented Mar 22, 2024 • edited

Codecov Report

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Apr 8, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg Apr 23, 2024

Choose a reason for hiding this comment

auvred May 1, 2024

Choose a reason for hiding this comment

auvred May 1, 2024

Choose a reason for hiding this comment

developer-bandi commented May 3, 2024

kirkwaiblinger May 8, 2024

Choose a reason for hiding this comment

developer-bandi May 11, 2024

Choose a reason for hiding this comment

feat(eslint-plugin): [array-type] detect `Readonly<string[]>` case #8752

feat(eslint-plugin): [array-type] detect `Readonly<string[]>` case #8752

netlify bot commented Mar 22, 2024 •

edited

nx-cloud bot commented Mar 22, 2024 •

edited

codecov bot commented Mar 22, 2024 •

edited