refactor: migrate type declarations to use the `@import` syntax by lumirlumir · Pull Request #367 · eslint/markdown

lumirlumir · 2025-05-09T06:59:08Z

Prerequisites checklist

I have read the contributing guidelines.

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 for MessageIds.

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 and MarkdownRuleDefinition 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 for MessageIds.

Related Issues

ref: #336 (comment)

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

Prerequisite

…gins

lumirlumir · 2025-05-09T11:16:10Z

tools/dedupe-types.js

I've followed the script from JSON.

tests/types/types.test.ts

src/index.js

src/types.ts

…or-align-rule-type-definitions-with-conventions-from-other-plugins

lumirlumir · 2025-06-23T09:06:31Z

The missing type checking issue reported in #432 has been addressed in this PR, so type checking now works as expected for all files.

nzakas · 2025-06-23T14:05:19Z

src/index.js

- * @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";


Can we just use the regular names? I think that makes things a lot easier to understand.

Suggested change

* @import { MarkdownRuleVisitor as MRV, MarkdownRuleDefinition as MRD, MarkdownRuleDefinitionTypeOptions } from "./types.js";

* @import { MarkdownRuleVisitor , MarkdownRuleDefinition, MarkdownRuleDefinitionTypeOptions } from "./types.js";

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)

Maybe instead of renaming the imported types we could import the whole types module as a namespace to avoid the naming collision? I.e:

/** * @import { Linter } from "eslint"; * @import * as Types from "./types.js"; * @typedef {Linter.RulesRecord} RulesRecord * @typedef {Types.MarkdownRuleDefinition} RuleModule * @typedef {Types.MarkdownRuleVisitor} MarkdownRuleVisitor */

And similarly for the MarkdownRuleDefinition typedef below.

Awesome, it works 👍

I've added a new commit a001891

nzakas · 2025-06-23T14:06:03Z

src/index.js

+ * @typedef {MRV} MarkdownRuleVisitor
+ */
+
+/**
+ * @typedef {MRD<Options>} MarkdownRuleDefinition<Options>


Then I don't think we need these.

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.

Sounds good. Because the next release will be a major release, this is the time to get all the breaking changes in. 👍

I've made a separate PR #446 👍

src/rules/no-duplicate-definitions.js

lumirlumir · 2025-06-25T13:40:08Z

src/index.js

+ */
+
+/**
+ * @typedef {Types.MarkdownRuleDefinition<Options>} MarkdownRuleDefinition<Options>


Suggested change

* @typedef {Types.MarkdownRuleDefinition<Options>} MarkdownRuleDefinition<Options>

* @typedef {Types.MarkdownRuleDefinition<Options>} MarkdownRuleDefinition<Options>

This line should be separated from the typedefs above; otherwise, it causes a type error like the one below:

fasttime

LGTM, thanks! Leaving open for @nzakas to verify before merging.

snitin315

LGTM

JoshuaKGoldberg · 2025-06-26T12:15:47Z

@eslint/markdown/types ... already used in the CSS and JSON plugins

👋 I'm catching up on this PR now and there's a lot of context. But I can't find where the original decision on that was made. Was there a driving reason to put types in the specific /types export? This is pretty unusual.

fasttime · 2025-06-26T21:16:40Z

@eslint/markdown/types ... already used in the CSS and JSON plugins

👋 I'm catching up on this PR now and there's a lot of context. But I can't find where the original decision on that was made. Was there a driving reason to put types in the specific /types export? This is pretty unusual.

@JoshuaKGoldberg There was a discussion in eslint/json#84. Do you know a better solution? I have to admit I haven't seen our approach used anywhere else.

JoshuaKGoldberg · 2025-06-26T21:27:16Z

Ah, I'd missed that PR. Darn.

Generally, projects try to in order of preference:

Not auto-generate types, so they can be directly exported from the root export
Have type auto-generation always run before publish, so they can still be exported from the root export

From eslint/json#82 (comment):

there's no easy way to just say "export this type"

Just to confirm, is the issue that index.js needs to export types from its sibling types.ts - and there's no way in TypeScript's JSDoc support to "re-export" a type in a JS file?

I haven't dug too deep, but the first thing that comes to mind is injecting an \nexport type * from "./types.d.ts"; at the bottom of the dist/*/index.d.ts files. That would do it in a pinch.

lumirlumir · 2025-06-28T10:17:15Z

@fasttime @JoshuaKGoldberg

Hi everyone, is there anything I need to do on my side? If so, I’d be happy to take a look.

JoshuaKGoldberg · 2025-06-30T16:02:40Z

I don't know what the right next step is 😅. #453 is a reference for the alternate strategy of adding a re-export line.

I also asked for help in the TypeScript Discord to see if there's a better way: https://discord.com/channels/508357248330760243/1389274620212809829

Edit: and Bluesky, https://bsky.app/profile/did:plc:hwtki3j7oghodc7h6gqnrtro/post/3lsu2bsv4gs2w

…nventions-from-other-plugins

lumirlumir · 2025-07-01T15:19:03Z

@JoshuaKGoldberg Thanks for the references!

I’ve explored some alternatives, but I think #453 is the best solution for now 👍

@nzakas I’d appreciate it if you could take a final look when you have time 😄

(FYI, I’ve resolved the merge conflicts, so the previous review has gone stale)

nzakas · 2025-07-02T15:06:29Z

Given that we've already followed the /types approach in @eslint/json, I think it's fine to keep it here. We can always revisit later if we decide there's a cleaner approach and then apply that all of the plugins.

refactor: align rule type definitions with conventions from other plu…

2ae787e

