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

fix(utils): make Markdown link replacement much more rigorous #8927

Merged
merged 2 commits into from
Apr 27, 2023

Conversation

Josh-Cena
Copy link
Collaborator

Pre-flight checklist

Motivation

This makes our Markdown parsing much more in line with how it's actually handled at runtime.

Test Plan

Test links

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

Related issues/PRs

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 25, 2023
@Josh-Cena Josh-Cena added the pr: bug fix This PR fixes a bug in a past release. label Apr 25, 2023
@netlify
Copy link

netlify bot commented Apr 25, 2023

[V2]

Name Link
🔨 Latest commit 2c63371
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/644a9686feee390008eaf670
😎 Deploy Preview https://deploy-preview-8927--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 settings.

@github-actions
Copy link

github-actions bot commented Apr 25, 2023

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 70 🟢 97 🟢 100 🟢 100 🟠 89 Report
/docs/installation 🟠 73 🟢 100 🟢 100 🟢 100 🟠 89 Report

@github-actions
Copy link

github-actions bot commented Apr 25, 2023

Size Change: 0 B

Total Size: 1.01 MB

ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 101 kB
website/build/assets/css/styles.********.css 113 kB
website/build/assets/js/main.********.js 752 kB
website/build/index.html 41.3 kB

compressed-size-action

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.

LGTM if tests are passing 👍

Left some minor review comments


My thoughts recently: we should process the MD AST with a remark plugin to swap links instead of using regexes to preprocess the string content of the doc. It would be cleaner, simpler to handle only valid markdown links, and faster to run.

That's a much larger refactoring so I'm fine merging this for now.

Comment on lines +105 to +124
const linkTitlePattern = '(?:\\s+(?:\'.*?\'|".*?"|\\(.*?\\)))?';
const linkSuffixPattern = '(?:\\?[^#>\\s]+)?(?:#[^>\\s]+)?';
const linkCapture = (forbidden: string) =>
`((?!https?://|@site/)[^${forbidden}#?]+)`;
const linkURLPattern = `(?:${linkCapture(
'()\\s',
)}${linkSuffixPattern}|<${linkCapture('>')}${linkSuffixPattern}>)`;
const linkPattern = new RegExp(
`\\[(?:(?!\\]\\().)*\\]\\(\\s*${linkURLPattern}${linkTitlePattern}\\s*\\)|^\\s*\\[[^[\\]]*[^[\\]\\s][^[\\]]*\\]:\\s*${linkURLPattern}${linkTitlePattern}$`,
'dgm',
);
let mdMatch = linkPattern.exec(modifiedLine);
while (mdMatch !== null) {
// Replace it to correct html link.
const mdLink = mdMatch.groups!.filename!;
const mdLink = mdMatch.slice(1, 5).find(Boolean)!;
const mdLinkRange = mdMatch.indices!.slice(1, 5).find(Boolean)!;
if (!/\.mdx?$/.test(mdLink)) {
mdMatch = linkPattern.exec(modifiedLine);
continue;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Josh-Cena
Copy link
Collaborator Author

My thoughts recently: we should process the MD AST with a remark plugin to swap links instead of using regexes to preprocess the string content of the doc. It would be cleaner, simpler to handle only valid markdown links, and faster to run.

That would be solved by #6370

@slorber
Copy link
Collaborator

slorber commented Apr 27, 2023

That would be solved by #6370

Maybe, will have to study this more in-depth 😄


What I was thinking is more something like replacing:

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));
}

By

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

(not even sure it's a good idea because mdx-loader has a caching a creating a new option object might lead to weird things: maybe better to handle in plugin index.ts)

@slorber slorber merged commit 9e0b62f into main Apr 27, 2023
@slorber slorber deleted the fix-link-replacement branch April 27, 2023 15:57
@slorber slorber added the to backport This PR is planned to be backported to a stable version of Docusaurus label Sep 21, 2023
This was referenced Oct 19, 2023
@slorber slorber removed the to backport This PR is planned to be backported to a stable version of Docusaurus label Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Same file + extension in directory names breaks relative image assets
3 participants