From 6ef73be7b5360f3660e44ee46f283e6d610e098d Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Mon, 18 Nov 2019 20:11:06 +1300 Subject: [PATCH 1/9] New: First-Class Support For `.eslintrc.ts` --- .../README.md | 335 ++++++++++++++++++ 1 file changed, 335 insertions(+) create mode 100644 designs/2019-typescript-eslintrc-support/README.md diff --git a/designs/2019-typescript-eslintrc-support/README.md b/designs/2019-typescript-eslintrc-support/README.md new file mode 100644 index 00000000..8f6312b0 --- /dev/null +++ b/designs/2019-typescript-eslintrc-support/README.md @@ -0,0 +1,335 @@ +- Start Date: 2019-11-18 +- RFC PR: (leave this empty, to be filled in later) +- Authors: Gareth Jones ([@g-rath](https://github.com/g-rath)) + +# First-Class Support For `.eslintrc.ts` + +## Summary + +Support the ability to consume `.eslintrc.ts` configuration files natively. + +## Motivation + +Currently configuration settings are validated by JSON Schema, which while +powerful does not have ubiquitous & high quality support across editors & +tooling. + +It also tends to generate somewhat cryptic messages, especially when using +complex elements such as combining `oneOf`, `anyOf`, and other such schemas. + +Meanwhile, TypeScript is a powerful & popular superset of Javascript that has +high quality support as a first class language in the majority of editors, and +is quickly gaining ubiquitous support in general. + +By allowing first-class support for `.eslintrc.ts`, the amount of configuration +problems & general confusion over such experienced by users can be reduced, +since it would enable us to provide tooling to type configuration files and so +allow TypeScript to catch ESLint configuration errors at compile time, rather +than run time. + +While we can already hack ways to use `.eslintrc.ts`, none of them lend +themselves to ease-of-use in the same manner that ESLint encourages, and so +first-class support for `.eslintrc.ts` will go long way towards encouraging +community driven typing of ESLint plugins. + +Finally, ESLint is being adopted as the linter for TypeScript, meaning +first-class support for `.eslintrc.ts` will help enable a positive feedback loop +to help improve the two tools. + +There are some potential UX benefits for users that use & understand TypeScript, +in the form + +## Detailed Design + +- `loadConfigFile` function is passed a `filePath` string parameter. +- If the `filePath` parameter ends with the extension `.ts`, `loadTSConfigFile` + is called passing it `filePath`. +- `loadTSConfigFile` attempts to require `ts-node` and call `register()` + - If an error is thrown, `eslint` bails out with an error. +- The `registerer` returned by `register()` is stored for use. +- The `registerer` is enabled (by calling `registerer.enabled(true)`) +- `importFresh` is called, being passed the `filePath`. + - The return is stored for later use. +- The `registerer` is disabled (by calling `registerer.enabled(false)`) + - This is to reduce risk of errors by mistakenly requiring `ts` files during + standard linting. +- The stored return of `importFresh` is returned. +- `loadConfigFile` returns the result of `loadTSConfigFile` +- `eslint` continues per normal. + +## Documentation + +I don't think a large amount of ceremony is required, but @typescript-eslint +might like to do something. + +Overall, updating the list of accepted formats for `eslintrc`, and a small note +with a link to @typescript-eslint I think would be all that's needed. + +@typescript-eslint probably would like to provide some documentation on how to +nicely type & write definitions for ESLint plugins, but that's not strictly +required by this feature. + +## Drawbacks + +> Slight increase in maintenance burden for core support. + +People may open issues complaining about TypeScript-related errors, or errors +relating to the registering of `ts-node`. + +The majority of these should be detectable from their stacktrace or use of +keywords such as TypeScript, `ts`, `ts-node`, and `.eslintrc.ts`. As such, I'd +expect it should be possible to refine `eslint-bot` to triage these issues and +post a comment detailing common problems & their solutions to help reduce this +cost. + +Personally I don't think the general amount of maintenance that would result +from this change would be too drastic, as I'm confident that both the core +TypeScript and @typescript-eslint communities would be happy to foot the +maintenance bill for this change. + +## Backwards Compatibility Analysis + +This should be a completely backwards compatible change, since ESLint currently +ignores `.eslintrc.ts` files. + +There appears to be no usage of `.eslintrc.ts` in _any_ public code on GitHub, +meaning no one is compiling `.eslintrc.ts` -> `.eslintrc.js`. + +However, this can be made a complete non-issue by ensuring implementation favors +`.js` over `.ts` in the event both are on offer. + +## Alternatives + +> Use the TypeScript --checkJs` flag + +While TypeScript can provide limited typechecking of js files via `--checkJs`, +it's no where near as powerful as native `.ts` type checking. + +> Using a flag a la `--require` + +While that would be nice, it would also increase the overall implementation +footprint, and (more importantly) defeat one of the great things about ESLint: +It Just Works (everywhere. with everything). + +ESLint has near ubiquitous support in editors and tooling, meaning that simply +opening a project that has an ESLint configuration causes editors to traverse +code paths that search for ESLint, and if found, start linting any open files. + +Supporting this feature behind a flag would be a poor middle ground that'd make +it far more painful to use, since you'd have to change your settings every time +to call ESLint with the flag, and that's assuming your editor or tool allows you +to pass additional flags to ESLint anytime it gets called! + +> Can you just `ts-node/register` in `.eslintrc.js`? + +i.e + +```js +require('ts-node').register(); +module.exports = require('./eslintrc.ts'); +``` + +While the above would work, it means you'd have to have two `eslintrc` files; +this can also cause additional confusion as typically when compiling `.ts` files +their respective outputted `.js` file is placed next to the original file. +Hence, in the TypeScript community it'd be typically assumed `.eslintrc.js` is +the compiled form of `.eslintrc.ts`. + +> Why not just compile `.eslintrc.ts` + +Because that would require a build step to be run before every lint, and if you +forgot to do that, it would result in either confusing errors from ESLint about +rules you configured differently, or about config files not existing. + +### Prior Arts + +#### [Webpack](https://github.com/webpack/webpack) + +Webpack allows the loading of `.ts` configuration files when `ts-node` is +installed. + +#### [webpack-nano](https://github.com/shellscape/webpack-nano/blob/master/lib/config.js#L27-L64) + +Uses [`rechoir`](http://github.com/tkellen/node-rechoir) to support arbitrary +module loading, including TypeScript. `rechoir` handles the automatic loading & +registering of transpilers without bundling them; allowing you to simply install +the transpiler required to + +```js +const fileTypes = { + '.esm.js': [esmRegister], + '.es6': ['@babel/register', esmRegister], + '.mjs': ['@babel/register', esmRegister], + '.babel.js': [ + '@babel/register', + 'babel-register', + 'babel-core/register', + 'babel/register' + ], + '.babel.ts': ['@babel/register'], + '.ts': [ + 'ts-node/register', + 'typescript-node/register', + 'typescript-register', + 'typescript-require' + ] +}; + +/* ... */ + +const requireLoader = extension => { + try { + rechoir.prepare(fileTypes, `config${extension}`, cwd); + } catch (e) { + /* ... */ + } +}; +``` + +#### [karma](https://github.com/karma-runner/karma/blob/7617279f68507642fdc7eb0083916f3c6c2a28cb/lib/config.js#L36-L39) + +Karma automatically register `ts-node` in their config parser to support +`karma.conf.ts`: + +``` +try { + require('ts-node').register() + TYPE_SCRIPT_AVAILABLE = true +} catch (e) {} +``` + +#### [DangerJS](https://danger.systems/js/guides/getting_started.html#creating-a-dangerfile) + +> Create either a new file, either dangerfile.js or dangerfile.ts will be picked +> up automatically. Otherwise you can use an argument to pass in a custom file. + +## Open Questions + + + +## Help Needed + + + +## Frequently Asked Questions + +> if ESLint isn't written in TypeScript, will you still get type hinting for the +> config file? + +Yes! TypeScript doesn't need the actual runtime code to be written in +Javascript - in fact, TypeScript source code is generally never shipped in npm +packages. + +Instead, TypeScript consumes +[type definitions](https://www.typescriptlang.org/docs/handbook/modules.html#working-with-other-javascript-libraries) +which allow you to type pure Javascript libraries. These files have the +extension `.d.ts`, and you've probably seen them around in npm packages. + +These definitions can (and are) shipped either with the npm package their +defining, or as a completely separate npm package. +[DefinitelyTyped](https://github.com/DefinitelyTyped/DefinitelyTyped) exists +specifically to support this effort - it's a community driven repo that hosts, +maintains, and publishes type definitions for npm packages. + +For example, +[DefinitelyTyped](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/eslint/index.d.ts) +has eslint types based off of the standard estree spec, and +[typescript-eslint](https://github.com/typescript-eslint/typescript-eslint/tree/master/packages/experimental-utils/src/ts-eslint) +has types for eslint based off of the typescript-estree spec. + +This means that even if an `eslint` plugin didn't provide types, someone in the +community could add them to `DefinitelyTyped` to enable TypeScript support for +that plugin. + +This is an example of how a definition files might look for +`eslint-plugin-jest`: + +```ts +// @types/eslint +type ErrorLevel = 0 | 1 | 2 | 'off' | 'warn' | 'error'; +declare namespace ESLint { + interface Rules { + indent?: + | ErrorLevel + | [ErrorLevel, number | 'tabs', { SwitchCase: number /* ... */ }]; + } + + interface Config { + rules: Rules; + } +} + +// @types/eslint-plugin-jest +declare namespace ESLint { + interface Rules { + 'consistent-test-it'?: + | ErrorLevel + | [ErrorLevel, { fn?: 'it' | 'test' /* ... */ }]; + } +} + +// .eslintrc.ts +const config: ESLint.Config = { + rules: { + 'consistent-test-it': ['error', { fn: 'it' }] + } +}; +``` + +[Playground Link](https://www.typescriptlang.org/play/?ssl=1&ssc=1&pln=36&pc=1#code/PTAEAEBcE8AcFMDOwkBsCWA7SBYAUDAqAKIBOpA9qQDLwBu8qoAvKAAygA+oAjF6ACZ+AcgoAzMcJEB3AIalMU7sPjkqwgNz4AJvADGqefFCZZAWySxZe48QDK1LJFABvfKA+gnqsdeMAlAFdUJFd3TwisXWwAfgAuEjUaekZ+AG1wiKzEymSGVAAaTOzPTECzACNVEUhZCsRhIrwSrJdQO2l0SD0ACwBhWUR4BLLK1QLQYAAqUAA6edApsABfJpaAXS1mz2X8Yu9SXxtQPopMMXQAczDt7NJgpASgkMQtiN28D-wQCEIkFEQGGwAFpYKhApcsMCAFZIXB4XQGIwmcyWPwkBxOG6RbA+dHPUJuW5ZYR6M6IdCISDwEHUqnArrCeI5Ki0fLpYotMi5NmMNYtTxtMSYZnCRk1OGNSYzeazRYrfnZTbFD5fPA-WZoJykPSzSCIfBkzBU0BGi6XBL2RzYWanc5XFjYzz3F4JIktABERopVJpkGBdP9XQ9CQyxM9qlyHsVAqFmASHuDoFWnM86xjKc+WyAA) + +> So does this mean we'll have to write & maintain TypeScript definitions for +> all the rules? + +Sort of. Technically it depends on where the definitions end up being hosted as +to who ideally should maintain them, but ultimately it will be a community +effort. + +However, it's very easy to automate a large amount of the process, which has +actually already been done via +[this script](https://github.com/bradzacher/eslint-config-brad/blob/master/scripts/convertRuleOptionsToTypescriptTypes.ts), +which generated +[these types](https://github.com/bradzacher/eslint-config-brad/tree/master/src/types/eslint), +and actually found a number of bugs, including one in a base ESLint rule! +([#12051](https://github.com/eslint/eslint/pull/12051)) + +The majority of the work will be once-off, as once the definitions are written, +the maintenance burden will be roughly the same as that of maintaining the +schemas & docs. + +Finally, this is actually technically out of scope of this RFS - whats being +described is something that can happen without involvement from the ESLint core +team. + +> Will `.eslintrc.ts` be recompiled every time it's loaded? + +Potentially yes, due to the bypassing of the config cache by ESLint. + +In theory, the TypeScript compiler instance should be persisted between loads +meaning it should be able to leverage its own cache to not recompile the config +if it hasn't changed. + +> Won't this slow down my linting? + +In theory this will increase initial lint speeds, but it should only be by a few +seconds, depending on config size, since TypeScript only compiles what it needs +to, and it's rare to import core project files into eslint configuration. + +## Related Discussions + +https://github.com/eslint/eslint/issues/12078 +https://github.com/eslint/eslint/pull/12082 From 0a931a787b9003b645ae3acea43aa4ce92f164fc Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Mon, 18 Nov 2019 21:14:14 +1300 Subject: [PATCH 2/9] Update: rewrite `Detailed Design` to be more user-perspective focused --- .../README.md | 26 ++++++++----------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/designs/2019-typescript-eslintrc-support/README.md b/designs/2019-typescript-eslintrc-support/README.md index 8f6312b0..3425f975 100644 --- a/designs/2019-typescript-eslintrc-support/README.md +++ b/designs/2019-typescript-eslintrc-support/README.md @@ -41,21 +41,17 @@ in the form ## Detailed Design -- `loadConfigFile` function is passed a `filePath` string parameter. -- If the `filePath` parameter ends with the extension `.ts`, `loadTSConfigFile` - is called passing it `filePath`. -- `loadTSConfigFile` attempts to require `ts-node` and call `register()` - - If an error is thrown, `eslint` bails out with an error. -- The `registerer` returned by `register()` is stored for use. -- The `registerer` is enabled (by calling `registerer.enabled(true)`) -- `importFresh` is called, being passed the `filePath`. - - The return is stored for later use. -- The `registerer` is disabled (by calling `registerer.enabled(false)`) - - This is to reduce risk of errors by mistakenly requiring `ts` files during - standard linting. -- The stored return of `importFresh` is returned. -- `loadConfigFile` returns the result of `loadTSConfigFile` -- `eslint` continues per normal. +- Adds support for `.eslintrc.ts` +- Requires uses to install `ts-node` manually adjacent to ESLint in + `node_modules`. + - Failure to do so with a `.eslintrc.ts` results in ESLint throwing a fatal + error + - The error message should include understandable plain text instructions on + how to fix the error. +- ESLint does not validate the configuration object for now. +- `.eslintrc.ts` is second highest priority, after `.eslintrc.js` + - This matches the priority of the other supported types, and generally where + TypeScript files it for other tools i.e webpack ## Documentation From f95eb211ed6faf6fbb6bdf40b849296a75e6feb0 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Mon, 18 Nov 2019 22:27:20 +1300 Subject: [PATCH 3/9] Update: Set RFC PR Co-Authored-By: Toru Nagashima --- designs/2019-typescript-eslintrc-support/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/designs/2019-typescript-eslintrc-support/README.md b/designs/2019-typescript-eslintrc-support/README.md index 3425f975..60296d75 100644 --- a/designs/2019-typescript-eslintrc-support/README.md +++ b/designs/2019-typescript-eslintrc-support/README.md @@ -1,5 +1,5 @@ - Start Date: 2019-11-18 -- RFC PR: (leave this empty, to be filled in later) +- RFC PR: https://github.com/eslint/rfcs/pull/50 - Authors: Gareth Jones ([@g-rath](https://github.com/g-rath)) # First-Class Support For `.eslintrc.ts` From e8809b72a0d0b65d8ea995d4c161360da606f4a5 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Mon, 18 Nov 2019 22:46:55 +1300 Subject: [PATCH 4/9] Update: Add detail about support for other file fields --- .../README.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/designs/2019-typescript-eslintrc-support/README.md b/designs/2019-typescript-eslintrc-support/README.md index 60296d75..cf636813 100644 --- a/designs/2019-typescript-eslintrc-support/README.md +++ b/designs/2019-typescript-eslintrc-support/README.md @@ -65,6 +65,11 @@ with a link to @typescript-eslint I think would be all that's needed. nicely type & write definitions for ESLint plugins, but that's not strictly required by this feature. +It should be documented that _only_ `.eslintrc.ts` is supported (i.e not +`parsers`,`plugins`, etc), and that the shipping of `.ts` configurations is +discouraged - instead they should be compiled into JavaScript, and shipped with +a type definition file (`.eslintrc.d.ts`). + ## Drawbacks > Slight increase in maintenance burden for core support. @@ -325,6 +330,20 @@ In theory this will increase initial lint speeds, but it should only be by a few seconds, depending on config size, since TypeScript only compiles what it needs to, and it's rare to import core project files into eslint configuration. +> Does this include TypeScript support for `parser`, `plugins`, or +> `obj.configs.recommended` of eslint-plugin-foo? + +No, this RFC intentionally pertains only to the support of `.eslintrc.ts`. + +The reason for this is that the shipping of TypeScript source in packages means +TypeScript has to be installed to consume the, regardless of if the project is +using TypeScript. + +Instead, these files should be compiled, and shipped with their `.d.ts` +definition file to allow usage in TypeScript. When using TypeScript, this is the +same as consuming a TypeScript source file, so still allows for such sharables +to be written in TypeScript. + ## Related Discussions https://github.com/eslint/eslint/issues/12078 From e4ee497141e30c260cec943247e6be26da9fde22 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Mon, 18 Nov 2019 22:52:13 +1300 Subject: [PATCH 5/9] Update: adjust wording of how `ts-node` is required --- designs/2019-typescript-eslintrc-support/README.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/designs/2019-typescript-eslintrc-support/README.md b/designs/2019-typescript-eslintrc-support/README.md index cf636813..09bccf0e 100644 --- a/designs/2019-typescript-eslintrc-support/README.md +++ b/designs/2019-typescript-eslintrc-support/README.md @@ -42,8 +42,9 @@ in the form ## Detailed Design - Adds support for `.eslintrc.ts` -- Requires uses to install `ts-node` manually adjacent to ESLint in - `node_modules`. +- Requires uses to install `ts-node` manually relative to the `node_modules` of + the configuration file, per standard Node + [module resolution logic](https://nodejs.org/api/modules.html#modules_loading_from_node_modules_folders) - Failure to do so with a `.eslintrc.ts` results in ESLint throwing a fatal error - The error message should include understandable plain text instructions on From 68233c58da16d9b2c64b08a20f322ccac9dbca88 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Tue, 19 Nov 2019 07:41:21 +1300 Subject: [PATCH 6/9] Update: remove stray thought & fix typo --- designs/2019-typescript-eslintrc-support/README.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/designs/2019-typescript-eslintrc-support/README.md b/designs/2019-typescript-eslintrc-support/README.md index 09bccf0e..5ad45cbe 100644 --- a/designs/2019-typescript-eslintrc-support/README.md +++ b/designs/2019-typescript-eslintrc-support/README.md @@ -36,13 +36,10 @@ Finally, ESLint is being adopted as the linter for TypeScript, meaning first-class support for `.eslintrc.ts` will help enable a positive feedback loop to help improve the two tools. -There are some potential UX benefits for users that use & understand TypeScript, -in the form - ## Detailed Design - Adds support for `.eslintrc.ts` -- Requires uses to install `ts-node` manually relative to the `node_modules` of +- Requires users to install `ts-node` manually relative to the `node_modules` of the configuration file, per standard Node [module resolution logic](https://nodejs.org/api/modules.html#modules_loading_from_node_modules_folders) - Failure to do so with a `.eslintrc.ts` results in ESLint throwing a fatal From 55130c2e393d76a5a9db75c4b22015a230a4028b Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Tue, 19 Nov 2019 08:36:07 +1300 Subject: [PATCH 7/9] Update: remove personal thoughts about the footing of the maintenance bill --- designs/2019-typescript-eslintrc-support/README.md | 5 ----- 1 file changed, 5 deletions(-) diff --git a/designs/2019-typescript-eslintrc-support/README.md b/designs/2019-typescript-eslintrc-support/README.md index 5ad45cbe..81e8aec0 100644 --- a/designs/2019-typescript-eslintrc-support/README.md +++ b/designs/2019-typescript-eslintrc-support/README.md @@ -81,11 +81,6 @@ expect it should be possible to refine `eslint-bot` to triage these issues and post a comment detailing common problems & their solutions to help reduce this cost. -Personally I don't think the general amount of maintenance that would result -from this change would be too drastic, as I'm confident that both the core -TypeScript and @typescript-eslint communities would be happy to foot the -maintenance bill for this change. - ## Backwards Compatibility Analysis This should be a completely backwards compatible change, since ESLint currently From 2edda07f14b95d16a4be4a26f71d55b09903629e Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Tue, 19 Nov 2019 08:37:57 +1300 Subject: [PATCH 8/9] Update: improve wording of when ESLint will throw a fatal error --- designs/2019-typescript-eslintrc-support/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/designs/2019-typescript-eslintrc-support/README.md b/designs/2019-typescript-eslintrc-support/README.md index 81e8aec0..e3a1414d 100644 --- a/designs/2019-typescript-eslintrc-support/README.md +++ b/designs/2019-typescript-eslintrc-support/README.md @@ -42,8 +42,8 @@ to help improve the two tools. - Requires users to install `ts-node` manually relative to the `node_modules` of the configuration file, per standard Node [module resolution logic](https://nodejs.org/api/modules.html#modules_loading_from_node_modules_folders) - - Failure to do so with a `.eslintrc.ts` results in ESLint throwing a fatal - error + - Failure to require `ts-node` with a `.eslintrc.ts` results in ESLint + throwing a fatal error - The error message should include understandable plain text instructions on how to fix the error. - ESLint does not validate the configuration object for now. From af74c0c867b192af60d4bd6ddc79724aabc8f05e Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Tue, 19 Nov 2019 08:38:31 +1300 Subject: [PATCH 9/9] Update: adjust wording of where `ts-node` will be required from --- designs/2019-typescript-eslintrc-support/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/designs/2019-typescript-eslintrc-support/README.md b/designs/2019-typescript-eslintrc-support/README.md index e3a1414d..29673032 100644 --- a/designs/2019-typescript-eslintrc-support/README.md +++ b/designs/2019-typescript-eslintrc-support/README.md @@ -39,8 +39,8 @@ to help improve the two tools. ## Detailed Design - Adds support for `.eslintrc.ts` -- Requires users to install `ts-node` manually relative to the `node_modules` of - the configuration file, per standard Node +- Requires users to install `ts-node` separately along the path that the + `eslint` command is being run, per standard Node [module resolution logic](https://nodejs.org/api/modules.html#modules_loading_from_node_modules_folders) - Failure to require `ts-node` with a `.eslintrc.ts` results in ESLint throwing a fatal error