…gins

eslint-github-bot bot added the chore label May 9, 2025

lumirlumir marked this pull request as draft May 9, 2025 06:59

lumirlumir added this to Triage May 9, 2025

github-project-automation bot moved this to Needs Triage in Triage May 9, 2025

lumirlumir moved this from Needs Triage to Implementing in Triage May 9, 2025

lumirlumir added 7 commits May 9, 2025 16:08

wip

d9b4be6

wip: update dedupe-types

f023049

wip: update rules type import path

c65d558

wip: complete migration

7e73c58

wip: fix errors

bfb7894

wip: cleanup

31d2a1b

wip: complete migration

e66c951

lumirlumir commented May 9, 2025

View reviewed changes

wip

1ead275

lumirlumir marked this pull request as ready for review May 9, 2025 11:26

lumirlumir requested a review from fasttime May 9, 2025 11:27

lumirlumir changed the title ~~refactor: align rule typedefs with conventions from other plugins~~ refactor: migrate type declarations to use the @import syntax May 9, 2025

lumirlumir self-assigned this May 9, 2025

nzakas reviewed May 9, 2025

View reviewed changes

src/index.js Outdated Show resolved Hide resolved

src/types.ts Outdated Show resolved Hide resolved

lumirlumir marked this pull request as draft May 10, 2025 08:45

lumirlumir mentioned this pull request May 14, 2025

Change Request: Stop rolling up package #374

Closed

1 task

lumirlumir mentioned this pull request May 26, 2025

fix!: remove rollup and extraneous types #383

Merged

1 task

lumirlumir mentioned this pull request Jun 9, 2025

feat: add no-missing-link-fragments rule #380

Merged

1 task

lumirlumir added 6 commits June 21, 2025 17:23

Merge branch 'main' of https://github.com/eslint/markdown into refact…

b1e6298

…or-align-rule-type-definitions-with-conventions-from-other-plugins

wip: revert type testing

6daf0a6

wip: update index.js

200a2e9

wip: update fenced-code-language.js

d047161

wip: update heading-increment.js

1c62c89

wip: update no-bare-urls

ffa31ed

Merge branch 'main' of https://github.com/eslint/markdown into refact…

c01f54e

…or-align-rule-type-definitions-with-conventions-from-other-plugins

nzakas reviewed Jun 23, 2025

View reviewed changes

wip: extra blank line

47f06b7

lumirlumir requested a review from nzakas June 25, 2025 05:54

lumirlumir added the accepted label Jun 25, 2025

wip: address comment

a001891

lumirlumir commented Jun 25, 2025

View reviewed changes

lumirlumir mentioned this pull request Jun 26, 2025

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

Merged

1 task

fasttime previously approved these changes Jun 26, 2025

View reviewed changes

fasttime moved this from Implementing to Second Review Needed in Triage Jun 26, 2025

snitin315 previously approved these changes Jun 26, 2025

View reviewed changes

Merge branch 'main' into refactor-align-rule-type-definitions-with-co…

7e3f031

…nventions-from-other-plugins

lumirlumir dismissed stale reviews from snitin315 and fasttime via 7e3f031 July 1, 2025 14:38

nzakas approved these changes Jul 2, 2025

View reviewed changes

nzakas merged commit 0afc924 into main Jul 2, 2025
21 checks passed

nzakas deleted the refactor-align-rule-type-definitions-with-conventions-from-other-plugins branch July 2, 2025 15:06

github-project-automation bot moved this from Second Review Needed to Complete in Triage Jul 2, 2025

lumirlumir mentioned this pull request Jul 16, 2025

fix: remove unused types from types.ts #462

Merged

1 task

This was referenced Jul 30, 2025

TODO lumirlumir/web-lumir.page#217

Open

Change Request: Export types from main entry point eslint/rewrite#403

Open

lumirlumir mentioned this pull request Sep 7, 2025

feat: export types from main entry point #520

Merged

1 task

	* @import { MarkdownRuleVisitor as MRV, MarkdownRuleDefinition as MRD, MarkdownRuleDefinitionTypeOptions } from "./types.js";
	* @import { MarkdownRuleVisitor , MarkdownRuleDefinition, MarkdownRuleDefinitionTypeOptions } from "./types.js";

	import markdown, {
	MarkdownSourceCode,
	MarkdownRuleDefinition,
	MarkdownRuleVisitor,
	type RuleModule,
	} from "@eslint/markdown";

	* @typedef {Types.MarkdownRuleDefinition<Options>} MarkdownRuleDefinition<Options>
	* @typedef {Types.MarkdownRuleDefinition<Options>} MarkdownRuleDefinition<Options>

Conversation

lumirlumir commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prerequisites checklist

What is the purpose of this pull request?

What changes did you make? (Give an overview)

Related Issues

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

Prerequisite

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lumirlumir commented Jun 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lumirlumir Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lumirlumir Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fasttime left a comment

Choose a reason for hiding this comment

Uh oh!

snitin315 left a comment

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg commented Jun 26, 2025

Uh oh!

fasttime commented Jun 26, 2025

Uh oh!

JoshuaKGoldberg commented Jun 26, 2025

Uh oh!

lumirlumir commented Jun 28, 2025

Uh oh!

JoshuaKGoldberg commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lumirlumir commented Jul 1, 2025

Uh oh!

nzakas commented Jul 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lumirlumir commented May 9, 2025 •

edited

Loading

lumirlumir Jun 23, 2025 •

edited

Loading

lumirlumir Jun 23, 2025 •

edited

Loading

JoshuaKGoldberg commented Jun 30, 2025 •

edited

Loading