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

MDX loader: linkify should process the md AST instead of the md string #9048

Closed
2 tasks done
slorber opened this issue Jun 8, 2023 · 7 comments · Fixed by #10168
Closed
2 tasks done

MDX loader: linkify should process the md AST instead of the md string #9048

slorber opened this issue Jun 8, 2023 · 7 comments · Fixed by #10168
Labels
apprentice Issues that are good candidates to be handled by a Docusaurus apprentice / trainee domain: markdown Related to Markdown parsing or syntax proposal This issue is a proposal, usually non-trivial change
Milestone

Comments

@slorber
Copy link
Collaborator

slorber commented Jun 8, 2023

Have you read the Contributing Guidelines on issues?

Motivation

Our current MDX loader code looks like this:

export default function markdownLoader(
  this: LoaderContext<DocsMarkdownOption>,
  source: string,
): void {
  const fileString = source;
  const callback = this.async();
  const options = this.getOptions();
  return callback(null, linkify(fileString, this.resourcePath, options));
}

The linkify process that turns [link](./xyz.md) to [link](/baseUrl/xyz-pathname) is based on string manipulations and regexes.

As a result, linkify has many edge case bugs related to the unability to properly resolve markdown relative file path links such as [](./relative-path.md)

We should instead do this with a Remark plugin after MDX has parsed the string.

See also:

Self-service

  • I'd be willing to do some initial work on this proposal myself.
@slorber slorber added proposal This issue is a proposal, usually non-trivial change status: needs triage This issue has not been triaged by maintainers and removed status: needs triage This issue has not been triaged by maintainers labels Jun 8, 2023
@Josh-Cena Josh-Cena added the domain: markdown Related to Markdown parsing or syntax label Jun 8, 2023
@Josh-Cena
Copy link
Collaborator

See previous attempt: #6261

@slorber
Copy link
Collaborator Author

slorber commented Jan 9, 2024

Another case it would fix is a bug when using relative md links on markdown images:

[text](./installation.mdx)

[![alt](/img/slash-introducing.svg)](./installation.mdx)

Only the first link resolves.

But reported here: #9720 (comment), that should also be solved by this proposed solution.

@slorber
Copy link
Collaborator Author

slorber commented Feb 22, 2024

Another case reported where markdown links resolution fails due to usage of inline code blocks with triple backticks (TIL this exists and is supported by MDX 😅). #9876

```something```

[test](test.md)

@slorber
Copy link
Collaborator Author

slorber commented Apr 25, 2024

Another use-case reported is when the link is multi-line:

<!-- This works -->
A [link to another page](other-page.md)

<!-- This doesn't work -->
A [link to
another page](other-page.md)

#9636
#10067

@slorber
Copy link
Collaborator Author

slorber commented Apr 25, 2024

Another use-case reported is when the link contains certain combinations of symbols:

[a](./migration/migrate-6.md#hooks) ([b](./migration/migrate-6.md#hooks))

[`Type1`](some_doc.md#type1)\<[`Type2`](some_doc.md#type2)\>

#9518
#9553
#10072

@pellyadolfo

This comment was marked as off-topic.

@slorber

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apprentice Issues that are good candidates to be handled by a Docusaurus apprentice / trainee domain: markdown Related to Markdown parsing or syntax proposal This issue is a proposal, usually non-trivial change
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants