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

Linting should include type checking #352

Open
arvigeus opened this issue Nov 27, 2019 · 8 comments
Open

Linting should include type checking #352

arvigeus opened this issue Nov 27, 2019 · 8 comments
Labels
kind: discussion Discussion on changes to make kind: feature New feature or request scope: docs Documentation could be improved. Or changes that only affect docs

Comments

@arvigeus
Copy link

Current Behavior

const wrong = parseInt(8);

npm run lint reports no problem with this (which is strange, usually should require a second radix param, but this is not important right now)

Desired Behavior

Linting should run tsc --noEmit to catch that Argument of type '8' is not assignable to parameter of type 'string' error

Suggested Solution

Run tsc --noEmit followed by eslint

Who does this impact? Who is this for?

VS Code users (maybe others as well). If file is not open there is no way to get an error report from it, except building

@swyxio
Copy link
Collaborator

swyxio commented Dec 4, 2019

i think this is a reasonable suggestion, but has tradeoffs in running that extra step for linting. its easy to do in userland too. will leave it to @jaredpalmer but inclined to do nothing here. maybe document it in the lint section.

@swyxio swyxio added kind: feature New feature or request scope: docs Documentation could be improved. Or changes that only affect docs labels Dec 4, 2019
@arvigeus
Copy link
Author

arvigeus commented Dec 4, 2019

If not too overly complicated, what about an additional flag as an opt-in feature?
tsdx lint --include-ts or something like this?

@swyxio
Copy link
Collaborator

swyxio commented Dec 4, 2019

i have no strong opinions here. up to jared

@slikts
Copy link

slikts commented Feb 27, 2020

I don't understand what the intended workflow would be. Normally you get type checking in the files open in an editor and then have either a tsc watch process or run it manually after some changes to see if there's errors in the rest of the files. Building the project is an unnecessary overhead for just checking for type errors.

@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 17, 2020

Copying from #529:

#352's change doesn't actually align with how ESLint works.
An intermediate change would be to change the templates such that the lint script runs tsc --noEmit && tsdx lint. That way a workflow isn't being forced upon users (they can change it/it would be merely a recommendation) and tsdx lint isn't different from ESLint's behaviors.
Misalignment with the underlying tools & their integrations is already an increasing issue with TSDX (#514, #498, some on testing too), and that misalignment is very confusing and unintuitive, so I don't think further misaligning by changing tsdx lint itself is a good idea.

Another option is to have another command like tsdx tsc, but I have a feeling that would cause more issues than it solves

I think we'll either add a typecheck script to the templates or change the lint script to accommodate for this, but as this is already entirely accomplished by tsc I don't think it would be good to add even more surface area to TSDX internals by adding a tsdx tsc command.

(Please note the distinction between "command" and "script" in case the terminology is confusing)

@agilgur5 agilgur5 added the kind: discussion Discussion on changes to make label Mar 17, 2020
@gustavopch
Copy link

gustavopch commented Mar 26, 2020

According to Wikipedia:

lint, or a linter, is a tool that analyzes source code to flag programming errors, bugs, stylistic errors, and suspicious constructs. https://en.wikipedia.org/wiki/Lint_(software)

If we agree on that definition, type-checking should be part of tsdx lint and not another command. TSDX is all about providing a good developer experience for TypeScript users (hence the name) by reducing the amount of boilerplate on userland, right? It does that by being opinionated and I believe it's a pretty good opinion to have our types checked for the exact same reason we like to have our code checked by ESLint or Prettier.

The biggest issue here is that tsdx lint should continue to be able to receive the specific files as arguments so that it works with lint-staged and unfortunately tsc will ignore tsconfig.json in that situation and screw up the whole thing.

Good news is that I've built tsc-files to handle that case (it's just a tiny wrapper on top of tsc).

Edit: now I'm not sure type-checking specific files would be actually useful. If a change to foo.ts breaks some type on bar.ts and only foo.ts is checked, then bar.ts' broken types wouldn't be noticed. :/

@gburnett
Copy link

gburnett commented May 1, 2020

The biggest issue here is that tsdx lint should continue to be able to receive the specific files as arguments so that it works with lint-staged

@gustavopch Is there a workaround for this at the moment?

@gustavopch
Copy link

@gburnett First, upvote microsoft/TypeScript#27379 to show interest in an official solution from the TypeScript maintainers.

So, yes, there's a workaround. I started to write it: https://github.com/gustavopch/tsc-files. But, as I mentioned in the "Edit" section of my comment above, type-checking the files that were changed is not enough — we should also type-check all files that depend on the changed ones.

Currently, tsc-files won't do that, so I'm not even using it in my projects (I'm just type-checking the whole project with tsc --noEmit). I've opened an issue describing the problem and proposing a possible solution: gustavopch/tsc-files#6. A PR would be welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: discussion Discussion on changes to make kind: feature New feature or request scope: docs Documentation could be improved. Or changes that only affect docs
Projects
None yet
Development

No branches or pull requests

6 participants