-
Notifications
You must be signed in to change notification settings - Fork 80
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
Changes from 36 commits
2ae787e
d9b4be6
f023049
c65d558
7e73c58
bfb7894
31d2a1b
e66c951
1ead275
b1e6298
6daf0a6
200a2e9
d047161
1c62c89
ffa31ed
2e6f981
eb27cc1
a0db87f
3aea485
c2df307
89d8406
b046450
655d241
2dfb75f
36f64d4
5b992fe
6e6c436
cb574f4
c1f6c24
6cac104
af428d2
fa288c2
4f73f4d
be286e6
7de7180
c01f54e
47f06b7
a001891
7e3f031
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,5 @@ | ||||||||||||||
| /** | ||||||||||||||
| * @fileoverview Enables the processor for Markdown file extensions. | ||||||||||||||
| * @fileoverview Markdown plugin. | ||||||||||||||
| * @author Brandon Mills | ||||||||||||||
| */ | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -17,16 +17,18 @@ import rules from "./build/rules.js"; | |||||||||||||
| // Type Definitions | ||||||||||||||
| //----------------------------------------------------------------------------- | ||||||||||||||
|
|
||||||||||||||
| /** @typedef {import("eslint").Linter.RulesRecord} RulesRecord*/ | ||||||||||||||
| /** @typedef {import("eslint").Linter.Config} Config*/ | ||||||||||||||
| /** @typedef {import("eslint").ESLint.Plugin} Plugin */ | ||||||||||||||
| /** | ||||||||||||||
| * @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"; | ||||||||||||||
| * @typedef {Linter.RulesRecord} RulesRecord | ||||||||||||||
| * @typedef {MarkdownRuleDefinition} RuleModule | ||||||||||||||
| * @typedef {MRV} MarkdownRuleVisitor | ||||||||||||||
| */ | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * @typedef {MRD<Options>} MarkdownRuleDefinition<Options> | ||||||||||||||
|
||||||||||||||
| 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.
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.
Sounds good. Because the next release will be a major release, this is the time to get all the breaking changes in. 👍
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 made a separate PR #446 👍
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.
Uh oh!
There was an error while loading. Please reload this page.
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 definitionserror when running thetsccommand, since the same name is declared again in the following lines using the@typedefsyntax.(This redeclaration is intentional, for the reason I mentioned here.)
However, if we decide to remove these types from the
@eslint/markdownexport path and move them to the@eslint/markdown/typespath, then these lines can be safely removed. (ref: here)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.
Maybe instead of renaming the imported types we could import the whole
typesmodule as a namespace to avoid the naming collision? I.e:And similarly for the
MarkdownRuleDefinitiontypedef below.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.
Awesome, it works 👍
I've added a new commit a001891