Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
fix(eslint-plugin): correct rules.d.ts types to not rely on non-existent imports#9339
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
fix(eslint-plugin): correct rules.d.ts types to not rely on non-existent imports#9339
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for the PR,@JoshuaKGoldberg! 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 commentedJun 12, 2024 • 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 commentedJun 12, 2024 • 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.
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.
I don'tlove this copy & pasting of types between files. But we don't publish.d.ts
files underdist/
for this package and don't publishsrc/
.
My preference would be to start publishing.d.ts
files to move towards a future where folks can strongly type individual rule options in theireslint.config.ts
/// @ts-check
'deslint.config.js
. But that seems like a bigger change, with potential for serious package size increases, out of scope for this PR.
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.
I wonder what if we removeESLintPluginDocs
andESLintPluginRuleModule
frompackages/eslint-plugin/src/util/createRule.ts
and import them frompackages/eslint-plugin/rules.d.ts
instead? I don't see any reason why this could end badly 🙂
import { ESLintUtils } from '@typescript-eslint/utils';-import type {- RuleModuleWithMetaDocs,- RuleRecommendation,- RuleRecommendationAcrossConfigs,-} from '@typescript-eslint/utils/ts-eslint';--export interface ESLintPluginDocs {- /**- * Does the rule extend (or is it based off of) an ESLint code rule?- * Alternately accepts the name of the base rule, in case the rule has been renamed.- * This is only used for documentation purposes.- */- extendsBaseRule?: boolean | string;-- /**- * If a string config name, which starting config this rule is enabled in.- * If an object, which settings it has enabled in each of those configs.- */- recommended?: RuleRecommendation | RuleRecommendationAcrossConfigs<unknown[]>;-- /**- * Does the rule require us to create a full TypeScript Program in order for it- * to type-check code. This is only used for documentation purposes.- */- requiresTypeChecking?: boolean;-}+import { ESLintPluginDocs } from '../../rules'; export const createRule = ESLintUtils.RuleCreator<ESLintPluginDocs>( name => `https://typescript-eslint.io/rules/${name}`, );-export type ESLintPluginRuleModule = RuleModuleWithMetaDocs<- string,- readonly unknown[],- ESLintPluginDocs->;
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.
I am ... deeply amused at how much better of a solution this is. 😄
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.
👍
7812072
intotypescript-eslint:v8Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
PR Checklist
src/util
import in@typescript-eslint/eslint-plugin@rc-v8
with skipLibCheck: false #9164Overview
Removes dependencies from
rules.d.ts
that don't exist, so it'll correctly type check for all consumers - even if they don'tskipLibCheck
.💖