Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
feat(eslint-plugin): auto-generate plugin types#7272
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for the PR,@bradzacher! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently onhttps://opencollective.com/typescript-eslint. |
netlifybot commentedJul 19, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify site configuration. |
nx-cloudbot commentedJul 19, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
codecovbot commentedJul 19, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@## main #7272 +/- ##==========================================- Coverage 87.40% 87.39% -0.01%========================================== Files 381 381 Lines 13313 13315 +2 Branches 3934 3935 +1 ==========================================+ Hits 11636 11637 +1 Misses 1292 1292- Partials 385 386 +1
Flags with carried forward coverage won't be shown.Click here to find out more.
|
JoshuaKGoldberg left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry for taking a while on this - I kept pausing and coming back assuming I'd missed something. I don't follow why we'd need thesimpleTraverse approach rather than a runtime import of built files?
| } | ||
| constBREAK_TRAVERSE=Symbol(); | ||
| asyncfunctionextractExportedKeys( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So I read through this and I think it makes sense for the approach of manually traversing through the source files and reading in their declaration nodes... but that's a lot of work. Why not just import from the built output and use the runtime values?
importfsfrom'node:fs/promises';importpluginfrom'../dist/index.js';awaitfs.writeFile('./dist/types/rules.d.ts',`import type { RuleModule } from '@typescript-eslint/utils/ts-eslint';type Keys =${Object.keys(plugin.rules).map(ruleName=>` | '${ruleName}'`).join('\n')}export const rules: Readonly< Record<Keys, RuleModule<string, readonly unknown[]>>>;`,);
(that, but with Prettier formatting,make-dir first, etc.)
| |undefined; | ||
| constmeta=rule?.meta; | ||
| if(!meta?.docs){ | ||
| if(!rule||!meta?.docs){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: unnecessary change?
| if(!rule||!meta?.docs){ | |
| if(!meta?.docs){ |
Uh oh!
There was an error while loading.Please reload this page.
JoshuaKGoldberg commentedOct 18, 2023
👋 ping@bradzacher - just checking in, is this still something you have time for? |
JoshuaKGoldberg commentedJan 11, 2024
Switching this to draft so it doesn't show up on my queue. |
bradzacher commentedJan 13, 2024
I'm going to close this for now at least. I thought we'd need this for flat config support but there are ways around this - in#7935 I've essentially imported the plugin with its existing manual types and then add an explicit type to the name within the new package. In future we might want to add types for rule configs - but for now at least we're not going to. |
Background
We want type declarations for our plugin - but turning on type declarations is a PITA for our plugin.
TS wants to transpile each rule file to this
.d.tsfile:However because we don't ever do
import type { RuleModule } from '@typescript-eslint/utils/ts-eslint';and we don't often doimport type { TSESLint } from '@typescript-eslint/utils';- TS cannot name theRuleModuletype, leading to the following error:In a nutshell TS does not want to automatically add an import to the
.d.tsoutput because it doesn't know if that import might have type side-effects (eg ambient types / global augmentation) - which is completely fair.In the past we just said "fuck it" and bundled NO types with the plugin at all. But in v6 we had internal usecases for types so we had to add some. We added some manually crafted ones which are okay, but they're obviously quite loose - defining
Record<string, T>instead of typing the keys because it saves us trying to keep the keys in sync.Motivation
ESLint flat configs are coming and they have a new set of needs - namely that users will need to import our plugin to reference in their config - whereas classic configs the reference was implicit and resolved by ESLint.
So we need some external types and we want the keys typed so that users can reference things safely.
Overview
I assessed a few solutions:
tscwith--noEmitOnError=falseto force TS to emit despite errors.d.tsfiles for files with TS errors and gettingsrc/rules/index.tsto compile without errors with declarations is not fun.@microsoft/api-extractorto generate a type declaration without explicitly using TS.d.tsfiles.This PR implements a script to generate the
.d.tsfiles automatically as part of the build step. It's a simple script leveraging our parser to extract the keys and output the following 3 files.dist/_types/configs.d.ts
dist/_types/index.d.ts
dist/_types/rules.d.ts