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

docs: update with TypeScript info #17423

Merged
merged 5 commits into from Jul 28, 2023
Merged

docs: update with TypeScript info #17423

merged 5 commits into from Jul 28, 2023

Conversation

Zamiell
Copy link
Contributor

@Zamiell Zamiell commented Jul 27, 2023

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[X] Documentation update

What changes did you make? (Give an overview)

I added TypeScript information to the docs.

Is there anything you'd like reviewers to focus on?

No.

Motivation

The no-dupe-class-members rule helpfully documents that if you are using TypeScript, then you don't have to enable the rule. This is fantastic! However, there are many other ESLint rules that are also pointless if you are using TypeScript, but they are not documented. So this is the first step in solving that problem.

Other Discussion

I want to document all rules that are covered by the TypeScript compiler, and this PR is the first step in doing so.
In my next PR I would like to reduce the text duplication that I am introducing with this PR by using some kind of auto-filled template. I envision something like:

{{ TypeScript 2377 }}

Which would output the following text during the actual website build:

It is safe to disable this rule when using TypeScript because [TypeScript's compiler enforces this check (`ts(2377)`)](https://github.com/Microsoft/TypeScript/blob/main/src/compiler/diagnosticMessages.json).

I don't know if the docs already contains anything like this, so I would like a contributor to work with me to find the cleanest way to accomplish this.

Furthermore, I also want to include a TypeScript icon next to these kinds of texts to make it easier to see at a glance that the rule is covered already by TypeScript, which is another idea for a future PR.

@Zamiell Zamiell requested a review from a team as a code owner July 27, 2023 03:39
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 27, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@eslint-github-bot eslint-github-bot bot added the documentation Relates to ESLint's documentation label Jul 27, 2023
@netlify
Copy link

netlify bot commented Jul 27, 2023

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 9dbeae6
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/64c2a7ee7a5cd80008f89a64
😎 Deploy Preview https://deploy-preview-17423--docs-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. I don't think linking to the JSON file is helpful and for all we know that location could change.

Is there some documentation you could link to instead?

@Zamiell
Copy link
Contributor Author

Zamiell commented Jul 27, 2023

Hi nzakas,

I don't know if there is generic documentation for TypeScript error types. I googlged it and found this StackOverflow page that just links to the JSON file.

For now, I'll just edit the PR and remove the link.

Can you respond to the topics that I raised in the "Other Discussion" section?

@mdjermanovic
Copy link
Member

However, there are many other ESLint rules that are also pointless if you are using TypeScript, but they are not documented.

I think the typescript-eslint repository would be better place to maintain this list and maybe even provide a configuration that disables these rules. There's already a configuration that disables eslint:recommended rules that are redundant with TypeScript checks:

https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/src/configs/eslint-recommended.ts

@Zamiell
Copy link
Contributor Author

Zamiell commented Jul 27, 2023

@mdjermanovic I think that they have already decided against this; see this issue.
Furthermore, typescript-eslint doesn't want to maintain documentation for extended rules (meaning the core ESLint rules that they tack on logic to), let alone non-extended rules.
Given that the ESLint docs are already documenting some rules that conflict with TypeScript, I would like to continue that trend here.

@mdjermanovic
Copy link
Member

I think that they have already decided against this; see this issue.

Fair, but after reading typescript-eslint/typescript-eslint#6449 (comment), I'd agree with their suggestion that this should be a separate project as I'm not sure we could afford following versions of TypeScript and testing whether the rules we mark as safe to disable when using TypeScript are indeed completely covered by TypeScript.

@Zamiell
Copy link
Contributor Author

Zamiell commented Jul 28, 2023

@mdjermanovic I appreciate the concern about wanting to have 100% test coverage, but I don't think this has to be all that complicated. It is technically possible that some future TypeScript version turns out to make the type checker less strict, which would subsequently make my PR here outdated... but that seems pretty darn unlikely! Hopefully we can agree on that.

I agree that ESLint should not have to bother itself with specifying the exact TypeScript versions that a check applies to. It's enough to just say that TypeScript covers it, and we can expect the reader do more research themselves if they really need to. I think the Pareto principle applies here: even if the passage on TypeScript in an ESLint rule page is not super in-depth, it is still a great starting point to alert ESLint users of a potential problem.

Now, on the topic of putting the documentation somewhere else than the ESLint rule pages: I think that sucks for end-users. Consider the case of someone reading the ESLint rule docs, going over every rule to decide which ones they should turn on in their codebase. Naturally, the kind of person who is scouting out for good ESLint rules in order to keep their codebase error-free is exactly the kind of person who is already going to be using TypeScript. The state of affairs right now is that some of the ESLint rules mention TypeScript incompatibility properly. And others do not. This makes the "scouting" experience for this user a real pain. And even in a hypothetical where the "scouting" user has a solid 3rd party resource that contains core ESLint rule incompatibility issues with TypeScript (e.g. eslint-config-typescript), the experience for the user is still bad! Because now they are pulled away from the ESLint docs and have to constantly cross-reference every rule with the 3rd party list as they go through and browse the menu.

Counterargument: Doesn't this create more of a documentation maintenance burden on the ESLint core team? Will ESLint core team be blamed if the documentation isn't perfect? Well, I would argue no. There are only a few core rules that are known to be currently covered by TypeScript, and getting those documented is a pretty small undertaking. Beyond that, I would argue that the ESLint core team is not responsible for keeping a master list of TypeScript incompatibility; rather, the existing incompatibility documentation is just a community-based best-effort. Again, I feel that the Pareto principle strongly applies here: any small mention of TypeScript incompatibility on the official ESLint rule pages will go a very long way.

@nzakas
Copy link
Member

nzakas commented Jul 28, 2023

I'd say let's open a separate issue to discuss whether or not we want to track tsc settings. That's a much broader discussion than is appropriate for this small PR.

@nzakas nzakas merged commit 4d474e3 into eslint:main Jul 28, 2023
21 checks passed
@Zamiell Zamiell mentioned this pull request Aug 25, 2023
1 task
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jan 25, 2024
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jan 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants