Skip to content
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

feat(mdx-loader): Remark plugin to report unused MDX / Markdown anchors links #9512

Closed
wants to merge 4 commits into from

Conversation

OzakIOne
Copy link
Contributor

@OzakIOne OzakIOne commented Nov 7, 2023

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

As of today, Docusaurus only check broken links and numerous people would like Docusaurus to check broken anchors.

I've made a PoC remark plugin that aims to detect broken anchors and warn the user about it, so far here is the functionality implemented :

  • Detect broken anchors in the same file
  • Detect broken anchors in an other file
  • Detect broken anchors of headings having a custom id # Title {#custom-id}
  • Support mdx files with react pages

For the custom id it should be fairly easy to support it, the problem is just that in the jest tests the remark plugin for custom id doesn't seems to work, and implement the custom id in the rendered html, it just renders it as text

For the broken anchors related to an other .md file, I don't know how to implement it, I thought we could do it in this remark plugin by recursively checking .md files and checking the anchors however it doesn't seems to be the right way to do it.

For now the only way for users to check the anchors in other files is to setup a ci with remark validate links

Test Plan

Test links

yarn jest brokenAnchors --watch

Deploy preview: https://deploy-preview-9512--docusaurus-2.netlify.app/

Related issues/PRs

#3321

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 7, 2023
@OzakIOne OzakIOne changed the title feat(mdx-loader): Remark plugin to report unused MDX / Markdown ancho… feat(mdx-loader): Remark plugin to report unused MDX / Markdown anchors links Nov 7, 2023
Copy link

netlify bot commented Nov 7, 2023

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 15101cd
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/654a3ebc46cdea000872fe7d
😎 Deploy Preview https://deploy-preview-9512--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

github-actions bot commented Nov 7, 2023

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 58 🟢 98 🟢 100 🟢 100 🟠 89 Report
/docs/installation 🟠 74 🟢 98 🟢 100 🟢 100 🟠 89 Report
/docs/category/getting-started 🟠 67 🟢 100 🟢 100 🟢 90 🟠 89 Report
/blog 🟠 60 🟢 100 🟢 100 🟢 90 🟠 89 Report
/blog/preparing-your-site-for-docusaurus-v3 🟠 51 🟢 97 🟢 100 🟢 100 🟠 89 Report
/blog/tags/release 🟠 61 🟢 100 🟢 100 🟠 80 🟠 89 Report
/blog/tags 🟠 60 🟢 100 🟢 100 🟢 90 🟠 89 Report

@slorber slorber marked this pull request as draft November 10, 2023 17:34
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

The algorithm doesn't look good to me, and we already implemented a better broken anchor link checker

I thought maybe a remark plugin could be a good idea to give quicker feedback, but I don't think so anymore.

There would be no good way for this remark plugin to report cases such as:

<Link id="customId"/>

[link to custom id](#customId)

So, this remark plugin would report a false positive error while the other "slower" broken link checker would not report it.

Also I don't think we could support checking remote files efficiently.

It wouldn't be a very good experience to only support local anchors, and to support them less reliably than our existing solution

So I'm going to close this draft PR for now

Comment on lines +23 to +33
type NodeType = {
heading: 'heading';
link: 'link';
text: 'text';
};

const nodeType: NodeType = {
heading: 'heading',
link: 'link',
text: 'text',
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI using this TS is kind of equivalent and simpler:

const nodeType = {
  heading: 'heading',
  link: 'link',
  text: 'text',
} as const;

type NodeType = typeof nodeType;

visit<Parent, ['heading', 'link']>(
tree,
nodeTypes,
(directive: Heading | Link) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

headings and links are not directives (:x ::xy or :::xyz), they are just headings and links, or nodes

@slorber slorber closed this Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants