Adds FAQs with a comparison table explaining where they differ or are the same. I intentionally put the same number of ✅s for each side in the comparison table to try to convey that they're both fine to use, as long as a codebase remains consistent.

💖

docs(eslint-plugin): [consistent-type-definitions] add FAQs

d432914

Copy link

Contributor

typescript-eslintbot commentedJan 28, 2025

Thanks for the PR,@JoshuaKGoldberg!

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.

Copy link

netlifybot commentedJan 28, 2025•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`b307a44`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/67b5c4e032718e00089ed918
😎 Deploy Preview	https://deploy-preview-10731--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 26 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 yourNetlify site configuration.

Copy link

nx-cloudbot commentedJan 28, 2025•
edited
Loading

View yourCI Pipeline Execution ↗ for commitb307a44.

Command	Status	Duration	Result
`nx test eslint-plugin --coverage=false`	✅ Succeeded	7m 25s	View ↗
`nx test eslint-plugin`	✅ Succeeded	5m 58s	View ↗
`nx test visitor-keys --coverage=false`	✅ Succeeded	<1s	View ↗
`nx test utils --coverage=false`	✅ Succeeded	<1s	View ↗
`nx run types:build`	✅ Succeeded	<1s	View ↗
`nx test typescript-eslint --coverage=false`	✅ Succeeded	9s	View ↗
`nx test type-utils --coverage=false`	✅ Succeeded	<1s	View ↗
`nx run-many --target=build --exclude website --...`	✅ Succeeded	29s	View ↗
`Additional runs (24)`	✅ Succeeded	...	View ↗

☁️Nx Cloud last updated this comment at2025-02-19 12:02:43 UTC

kirkwaiblinger reviewed

Jan 28, 2025

View reviewed changes

packages/eslint-plugin/docs/rules/consistent-type-definitions.mdxShow resolvedHide resolved

kirkwaiblinger reviewed

Jan 28, 2025

View reviewed changes

packages/eslint-plugin/docs/rules/consistent-type-definitions.mdxShow resolvedHide resolved

kirkwaiblinger reviewed

Jan 28, 2025

View reviewed changes

packages/eslint-plugin/docs/rules/consistent-type-definitions.mdx

		Almost never.
		Most TypeScript projects do not -and should not- utilize types that exercise the performance differences between the two kinds of definitions.

		If you are having problems with type checking performance, see the [TypeScript Wiki's Performance page](https://github.com/microsoft/TypeScript/wiki/Performance).

Copy link

Member

kirkwaiblingerJan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is an incredibly useful page that I have never seen before 👌

kirkwaiblinger reviewed

Jan 28, 2025

View reviewed changes

packages/eslint-plugin/docs/rules/consistent-type-definitions.mdx OutdatedShow resolvedHide resolved

kirkwaiblinger reviewed

Jan 28, 2025

View reviewed changes

packages/eslint-plugin/docs/rules/consistent-type-definitions.mdx OutdatedShow resolvedHide resolved

kirkwaiblinger added the awaiting responseIssues waiting for a reply from the OP or another party label

Jan 28, 2025

fix link; make list for Record note

8d81a4c

JoshuaKGoldberg requested a review fromkirkwaiblinger

January 29, 2025 20:38

JoshuaKGoldberg removed the awaiting responseIssues waiting for a reply from the OP or another party label

Jan 29, 2025

Copy link

codecovbot commentedJan 29, 2025•
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.30%. Comparing base(94e8098) to head(b307a44).
Report is 64 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@##             main   #10731      +/-   ##==========================================+ Coverage   87.26%   87.30%   +0.03%==========================================  Files         450      450                Lines       15712    15758      +46       Branches     4600     4614      +14     ==========================================+ Hits        13711    13757      +46  Misses       1645     1645                Partials      356      356

Flag	Coverage Δ
unittest	`87.30% <ø> (+0.03%)`	⬆️

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

see 21 files with indirect coverage changes

JoshuaKGoldberg mentioned this pull request

Jan 30, 2025

Bug: [no-misused-spread] report for string spread harmfully conflates Unicode code points with UTF-16 code units#10745

Closed

4 tasks

kirkwaiblinger reviewed

Feb 2, 2025

View reviewed changes

packages/eslint-plugin/docs/rules/consistent-type-definitions.mdx Outdated

		For example:

		- If your project is a dependency of another project that relies on a specific type definition style
		- `Record<string, ...>` and `{ [i: string]: ... }` have subtle behavior differences

Copy link

Member

kirkwaiblingerFeb 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

(coming from#10731 (comment)) Maybe this is just me not understanding, but I still don't understand how mentioning Records and index signatures fits into the flow of this document at all lol 🙃.

There is no prior mention ofRecord anywhere on the page, and so it's not clear to me at all as a reader how theRecord-vs-index-signature really relates totypes andinterfaces. Especially sincetype Foo = Record<string, ...> andtype Foo = { [i: string]: ... } arebothtypes.

Even with the additional knowledge thatRecord is an alias for a mapped type, and index signatures may be specified on "object types" or on "interfaces", knowledge which I don't think we can assume on the user's part, I still genuinely don't follow.

Can this mention just be removed? Or can sufficient context be provided in order to make its intended meaning clear?

Copy link

Member

kirkwaiblingerFeb 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If anything it feels like a caveat that was intended to be put inconsistent-indexed-object-style, which currently does not haveany such caveat mentioning that Record and index signatures are not always identical???

Copy link

MemberAuthor

JoshuaKGoldbergFeb 17, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

(oops sorry missed this) yeah good point. Agreed this belongs more in the other rule, will remove for now. I think mentioning in the other rule's docs should be a followup. I'm having a hard time figuring out how to phrase the difference in the context of the dedicated rule...