Skip to content

refactor: reduce usage of markdown splicing#690

Merged
dsanders11 merged 20 commits intomainfrom
am-i-cooking-chat
Dec 6, 2024
Merged

refactor: reduce usage of markdown splicing#690
dsanders11 merged 20 commits intomainfrom
am-i-cooking-chat

Conversation

@erickzhao
Copy link
Copy Markdown
Member

@erickzhao erickzhao commented Nov 28, 2024

Fixes #691
Fixes #682

Background

This pull request simplifies some of the pre-processing steps that we use to transform the raw documentation in electron/electron into the rendered HTML on the website.

graph TD
  F[Download raw documentation from Electron repository] --> A[Regex-Based Markdown Processing]
  A --> B[Custom MDX Transformer Plugins]
  B --> C[docusaurus-plugin-content-docs]

  subgraph E[run as pre-build npm script]
    A
    F
  end

  subgraph D[run within Docusaurus]
    B
    C
  end
Loading

The first pre-processing step involves making changes to the raw Markdown files on disk by using regex-based replacement in yarn pre-build. The replacements made are crude but necessary for edge cases that can't be parsed by Docusaurus' MDX parser.

For example, MDX's usage of JSX means that <img> tags need to be self-closing, while they don't need to be according to the HTML spec:

/**
* MDX requires <img> tags to be closed (e.g. <img/>).
* This fixer isn't perfect and only works for <img> tags that take up a whole line.
* @param line
*/
const noUnclosedImageTags = (line: string) => {
if (line.match(/^(<img[^>]+)(?<!\/)>$/)) {
return `${line.slice(0, -1)}/>`;
} else {
return line;
}
};

Problem

The fixLinks function attempts to normalize in the documentation by storing a hashmap of each file and its respective Markdown string contents. It then replaces every single Markdown link with the normalized link matching the file name. This storage mechanism causes problems when two or more documents have the same name under different paths (e.g. API documentation and a tutorial for some exported module).

This fixer function was added early in the website's development cycle (#3) and is useless for the vast majority of links in the docs.

However, fixLinks is necessary to get another one of our Markdown hacks working. Namely, inlineApiStructures adds the ability to insert API structures in other documents using an ?inline Markdown link.

This situation leaves us at an impasse where we have broken links in our docs with fixLinks enabled, but we can't fix the problem without breaking links in inlineApiStructures.

Solution

This PR tackles the problem by migrating the inlining process to Docusaurus' MDX rendering pipeline, which parses the raw Markdown into an AST and converts it into HTML.

At a high level, we already have multiple plugins that manipulate ASTs, one of which (api-structure-previews) stores API structure content in memory to render hover previews on API pages.

This PR reuses this architecture to separately inline the appropriate API structure content in the AST and removes fixLinks and inlineApiStructures entirely.

Additional notes

  • api-structure-previews now stores the MDAST for each structure instead of the raw string to facilitate inlining.
    • This change allows the MDX plugin to avoid converting MDAST -> String -> HTML. For this reason, we don't need the react-markdown library anymore.
    • Instead, we directly process MDAST -> HAST -> HTML, which is rendered in React using dangerouslySetInnerHTML.
  • During PR review, please skip the entire /docs/ directory. It switches all normalized links to their raw counterparts, which still work. Docusaurus does not throw any broken link errors on build times.

@erickzhao erickzhao requested a review from a team as a code owner November 28, 2024 01:00
@erickzhao erickzhao changed the title Am i cooking chat refactor: Am i cooking chat Nov 28, 2024
@erickzhao erickzhao changed the title refactor: Am i cooking chat refactor: reduce usage of markdown splicing Nov 28, 2024
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages bot commented Nov 28, 2024

Deploying electron-website with  Cloudflare Pages  Cloudflare Pages

Latest commit: c3d0a0e
Status: ✅  Deploy successful!
Preview URL: https://1becdb8f.electron-website.pages.dev
Branch Preview URL: https://am-i-cooking-chat.electron-website.pages.dev

View logs

Copy link
Copy Markdown
Member

@VerteDinde VerteDinde left a comment

Choose a reason for hiding this comment

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

Thanks for fixing these! 🙇‍♀️

@erickzhao erickzhao requested a review from dsanders11 December 2, 2024 23:20
@dsanders11
Copy link
Copy Markdown
Member

For example, MDX's usage of JSX means that <img> tags need to be self-closing, while they don't need to be according to the HTML spec:

This is a tangent, but that feels like a good candidate that we might be able to fix upstream with a linting rule.

Copy link
Copy Markdown
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

Partial review, just wanted to throw the requested changes on now since it's a security-related change. Will finish reviewing the rest tomorrow.

Copy link
Copy Markdown
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, I'm not able to find any regressions. Cases checked:

  • ✅ Inlines
    • ✅ Labels render
    • ✅ Hovers on referenced structures work
    • ✅ Nested inlines unfurl correctly
  • ✅ Hover previews
    • ✅ First header renders correctly
    • ✅ Labels render
    • ✅ Referenced structures are plain links and not previews on previews

Leaving two comments where I think we should add more detailed comments for posterity (you did a great job of commenting other parts of the code and improving the existing comments!), then ready to merge!

@erickzhao erickzhao requested a review from dsanders11 December 6, 2024 19:19
Copy link
Copy Markdown
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

Great refactor! :shipit:

@dsanders11 dsanders11 merged commit e22c3c3 into main Dec 6, 2024
@dsanders11 dsanders11 deleted the am-i-cooking-chat branch December 6, 2024 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Links point to the wrong place if markdown files have the same name Incorrect rendering of embedded API structure hover cards

3 participants