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

Clarify relation to the typescript package #2051

Closed
tuukka opened this issue May 20, 2020 · 5 comments
Closed

Clarify relation to the typescript package #2051

tuukka opened this issue May 20, 2020 · 5 comments
Labels
awaiting response Issues waiting for a reply from the OP or another party package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin question Questions! (i.e. not a bug / enhancment / documentation)

Comments

@tuukka
Copy link

tuukka commented May 20, 2020

Something of a continuation to #2043 in understanding and documenting more of @typescript-eslint's relation to typescript:

In package.json, typescript is marked as an optional peer dependency, but it seems to be obligatory as tsutils fails to load without it. (Worth a comment in README?)

Further, if I use the "plugin:@typescript-eslint/recommended-requiring-type-checking" rules, who does the type checking? I would assume typescript, but it's not clear to me. (Worth a comment in README?)

Even further, would it be possible to create a new rule that would report type checking errors? Currently, the separate tsc --noEmit must be duplicating some if not all type checking work. (Worth a comment in FAQ?)

Repro

module.exports = {
  plugins: [
    "@typescript-eslint",
  ],
};
$ yarn eslint
yarn run v1.22.4
$ eslint --ext .js,.jsx,.ts,.tsx ./
Error: Failed to load plugin '@typescript-eslint' declared in '.eslintrc.js': Cannot find module 'typescript'
Referenced from: .eslintrc.js
    at Object.<anonymous> (node_modules/tsutils/typeguard/2.8/node.js:3:12)
error Command failed with exit code 2.

Versions

package version
@typescript-eslint/eslint-plugin 2.29.0
@typescript-eslint/parser 2.29.0
ESLint 6.8.0
node 10.19.0
@tuukka tuukka added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels May 20, 2020
@bradzacher
Copy link
Member

Does the explanation in our readme not do a good enough job here?

https://github.com/typescript-eslint/typescript-eslint#how-does-typescript-eslint-work-and-why-do-you-have-multiple-packages

typescript is marked as an optional peer dependency

https://github.com/typescript-eslint/typescript-eslint#supported-typescript-version

It is on purpose we don't want to install an extra version of TS accidentally (multiple versions of TS cause weird hard to reproduce errors).

tsutils uses the same peer dep strategy, though they explicitly define their version ranges there.

We have an optional peer dependency because we don't want packages and tools (such as create-react-app) that optionally use our tooling to throw warnings to users about TS versions.

We leverage runtime validation of the TS version instead of install time validation for a few reasons:

  • we release weekly, so things can change without a lot of ceremony
  • a lot of users flat out ignore the warnings output during an install
  • our userbase is usually less savvy than that of tsutils, so we want to be much louder to help stop people logging issues due to their incorrect versions.
  • for the above create-react-app case, it means we can warn users at a more relevant time (if they actually use the package).

I would assume typescript, but it's not clear to me.

OOC - as a consumer of the tooling, why does this matter to you?
If we have our own tooling built to analyse types, or if we leverage TS, does it matter to you, the end user, as long as it works and provides the correct results?

Even further, would it be possible to create a new rule that would report type checking errors?

No, for a few reasons.

First, we don't want people to conceptually confuse linting and type checking.
They're similar concepts, but they serve separate purposes.

Primarily TS's typechecking is about the correctness of your application - the things it reports should indicate incorrect types that will break at runtime.
OTOH Linting is a mixture of things like stylistic checking, logical validation, and best-practice anaylsis.

Second, it would be very slow doing it in a lint rule. We use different compiler APIs to that of tsc, so we get things in a different form. The compiler APIs we use are all very lazy, and only do the minimum amount of work required. This is why turning one of our typed lint rules can add a decent runtime to your lint run - because that lint rule will request type information that hasn't been accessed yet, causing it to be calculated.

OTOH, tsc is built and optimised for the explicit use case of checking and compiling your project.

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party question Questions! (i.e. not a bug / enhancment / documentation) and removed triage Waiting for maintainers to take a look labels May 20, 2020
@tuukka
Copy link
Author

tuukka commented May 23, 2020

Thank you for the quick yet thorough reply to my questions!

Does the explanation in our readme not do a good enough job here?

I gave the documentation another read through trying to analyse why it wasn't clear to me. One issue might be that TypeScript the language and TypeScript the Compiler are not the same anymore in a project using Create-React-App / Babel. Type checking and type inference are not explicitly mentioned in the main README or the typed linting page. Then there was talk of a custom parser and the typescript dependency was marked as optional, so I had to remove typescript to make sure that it really is required but I still wouldn't know if it's responsible for the type inferencing.

