Skip to content

fix!: move certain types to import them from @eslint/markdown/types #446

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

Conversation

lumirlumir
Copy link
Member

@lumirlumir lumirlumir commented Jun 26, 2025

Prerequisites checklist

What is the purpose of this pull request?

Hello.

The purpose of this PR is to align the type import path with the JSON and CSS plugins.

This is a breaking change, and it was discussed in #367 (comment) Also, it serves as a prerequisite for #367.

Currently, we import *RuleDefinition and *RuleVisitor types directly from the main entry point, like @eslint/markdown.
This PR moves those types under @eslint/markdown/types.

Here are some references:

Looking at the dist folders of the JSON and CSS plugins, they do not export *RuleDefinition and *RuleVisitor types from their main entries (e.g., @eslint/json, @eslint/css).
Instead, they export those types from @eslint/json/types and @eslint/css/types, respectively.


One thing I'm unsure about is how we should handle the RuleModule type in the Markdown plugin.
I've left a detailed comment regarding this concern.

What changes did you make? (Give an overview)

Related Issues

Ref: #367 (comment)

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

src/index.js Outdated
*/
/** @typedef {MarkdownRuleDefinition} RuleModule */
/** @typedef {import("./types.ts").MarkdownRuleVisitor} MarkdownRuleVisitor */
/** @typedef {import("./types.ts").MarkdownRuleDefinition} RuleModule */
Copy link
Member Author

@lumirlumir lumirlumir Jun 26, 2025

Choose a reason for hiding this comment

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

Suggested change
/** @typedef {import("./types.ts").MarkdownRuleDefinition} RuleModule */
/** @typedef {import("./types.ts").MarkdownRuleDefinition} RuleModule */

I'm not sure whether we should remove the RuleModule type or not.

This RuleModule type was originally introduced in #333 to align with the current Rule.RuleModule implementation in eslint:

However, a notable point is that the RuleModule type does not exist in the JSON or CSS plugins.

So, I think a decision from @eslint/eslint-tsc is needed on whether we should retain the RuleModule type in @eslint/markdown or not.

Copy link
Member

Choose a reason for hiding this comment

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

This RuleModule type was originally introduced in #333 to align with the current Rule.RuleModule implementation in eslint

It looks like that type was already exported before #333. I can trace it back to @eslint/markdown v6.0.0:

https://github.com/eslint/markdown/blob/v6.0.0/src/index.js#L22

It was later changed to match the definition of MarkdownRuleDefinition in #324.

However, a notable point is that the RuleModule type does not exist in the JSON or CSS plugins.

So, I think a decision from @eslint/eslint-tsc is needed on whether we should retain the RuleModule type in @eslint/markdown or not.

Since we are going to have a major release, I think it's the right time to remove RuleModule 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good to me 👍

I'll remove the RuleModule type and the related test cases once #367 is merged.

@@ -1,11 +1,14 @@
import markdown, {
MarkdownSourceCode,
MarkdownRuleDefinition,
MarkdownRuleVisitor,
type RuleModule,
Copy link
Member Author

@lumirlumir lumirlumir Jun 26, 2025

Choose a reason for hiding this comment

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

Suggested change
type RuleModule,
type RuleModule,

Same with #446 (comment)

@lumirlumir lumirlumir marked this pull request as ready for review June 26, 2025 07:44
@lumirlumir lumirlumir requested a review from a team June 26, 2025 07:44
@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Jun 26, 2025
@lumirlumir lumirlumir marked this pull request as draft June 28, 2025 11:40
@nzakas nzakas moved this from Needs Triage to Implementing in Triage Jul 2, 2025
@lumirlumir
Copy link
Member Author

lumirlumir commented Jul 3, 2025

I've removed the RuleModule type and the related test cases, referencing the JSON test implementation 👍

@lumirlumir lumirlumir requested review from fasttime and nzakas July 3, 2025 10:32
@lumirlumir lumirlumir marked this pull request as ready for review July 3, 2025 10:32
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Would like @fasttime to review before merging.

@nzakas nzakas moved this from Implementing to Second Review Needed in Triage Jul 8, 2025
Copy link
Member

@fasttime fasttime 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!

@fasttime fasttime merged commit 0dc6220 into main Jul 10, 2025
23 checks passed
@github-project-automation github-project-automation bot moved this from Second Review Needed to Complete in Triage Jul 10, 2025
@fasttime fasttime deleted the fix-move-certain-types-to-import-them-from-eslint-markdown-types branch July 10, 2025 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

3 participants