Skip to content
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

Enhancement: [array-type] Detect Readonly<string[]> #7755

Open
4 tasks done
neelance opened this issue Oct 16, 2023 · 9 comments · May be fixed by #8752
Open
4 tasks done

Enhancement: [array-type] Detect Readonly<string[]> #7755

neelance opened this issue Oct 16, 2023 · 9 comments · May be fixed by #8752
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@neelance
Copy link

Before You File a Proposal Please Confirm You Have Done The Following...

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Link to the rule's documentation

https://typescript-eslint.io/rules/array-type

Description

The rule should also turn Readonly<string[]> into readonly string[].

Fail

const y: Readonly<string[]> = ['a', 'b'];

Pass

const y: readonly string[] = ['a', 'b'];

Additional Info

No response

@neelance neelance added enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Oct 16, 2023
@bradzacher
Copy link
Member

Is this a common pattern you've seen around your codebase?

@neelance
Copy link
Author

Yes, we had a few of them. This is because we have another linter rule that enforces immutability on certain types. I guess this is why people started adding Readonly<...> also to array types.

I have now used no-restricted-syntax to detect this pattern. Still, it seems to me like it would make sense to add this to array-type. I was a bit surprised that it wasn't included.

@JoshuaKGoldberg
Copy link
Member

Enforcing Readonly<...> vs. readonly ... is covered by https://github.com/eslint-functional/eslint-plugin-functional/blob/e4bd4ad79502ff77ae26e703e1761ab80a46868b/docs/rules/prefer-immutable-types.md and https://github.com/eslint-functional/eslint-plugin-functional/blob/e4bd4ad79502ff77ae26e703e1761ab80a46868b/docs/rules/type-declaration-immutability.md (previously, https://github.com/eslint-functional/eslint-plugin-functional/blob/e4bd4ad79502ff77ae26e703e1761ab80a46868b/docs/rules/immutable-data.md). That's a separate concern from the Array type styles.

If folks have already started adding Readonly<...> to types and are enforcing immutability on certain types, then I bet https://github.com/eslint-functional/eslint-plugin-functional and/or https://github.com/jhusain/eslint-plugin-immutable are plugins you all might find useful (if you haven't already started using them).

Thanks for filing!

@JoshuaKGoldberg JoshuaKGoldberg closed this as not planned Won't fix, can't repro, duplicate, stale Oct 19, 2023
@JoshuaKGoldberg JoshuaKGoldberg added wontfix This will not be worked on and removed triage Waiting for maintainers to take a look labels Oct 19, 2023
@Josh-Cena
Copy link
Member

@JoshuaKGoldberg To clarify: this issue asks for the rule to check that readonly array types are declared consistently, not that they are marked as readonly. Do those rules cover this use case?

@JoshuaKGoldberg
Copy link
Member

Yeah I think this rule shouldn't cover that case because Readonly<...> isn't specific to arrays, and users often end up writing their own equivalents (ReadonlyDeep, etc.). Trying to expand this rule's purview to the assorted wrapper types folks use would make its area of responsibilities more than just array types.

@Josh-Cena
Copy link
Member

This rule already checks between readonly T[] and ReadonlyArray<T>. It's not unreasonable to me to expect Readonly<T[]> to work. But maybe this would be confusing to some users, I don't know.

@JoshuaKGoldberg
Copy link
Member

I think the difference (for me at least) is that ReadonlyArray is a built-in dedicated array type, whereas Readonly is general.

@bradzacher
Copy link
Member

I would say that we can easily include this case because it's syntactically simple to detect the case of Readonly<T[]> - the developer intent was clear - they wanted a readonly array and there are true constructs for that - so we can improve codebase consistency by reporting and suggesting the configured style instead.

I'm in favour of this, personally!

@JoshuaKGoldberg
Copy link
Member

Well, I guess I'm being outvoted on this one 😄

@JoshuaKGoldberg JoshuaKGoldberg added accepting prs Go ahead, send a pull request that resolves this issue and removed wontfix This will not be worked on labels Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants