Skip to content

Conversation

@lumirlumir
Copy link
Member

@lumirlumir lumirlumir commented Jul 14, 2025

Prerequisites checklist

What is the purpose of this pull request?

Hello,

While reviewing the types.ts file, I noticed that Message and RuleType are not used anywhere.

It seems these two types were not removed during the refactor.

I think removing unused types would be helpful, but I'm not sure if this should be considered a breaking change, since users can import types from types.ts via @eslint/markdown/types, and version 7 was just released.

On the other hand, since these types were likely included by mistake during a refactor, it might make sense to treat their removal as a fix.

I'd be interested to hear team's thoughts on how we should handle these unused types.

(Personally, I don't think these two types play an important role in the markdown plugins.)

What changes did you make? (Give an overview)

Related Issues

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

@JoshuaKGoldberg
Copy link
Contributor

In general, Knip is great at solving unused types and the like. Unfortunately it can't detect this specific issue because types.ts is an entry point per package.json > "exports". From Knip's perspective, anything that is exported from an entry point is "used". Sent: #466.

I suppose we could write a custom lint rule or similar for things that exist and are exported in one file, do not refer to existing types, and are not used elsewhere? That's quite niche.

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

✂️

@lumirlumir lumirlumir moved this from Needs Triage to Second Review Needed in Triage Jul 16, 2025
@fasttime
Copy link
Member

In general, Knip is great at solving unused types and the like. Unfortunately it can't detect this specific issue because types.ts is an entry point per package.json > "exports". From Knip's perspective, anything that is exported from an entry point is "used". Sent: #466.

I suppose we could write a custom lint rule or similar for things that exist and are exported in one file, do not refer to existing types, and are not used elsewhere? That's quite niche.

Exported types in src/types.ts are reflected in the generated file dist/esm/types.d.ts, and since that file is publicly exported, there is no way to determine automatically that some types in it are unused. In fact, all exported types could be imported and used by some consumers. Maybe a clearer description of the issue is to say that the Message and RuleType types are unnecessary because they duplicate types defined elsewhere, and they aren't used internally.

@lumirlumir
Copy link
Member Author

lumirlumir commented Jul 16, 2025

The Message type was moved to processor.js internally after I made this change: #367. So, that was my mistake for not removing it.

As for RuleType, I found that it was added in #268, but I noticed it wasn’t actually used at that time either. So, I’m not sure where this type is currently being used.

I've also searched for it on GitHub, but it looks like ESLint doesn’t use this type internally.

https://github.com/search?q=%40eslint%2Fmarkdown%2Ftypes&type=code

@fasttime
Copy link
Member

As for RuleType, I found that it was added in #268, but I noticed it wasn’t actually used at that time either. So, I’m not sure where this type is currently being used.

I think it's safe to remove RuleType in a regular fix since that type isn't documented or otherwise referenced in this plugin. There are also no indications that that type is used by externally.

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 466f80e into main Jul 17, 2025
23 checks passed
@fasttime fasttime deleted the fix-remove-unused-types-from-types.ts branch July 17, 2025 10:01
@github-project-automation github-project-automation bot moved this from Second Review Needed to Complete in Triage Jul 17, 2025
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.

4 participants