feat(eslint-plugin): [prefer-readonly-parameter-types] added an optional type allowlist

[marekdedic]

BREAKING CHANGE:
Changes arguments to a public API

PR Checklist

• Addresses an existing issue: fixes [prefer-readonly-parameter-types] Proposal: Add type allowlist #4185
• That issue was marked as accepting prs
• Steps in CONTRIBUTING.md were taken

Overview

I have added an option type whitelist to the rule prefer-readonly-parameter-types.

This PR is not yet ready: ✔️

• Check that the type is from the standard library
• Decide whether the whitelist really should be user-configurable
• Decide what to do with third-party types that cannot be made readonly, such as JQueryStatic (which leads to ReadonlyDeep does not work for objects with call signatures sindresorhus/type-fest#337)

[typescript-eslint]

Thanks for the PR, @marekdedic!

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.

[netlify]

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	3d465cc
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/64186a8bf4aabf00087aee72
😎 Deploy Preview	https://deploy-preview-4436--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[codecov]

Codecov Report

Merging #4436 (3d465cc) into v6 (bd273dd) will decrease coverage by 0.40%.
The diff coverage is 92.10%.

Additional details and impacted files

@@            Coverage Diff             @@
##               v6    #4436      +/-   ##
==========================================
- Coverage   85.64%   85.25%   -0.40%     
==========================================
  Files         372      373       +1     
  Lines       12675    12764      +89     
  Branches     3747     3791      +44     
==========================================
+ Hits        10856    10882      +26     
- Misses       1461     1501      +40     
- Partials      358      381      +23     

Flag	Coverage Δ
unittest	85.25% <92.10%> (-0.40%)	⬇️

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

Impacted Files	Coverage Δ
packages/type-utils/src/TypeOrValueSpecifier.ts	89.28% <89.28%> (ø)
...lugin/src/rules/prefer-readonly-parameter-types.ts	100.00% <100.00%> (ø)
packages/type-utils/src/isTypeReadonly.ts	87.03% <100.00%> (+1.03%)	⬆️

... and 6 files with indirect coverage changes

[RebeccaStevens]

With regard to complex types; we'd probably want to mix allowlist with the following part of #4429

typescript-eslint/packages/type-utils/src/isTypeReadonly.ts

Lines 202 to 213 in 491c7bb

	// Special case for handling arrays/tuples (as readonly arrays/tuples always have mutable methods).
	if (
	type.types.some(t => checker.isArrayType(t) || checker.isTupleType(t))
	) {
	const allReadonlyParts = type.types.every(
	t =>
	seenTypes.has(t) ||
	isTypeReadonlyRecurser(checker, t, options, seenTypes) ===
	Readonlyness.Readonly,
	);
	return allReadonlyParts ? Readonlyness.Readonly : Readonlyness.Mutable;
	}

[marekdedic]

Hi,
in the end, some types not from the standard lib need to be whitelisted - for example JQueryStatic from @types/jquery AFAIK cannot be made readonly due to sindresorhus/type-fest#337.

Originally, I wanted to be able to extend the feature of checking whether a type belongs to the standard lib to these types as well - so you'd have something like allowlist: [["HTMLElement", "DOM"], ["JQueryStatic", "@types/jquery"]]. However, after digging through the is-standard-lib-check code, I found no easy way to specify the "package" from where the type is (I gues you could check for declaration file name substring, but that seems brittle).

So I split the allowlist between internal, which checks stdlib, and user-configurable. Let me know what you think of this or if you see any better approach.

[marekdedic]

Hi,
I think the PR is basically ready, with some outstanding points I don't know how to resolve myself

• What to do with internal vs. external allowlist - see feat(eslint-plugin): [prefer-readonly-parameter-types] added an optional type allowlist #4436 (comment)
• I needed to add lib: dom to the test tsconfig.json, however, that broke one existing test. Could the lib be set per-test? I'd also like to test a custom type that happens to have the same name as an internal-whitelisted one (but isn't internal, so shouldn't actually be whitelisted)
• What should the internal whitelist look like? Currently, it's just ["HTMLElement"], do you know of any other types causing issues?
• There's something strange going on with the netlify deployment - I think I didn't break it, so 🤷

[JoshuaKGoldberg]

after digging through the is-standard-lib-check code, I found no easy way to specify the "package" from where the type is (I guess you could check for declaration file name substring, but that seems brittle).

Yup, that's... pretty much what I'd do 😄. Checking whether it's node_modules/package/ or node_modules/@types/package is the best I can think of. This is part of why we generally prefer not to include these kinds of allowlists; names such as Node can be re-used and quite ambiguous. Long term we may want to build off of this PR and set up a standard format for how rules can specify type locations...

internal whitelist look like

How about empty? We generally try not to put forward opinions like which internal types do or don't match up with specific rule options. I don't think we want "HTMLElement" or any others by default.

Netlify

Indeed, #4335 😢... it's better than it was, but still not fully fixed, on main.

[JoshuaKGoldberg]

Looks good so far, thanks!

[marekdedic]

after digging through the is-standard-lib-check code, I found no easy way to specify the "package" from where the type is (I guess you could check for declaration file name substring, but that seems brittle).

Yup, that's... pretty much what I'd do smile. Checking whether it's node_modules/package/ or node_modules/@types/package is the best I can think of.

I found a better way! We could use https://github.com/microsoft/TypeScript/blob/b9efc3b6141f06d926b4067d086ed5f03f221c49/src/compiler/types.ts#L4018-L4019 if it weren't internal... Let's merge this and improve it if I can get TS to make it public, would that be okay?

[marekdedic]

How about empty? We generally try not to put forward opinions like which internal types do or don't match up with specific rule options. I don't think we want "HTMLElement" or any others by default.

I don't think that should be the default. Given that certain types are basically impossible to pass in a readonly fashion, reporting for e.g. HTMLElement is almost always pointless - there is just no way around this and everyone who stumbles upon this can either add it to the whitelist or disable the rule for that line. The only way it's just almost pointless is that I can imagine you rewrite some functions to not take the full element as parameter, but something else (like its text contents or whatever...) See #4185 or #2699

I'm in favor of being conservative and not whitelisting anything where there's any chance of it being an actionable report, but I feel that for some types this is just not helpful.

[marekdedic]

🎉 Only took a year and a bit :D

Thanks for all the feedback and collaboration getting this ready and merged.

[JoshuaKGoldberg]

Yes! It took a while but the result is fantastic - thank you @marekdedic for working with us!