I tried to clarify these in my pull request (#2081).

I would assume typescript, but it's not clear to me.

OOC - as a consumer of the tooling, why does this matter to you?

Ideally, as a user, I wouldn't need to understand these differences, but they can cause confusion when everything doesn't work as expected. More ideally, the users would understand these differences so they would have a better grasp of their tooling and could better contribute in the community.

We leverage runtime validation of the TS version instead of install time validation for a few reasons:

These reasons make perfect sense, but at first I would expect that installing a package would pull in everything necessary, or at least the getting started documentation would tell me how to get there. In #2081, I add typescript to the yarn add examples - I hope it makes sense as a clarification.

Even further, would it be possible to create a new rule that would report type checking errors?

No, for a few reasons.

If I understand your response right, it might be possible but not desirable?

First, we don't want people to conceptually confuse linting and type checking.
They're similar concepts, but they serve separate purposes.

Isn't the README making the opposite case though, presenting ESLint and TSC both as (complementary) static analysis tools on top of JavaScript?

Traditionally, type checking would be something that the compiler does - unfortunately, Babel doesn't. So we are left with separately running TSC, which is not something all tooling is prepared for. Then, having a rule "type check should pass" included in ESLint would be practical to fill in for Babel and also for pre-commit hooks etc.

Second, it would be very slow doing it in a lint rule. We use different compiler APIs to that of tsc, so we get things in a different form. The compiler APIs we use are all very lazy, and only do the minimum amount of work required. This is why turning one of our typed lint rules can add a decent runtime to your lint run - because that lint rule will request type information that hasn't been accessed yet, causing it to be calculated.

I would be very interested in testing and seeing the numbers, especially now that I've learned that the TSC parsing will run twice as well. (tsc --noEmit --diagnostics says parsing is slower than type check here.) I tried to have a look at the documentation of the TypeScript compiler to understand more about the type checking process, its architecture and laziness, but unfortunately there wasn't much.

BTW, I noticed this might need clarification wrt laziness on the typed linting page: "by including parserOptions.project in your config, you are essentially asking TypeScript to do a build of your project before ESLint can do its linting"

OTOH, tsc is built and optimised for the explicit use case of checking and compiling your project.

In a Babel project, the CI/CD looks something like this:

  1. ESLint runs a parse (and some type checking).
  2. "tsc --noEmit" runs a parse and all type checking.
  3. Babel runs a parse and emits.

It still looks to me like the easiest way to simplify and speed this up would be to add an ESLint rule that would make sure all types would be checked, and then step 2 could be skipped. Anyway, I think this could warrant an FAQ entry because I'm not the first one to be puzzled and asking about this.

On the long run, if all these tools would work as plugins on the same AST (ESTree?) like Prettier is available as an ESLint plugin now, one parse could be enough and there could be an incremental watch mode too?

@ghost
Copy link

ghost commented Jun 18, 2020

For a fresh perspective from someone new to TypeScript but has prior ESLint experience I took a peek at the docs and quickly started getting dizzy. There's just so much in here getting set-up I'd wager is going to be copypasta for most unless they're really procrastinating. I took a barebones Gatsby site I created today from scratch with only 4 deps, added one React component and TypeScript then tried this setup probably now outdated setup approach and...

Running lint takes 6 seconds with one component in my project. Is that normal? Also, I also stumbled on the typescript not included issue. If it's required to bootstrap it should be listed somewhere and the relationship clearly established.

I feel the docs could benefit greatly from a Recipies section where individuals can show off their various configurations. It seems a lot saner than following a going-on 2 year-old blog post to get set-up or trying to eagle-eye one's way through the docs as they are today.

These are my unabashed thoughs. Please take them with a grain of salt.

@bradzacher
Copy link
Member

@balibebas - have you checked out our getting started guide?
https://github.com/typescript-eslint/typescript-eslint/tree/master/docs/getting-started

That is intended to be the "this is how you set it up in your codebase" doc that talks through the setup and why everything exists.

@ghost
Copy link

ghost commented Jun 18, 2020

Great. Hadn't seen that yet. Will give it a look soon. Thanks, @bradzacher.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting response Issues waiting for a reply from the OP or another party package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin question Questions! (i.e. not a bug / enhancment / documentation)
Projects
None yet
Development

No branches or pull requests

2 participants