-
Notifications
You must be signed in to change notification settings - Fork 73
refactor: migrate type declarations to use the @import
syntax
#367
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
base: main
Are you sure you want to change the base?
refactor: migrate type declarations to use the @import
syntax
#367
Conversation
tools/dedupe-types.js
Outdated
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've followed the script from JSON.
@import
syntax
…or-align-rule-type-definitions-with-conventions-from-other-plugins
I've updated this PR to incorporate the latest changes from #383. There are no longer any breaking changes in this PR. |
…or-align-rule-type-definitions-with-conventions-from-other-plugins
The missing type checking issue reported in #432 has been addressed in this PR, so type checking now works as expected for all files. |
* @typedef {import("./types.ts").MarkdownRuleDefinition<Options>} MarkdownRuleDefinition<Options> | ||
* @template {Partial<import("./types.ts").MarkdownRuleDefinitionTypeOptions>} [Options={}] | ||
* @import { Linter } from "eslint"; | ||
* @import { MarkdownRuleVisitor as MRV, MarkdownRuleDefinition as MRD, MarkdownRuleDefinitionTypeOptions } from "./types.js"; |
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.
Can we just use the regular names? I think that makes things a lot easier to understand.
* @import { MarkdownRuleVisitor as MRV, MarkdownRuleDefinition as MRD, MarkdownRuleDefinitionTypeOptions } from "./types.js"; | |
* @import { MarkdownRuleVisitor , MarkdownRuleDefinition, MarkdownRuleDefinitionTypeOptions } from "./types.js"; |
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 initially tried using the original name, but without an alias, it results in a duplicate type definitions
error when running the tsc
command, since the same name is declared again in the following lines using the @typedef
syntax.
(This redeclaration is intentional, for the reason I mentioned here.)
However, if we decide to remove these types from the @eslint/markdown
export path and move them to the @eslint/markdown/types
path, then these lines can be safely removed. (ref: here)
* @typedef {MRV} MarkdownRuleVisitor | ||
*/ | ||
|
||
/** | ||
* @typedef {MRD<Options>} MarkdownRuleDefinition<Options> |
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.
Then I don't think we need these.
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.
This is because the type tests in types.test.js
currently depend on these type definitions.
So, removing them in this PR would introduce a breaking change:
markdown/tests/types/types.test.ts
Lines 1 to 6 in 47f06b7
import markdown, { | |
MarkdownSourceCode, | |
MarkdownRuleDefinition, | |
MarkdownRuleVisitor, | |
type RuleModule, | |
} from "@eslint/markdown"; |
Personally, I think it would be better to keep this PR non-breaking and create a separate PR to remove these types and move them under the @eslint/markdown/types
path when importing.
Both the JSON and CSS plugins follow a similar approach, so it would make sense to organize the types this way:
- JSON plugin:
https://github.com/eslint/json/blob/3f754f7f5f8e182ce88fc65a909b58a0116d04ac/tests/types/types.test.ts#L3-L7 - CSS plugin:
https://github.com/eslint/css/blob/211bf21f3c72530e60105a4f89c708bcbb00fc82/tests/types/types.test.ts#L55
If it’s acceptable, I can open a separate PR with the breaking changes by tomorrow.
Prerequisites checklist
What is the purpose of this pull request?
Hello,
In this PR, I’ve migrated the type declarations to use the
@import
syntax and added support forMessageIds
.This change was briefly discussed in #336 (comment).
I believe this migration will help ensure consistent type declarations across plugin repositories.
However, one concern I have is that the type module path for
RuleModule
andMarkdownRuleDefinition
has changed from@eslint/markdown
to@eslint/markdown/types
. While this path is already used in the CSS and JSON plugins, I’m worried it could introduce a breaking change.I’d appreciate any suggestions on how to avoid making this a breaking change, if possible.
What changes did you make? (Give an overview)
I’ve migrated the type declarations to use the
@import
syntax and added support forMessageIds
.Related Issues
ref: #336 (comment)
Is there anything you'd like reviewers to focus on?
Prerequisite