feat: Add config types in @eslint/core #237
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR centralizes config-related types in the @eslint/core package, updates downstream packages to import those types, and enforces a consistent array type syntax.
- Moved or added
@eslint/coreas a dependency and replacedeslinttype imports with@eslint/coreequivalents incompat,config-helpers, andmigrate-config. - Renamed several type aliases for consistency (e.g.,
RuleEntry→RuleConfig,Linter.Config→ConfigObject). - Enabled the
@typescript-eslint/array-typerule and updated all array type declarations to the generic form.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/migrate-config/src/migrate-config.js | Updated JSDoc typedefs to import new config types from @eslint/core. |
| packages/migrate-config/package.json | Replaced @types/eslint with @eslint/core in devDependencies. |
| packages/config-helpers/src/types.ts | Switched imports and array type syntax to use @eslint/core and Array<T>. |
| packages/config-helpers/src/global-ignores.js | Updated JSDoc typedef to use @eslint/core ConfigObject. |
| packages/config-helpers/src/define-config.js | Replaced several JSDoc typedefs to import types from @eslint/core. |
| packages/config-helpers/package.json | Moved @eslint/core from devDependencies to dependencies. |
| packages/compat/src/ignore-file.js | Updated typedef to import ConfigObject from @eslint/core. |
| packages/compat/src/fixup-rules.js | Replaced JSDoc typedefs to import plugin, rule, and config types from core. |
| packages/compat/package.json | Moved @eslint/core from devDependencies to dependencies. |
| packages/core/src/types.ts | Converted all T[] and any[] usages to Array<T> and added new config types. |
| eslint.config.js | Enforced the @typescript-eslint/array-type rule (generic for default arrays). |
Comments suppressed due to low confidence (2)
packages/core/src/types.ts:723
- It may be helpful to add type-level tests (e.g., with dtslint) for the new
ConfigObjectand related types to ensure they enforce the intended constraints and catch future regressions.
export interface ConfigObject<Rules extends RulesConfig = RulesConfig> {
packages/core/src/types.ts:103
- [nitpick] This file has grown significantly in size and responsibility. Consider splitting core, config, and legacy type definitions into separate modules to improve readability and maintainability.
((...args: Array<any>) => void) | undefined
| /** | ||
| * The configuration for a set of files. | ||
| */ | ||
| export interface ConfigObject<Rules extends RulesConfig = RulesConfig> { |
There was a problem hiding this comment.
I used ConfigObject instead of Config to disambiguate with the Config class in eslint.
There was a problem hiding this comment.
+1, this has been a mild confusion for me for a while!
| /* eslint-disable @typescript-eslint/consistent-indexed-object-style, @typescript-eslint/no-explicit-any -- needed for backward compatibility */ | ||
|
|
||
| /** @deprecated Only supported in legacy eslintrc config format. */ | ||
| export type GlobalAccess = |
There was a problem hiding this comment.
Renamed from GlobalConf for clarity.
| | "writeable"; | ||
|
|
||
| /** @deprecated Only supported in legacy eslintrc config format. */ | ||
| export interface GlobalsConfig { |
There was a problem hiding this comment.
Renamed from Globals for clarity.
| /** | ||
| * The type of JavaScript source code. | ||
| */ | ||
| type JavaScriptSourceType = "script" | "module" | "commonjs"; |
There was a problem hiding this comment.
renamed from SourceType for clarity
| * @deprecated Only supported in legacy eslintrc config format. | ||
| * @see [Specifying Parser Options](https://eslint.org/docs/latest/use/configure/language-options#specifying-parser-options) | ||
| */ | ||
| export interface JavaScriptParserOptionsConfig { |
There was a problem hiding this comment.
Renamed from ParserOptions for clarity.
| } | ||
|
|
||
| /** @deprecated Only supported in legacy eslintrc config format. */ | ||
| export interface EnvironmentConfig { |
There was a problem hiding this comment.
Renamed from Environments for clarity.
| * @see [ESLint Legacy Configuration](https://eslint.org/docs/latest/use/configure/) | ||
| */ | ||
| // https://github.com/eslint/eslint/blob/v8.57.0/conf/config-schema.js | ||
| export interface LegacyConfigObject< |
There was a problem hiding this comment.
Renamed from LegacyConfig to match ConfigObject
|
Hi, it looks like the CI is currently failing. Would you mind taking a look when you have a moment? |
There was a problem hiding this comment.
🔥 Lovely! I'm really excited to have @eslint/core provide these types as a full dependency.
I'd like to see input from @kirkwaiblinger since Kirk has been working more deeply on the types integrations. This structure looks good to me and on playing around with the types, but if there's some deep intricate issue I might not know it.
packages/core/src/types.ts
Outdated
| */ | ||
| interface LintSuggestion { | ||
| /** A short description. */ | ||
|
|
| /** | ||
| * The configuration for a set of files. | ||
| */ | ||
| export interface ConfigObject<Rules extends RulesConfig = RulesConfig> { |
There was a problem hiding this comment.
+1, this has been a mild confusion for me for a while!
| */ | ||
| fix?: RuleFixer | null | undefined; | ||
| fix?: RuleTextEdit | undefined; | ||
|
|
There was a problem hiding this comment.
Just confirming, was this actual change intentional?
There was a problem hiding this comment.
Yes, the original one was incorrect.
| * An object containing settings related to how JavaScript is configured for | ||
| * linting. | ||
| */ | ||
| languageOptions?: LanguageOptions; |
There was a problem hiding this comment.
I see that LanguageOptions is just Record<string, unknown> for now...
Can it include a parser field with a real Parser type (also exported)?
I had a hard time finding an ESLint-authored type for a parser, see https://github.com/typescript-eslint/typescript-eslint/pull/11190/files#diff-4f6c93a5b54f249fc56a41d19a39f8279bb5f633f2f379c7b8a2272aca3b68ebR13-R15
There was a problem hiding this comment.
parser is a JavaScript-specific language option, and this is meant to work for all languages.
There was a problem hiding this comment.
Oh, I see. All the same can a Parser type be exported for downstream use? (Ie not enforced within languageOptions but just the type made available)
There was a problem hiding this comment.
I guess this can be a separate request/followup too
There was a problem hiding this comment.
Yeah, please open a separate issue for that.
|
I think this is finally ready. 🎉 |
|
It looks like something went wrong with the rebase. Can you check? |
Co-authored-by: Francesco Trotta <github@fasttime.org>
|
Okay, I think I fixed the rebase issues and addressed the other feedback. |
There was a problem hiding this comment.
Just a few minor suggestions, then LGTM. Leaving open for @mdjermanovic to review as requested.
Co-authored-by: Francesco Trotta <github@fasttime.org>
Co-authored-by: Francesco Trotta <github@fasttime.org>
Co-authored-by: Francesco Trotta <github@fasttime.org>
Prerequisites checklist
What is the purpose of this pull request?
Copied config-related types over from the
eslintpackage into@eslint/coreWhat changes did you make? (Give an overview)
package.jsonfiles inpackages/compat,packages/config-helpers, andpackages/migrate-configto move@eslint/coreto thedependenciessection, ensuring it is available at runtime. [1] [2] [3]eslinttype imports with@eslint/coreinpackages/compat/src/fixup-rules.js,packages/compat/src/ignore-file.js,packages/config-helpers/src/define-config.js,packages/config-helpers/src/global-ignores.js,packages/config-helpers/src/types.ts, andpackages/migrate-config/src/migrate-config.js. This includes updates to typedefs likeConfig,Plugin,RuleEntry, and others to their corresponding@eslint/corecounterparts. [1] [2] [3] [4] [5] [6]RuleEntrytoRuleConfigandLinter.ConfigtoConfigObject. [1] [2]@typescript-eslint/array-typeto enforce consistent array type definitions in TypeScript files. The rule is configured to usegenericfor default array types andarrayfor readonly arrays.Related Issues
fixes #226
Is there anything you'd like reviewers to focus on?