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

Recommend using imported type for .eslintrc.js #2153

Closed
KamasamaK opened this issue Jun 2, 2020 · 7 comments
Closed

Recommend using imported type for .eslintrc.js #2153

KamasamaK opened this issue Jun 2, 2020 · 7 comments
Labels
awaiting response Issues waiting for a reply from the OP or another party question Questions! (i.e. not a bug / enhancment / documentation)

Comments

@KamasamaK
Copy link

I am willing to provide the PR, but first wanted to know if this was something you'd be willing to accept. The recommended file starter doesn't include the import type hint /** @type {import('eslint').Linter.Config} */ at the top. I think type checking JavaScript files should be recommended, even if it's just a configuration file. It would be especially useful with editor tools -- the issue at microsoft/vscode-eslint#124 demonstrates a desire for this and it wasn't clear to them what the solution would be but turned out to be this simple.

@bradzacher bradzacher added the question Questions! (i.e. not a bug / enhancment / documentation) label Jun 2, 2020
@bradzacher
Copy link
Member

bradzacher commented Jun 2, 2020

import('eslint') is not a great thing as it will bring in a dependency on the @types/eslint package. Depending on your project, this can cause weird, hard to debug problems alongside our tooling.

Additionally those types aren't always 100% correct, as they're maintained by whomever wants or needs something, and nobody is providing proper oversight (they're only correct right now because I updated them because I needed them for something).

OTOH, our types are guaranteed to be correct for the version of ESLint we support, and are well documented.


Unfortunately it's not as simple as adding the comment, the docs would also have to describe in sufficient detail how to enable checking of that file as well, which is a bit beyond the simple installation steps we want in the root readme.

The feedback we've gotten from many users is that there are already too many steps required to setup linting, so I think adding in more isn't what we want.

ESLint also provides runtime validation of your configuration using their schemas, so it's not a requirement for setting up linting.

In the community we have been thinking and talking about this for a while now; see #1296 and the links in the description. There are solutions that can be worked on, but nobody has enough time to work on it.


OOC how did you file this issue? You filed one without an issue template or automatic tagging, but that shouldn't be possible as I've disabled the empty template.

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Jun 2, 2020
@KamasamaK
Copy link
Author

I've found it to work perfectly fine in VS Code without having @types/eslint as a project dependency -- it appears that it's being pulled into %USERPROFILE%\AppData\Local\Microsoft\TypeScript\x.x\node_modules on Windows somehow. If you would prefer to use your types instead, feel free.

Regarding it not being as simple as adding the comment, it was seamless with my editor (VS Code) and required no additional configuration. Also, I was viewing this as being a recommendation, not a requirement to use. It is just intended to help with editing and validating the configuration file. Whatever configuration you're talking about should be clearly indicated as being optional if they want that added functionality. I would imagine that since it's a comment, it would just be ignored if the tool didn't already support it.

I'm glad that the community is already working on solutions, and if one of those solutions can accomplish a better editing experience then I would be fine with that instead too.

Regarding how the issue was filed, I did so using https://github.com/typescript-eslint/typescript-eslint/issues/new/. Since this is regarding a README and is rather meta, I'm not sure which of those issue templates would apply.

@bradzacher
Copy link
Member

I guess it depends on what you want.
If you want autocomplete, then this will be enough to give you autocomplete, but it will provide no typechecking..

I just added the comment to our .eslintrc.js, made an invalid change, and got an ESLint runtime error, but no typecheck error:
image

So if you want typechecking, then it will require additional setup.

This is thing I'm cautious about in adding this to the docs.
The comment by itself gives the impression that there is some type-checking provided; when likely there isn't (as users are unlikely to have JS checking or config files setup to be checked). This will confuse users.

Sorry if I'm sounding opposed or defeatist. I'm trying to talk through this to figure out if it's something we can add and how.

I think you could add a separate doc to the docs/getting-started/linting/ folder, and link to it from the main readme.
That way it wouldn't over-crowd the main getting started doc, and would be an optional step for people that are interested. Also that would give you the space to clearly outline what it does, how to enable it as well as any steps to enable full typechecking for the file.

Note that it would likely be better to use this type instead as previously mentioned

/** @type {import('@typescript-eslint/experimental-utils').TSESLint.Linter.Config} */

@G-Rath
Copy link
Contributor

G-Rath commented Jun 21, 2020

it appears that it's being pulled into %USERPROFILE%\AppData\Local\Microsoft\TypeScript\x.x\node_modules on Windows somehow

That's probably vscode doing automatic type acquisition.

@bradzacher

There are solutions that can be worked on

I'd be interested in hearing more about what other solutions there could be, aside from #1296 & my rejected eslint/rfcs#50 proposal (which I hope might be open to being revisited if eslint/rfcs#9 ever lands, as part of that has the config loading broken out into another package so it'd no longer be in the core), as I've not not really been able to come up with a better one than doing:

/** @type {import('eslint').Linter.Config} */
const config = { ... }

module.exports = config;

I did recently publish eslint-plugin-eslint-config which could be considered almost a third solution, but it can't lint the config that configures it since if that has errors it can't run in the first place.

The primary problem I ran into when thinking of solutions is that IDEs & tools tend to look explicitly for eslint (and sometimes support "standard" too), so something like publishing a package that wraps ESLint + apply my patch from the rfc proposal wouldn't work well out of the box as IDEs would favor the ESLint package, and also not view .eslintrc.ts as a config.

@bradzacher
Copy link
Member

#1296 is really the only solution that will work everywhere, I believe.
As the function is externally defined in TS, and strictly typed, it'll work in a .eslintrc.js file, and TS will explicitly check and enforce correctness:

image

@G-Rath
Copy link
Contributor

G-Rath commented Jun 21, 2020

I do like it as a solution; is there anything outstanding that still needs to be done before it can be landed?

@bradzacher
Copy link
Member

it needs someone to champion it - to take it, finish it, test it and make sure it's all working as intended.
I don't remember the exact state it's in right now - it was just a quick proof of concept.

@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 question Questions! (i.e. not a bug / enhancment / documentation)
Projects
None yet
Development

No branches or pull requests

3 participants