Skip to content

chore: disambiguate internal types LanguageOptions and Rule #19669

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

Merged
merged 2 commits into from
May 1, 2025

Conversation

fasttime
Copy link
Member

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[X] Other, please explain:

Fix internal types

What changes did you make? (Give an overview)

In the ESLint code, the types LanguageOptions and Rule are currently being used for both language-generic types (which now have equivalents in the core types) and for JavaScript-specific types. This PR resolves the ambiguities as follows:

  • For language-generic language options, the type LanguageOptions from core types is used.
  • For language-generic rule objects, the type RuleDefinition from core types is used.
  • For JS-specific language options, the type Linter.LanguageOptions from the ESLint types is used with the alias JSLanguageOptions. This is probably the name we'll use when the JS language logic is moved to the @eslint/js plugin (similar to CSSLanguageOptions or MarkdownLanguageOptions), so I chose to use JSLanguageOptions to avoid confusion.
  • For JS-specific rules, the type Rule.RuleModule from the ESLint types is used with the alias Rule. Rule.RuleModule is the type historically found in place of JSRuleDefinition in legacy code or for built-in rules.

I also removed the unused LanguageOptions type from lib/shared/types.js.

Is there anything you'd like reviewers to focus on?

@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Apr 28, 2025
@eslint-github-bot eslint-github-bot bot added the chore This change is not user-facing label Apr 28, 2025
@github-actions github-actions bot added the core Relates to ESLint's core APIs and features label Apr 28, 2025
Copy link

netlify bot commented Apr 28, 2025

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit e92fa18
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6811ccf394c34800087915c5

@fasttime fasttime marked this pull request as ready for review April 28, 2025 18:40
@fasttime fasttime requested a review from a team as a code owner April 28, 2025 18:40
snitin315
snitin315 previously approved these changes Apr 29, 2025
Copy link
Contributor

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I'll leave it open for others to review

@snitin315 snitin315 added the accepted There is consensus among the team that this change meets the criteria for inclusion label Apr 29, 2025
@snitin315 snitin315 moved this from Needs Triage to Second Review Needed in Triage Apr 29, 2025
@@ -2645,7 +2644,7 @@ class Linter {
/**
* Defines a new linting rule.
* @param {string} ruleId A unique rule identifier
* @param {Rule} rule A rule object
* @param {JSRuleDefinition} rule A rule object
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no type JSRuleDefinition defined or imported in this module?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e92fa18, thanks. I first thought to use JSRuleDefinition in this PR but since this is a legacy method parameter and it's typed as Rule.RuleModule in the TS types we can stick with that.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit 0958690 into main May 1, 2025
32 checks passed
@mdjermanovic mdjermanovic deleted the disambiguate-internal-types branch May 1, 2025 07:19
@github-project-automation github-project-automation bot moved this from Second Review Needed to Complete in Triage May 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion chore This change is not user-facing core Relates to ESLint's core APIs and features
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

3 participants