Conversation
src/index.js
Outdated
| */ | ||
| /** @typedef {MarkdownRuleDefinition} RuleModule */ | ||
| /** @typedef {import("./types.ts").MarkdownRuleVisitor} MarkdownRuleVisitor */ | ||
| /** @typedef {import("./types.ts").MarkdownRuleDefinition} RuleModule */ |
There was a problem hiding this comment.
| /** @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.
There was a problem hiding this comment.
This
RuleModuletype was originally introduced in #333 to align with the currentRule.RuleModuleimplementation ineslint
It looks like that type was already exported before #333. I can trace it back to @eslint/markdown v6.0.0:
It was later changed to match the definition of MarkdownRuleDefinition in #324.
However, a notable point is that the
RuleModuletype 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
RuleModuletype in@eslint/markdownor not.
Since we are going to have a major release, I think it's the right time to remove RuleModule 👍
There was a problem hiding this comment.
Sounds good to me 👍
I'll remove the RuleModule type and the related test cases once #367 is merged.
tests/types/types.test.ts
Outdated
| MarkdownSourceCode, | ||
| MarkdownRuleDefinition, | ||
| MarkdownRuleVisitor, | ||
| type RuleModule, |
There was a problem hiding this comment.
| type RuleModule, | |
| type RuleModule, |
Same with #446 (comment)
|
I've removed the |
github-project-automation
bot
moved this from Second Review Needed
to Complete
in Triage
fasttime
deleted the
fix-move-certain-types-to-import-them-from-eslint-markdown-types
branch
3 participants
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
*RuleDefinitionand*RuleVisitortypes 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
distfolders of the JSON and CSS plugins, they do not export*RuleDefinitionand*RuleVisitortypes from their main entries (e.g.,@eslint/json,@eslint/css).Instead, they export those types from
@eslint/json/typesand@eslint/css/types, respectively.One thing I'm unsure about is how we should handle the
RuleModuletype 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?