feat: add no-missing-link-fragments rule #380
Conversation
There was a problem hiding this comment.
Thank you for taking this up. This looks like a good start. I have added a few suggestions on documentation & few false postives. I think there are few more cases we should cover that MD051 handles as well:
-
HTML ids:
<a id="bookmark"></a> [Link](#bookmark) // valid
-
#top: HTML links to #top scroll to the top of a document.[Link](#top)
-
Custom fragment syntax used by GitHub to highlight specific content in a document.
[Link](#L20) // Valid [Link](#L19C5-L21C11) // Valid
SwetaTanwar
force-pushed
the
feat/no-missing-link-fragments
branch
from
67f7cc1 to
b051c1a
Compare
SwetaTanwar
force-pushed
the
feat/no-missing-link-fragments
branch
from
1301dba to
a36b406
Compare
|
@SwetaTanwar please double-check the merge conflict. |
|
@lumirlumir Can you please review the PR |
| if (customIdMatch) { | ||
| baseId = customIdMatch[1]; | ||
| } else { | ||
| const tempSlugger = new GithubSlugger(); | ||
| baseId = tempSlugger.slug(rawHeadingText); | ||
| } | ||
|
|
||
| const finalId = slugger.slug(baseId); | ||
| fragmentIds.add(ignoreCase ? finalId.toLowerCase() : finalId); |
There was a problem hiding this comment.
Can I ask why you're using slug twice?
If it's intended to handle the case I mentioned in the earlier comment, I'd like to suggest traversing the heading node's children, finding the text nodes, and combining them to make a valid base ID.
So how do we distinguish between underscores used intentionally and those used for Markdown formatting?
This distinction is important because the Markdown heading below generates a slug of test_, not test_:
test_
To handle this accurately, we can't rely on regex. Instead, we need to traverse to the deepest text nodes, extract their raw values, and reassemble them—effectively reconstructing the heading text without interpreting Markdown syntax manually. This ensures the slug generation mirrors GitHub's behavior and avoids false positives or negatives.
The current logic has a problem, since GitHubSlugger remembers duplicate heading texts.
For example:
# foo
# fooThe first heading foo will have the slug foo, and the second heading foo will have the slug foo-1.
If we declare GitHubSlugger only once, then we can track duplicate heading IDs and append -1, -2, etc., to the slug as we encounter duplicate headings.
But in this situation, if we declare GitHubSlugger each time, the tracking will fail and result in false positives or negatives.
So in conclusion, I'd recommend traversing the heading node, finding its children’s text nodes, and recombining them to get valid text without Markdown syntax.
It would be nice if you could reference the logic implemented in https://github.com/remarkjs/strip-markdown.
There was a problem hiding this comment.
Can you share any example of a false positive or negative, the following case works as expected:
<!-- eslint markdown/no-missing-link-fragments: "error" -->
# foo
## foo
[Link](#foo) // No error
[Link](#foo-1) // No error
[Link](#foo-2) // Error Link fragment '#foo-2' does not reference a heading or anchor in this documentThere was a problem hiding this comment.
Added test cases in ff82b1b
There was a problem hiding this comment.
I got it, fixed the behavior in 4c98730
| if ( | ||
| htmlText.startsWith("<!--") && | ||
| htmlText.endsWith("-->") | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Could you use a stricter check? Maybe we can use a negative lookbehind regex to find invalid comments.
Here’s another edge case:
<div>
<!-- comment -->
</div>References:
There was a problem hiding this comment.
Fixed in 757a61b
| const extractedId = match[1]; | ||
| const finalId = slugger.slug(extractedId); | ||
| fragmentIds.add( | ||
| ignoreCase ? finalId.toLowerCase() : finalId, |
There was a problem hiding this comment.
Could you check whether the id and name are inside <a> or <h1>, <h2>, ... tags?
Maybe we can reference the current implementation of another rule.
https://github.com/eslint/markdown/blob/main/src/rules/require-alt-text.js
There was a problem hiding this comment.
Already checking that, added more test cases in b9ec4da
|
@lumirlumir please re-review this when you can. |
|
@nzakas Sure, but I think my reviews haven’t been addressed yet. It seems like a bit more time might be needed. @SwetaTanwar If you don’t mind, and if you’re still working on it, could you mark this PR as a draft while you continue making progress? :) |
Prerequisites checklist
What is the purpose of this pull request?
This PR adds a new rule
no-missing-link-fragmentsto ensure there is no missing link fragment in the markdownWhat changes did you make? (Give an overview)
Added the
no-missing-link-fragmentsrule, along with documentation and tests.Related Issues
fixes #369
Is there anything you'd like reviewers to focus on?