Skip to content

Compatible Astro 5.9 new API renderMarkdown#3241

Closed
sgalcheung wants to merge 3 commits intowithastro:mainfrom
sgalcheung:support-render-remote-markdown
Closed

Compatible Astro 5.9 new API renderMarkdown#3241
sgalcheung wants to merge 3 commits intowithastro:mainfrom
sgalcheung:support-render-remote-markdown

Conversation

@sgalcheung
Copy link
Copy Markdown
Contributor

@sgalcheung sgalcheung commented Jun 11, 2025 •
edited
Loading

Description

Create a createMarkdownTestHelper method to reduce repeated creation of createMarkdownProcessor, and this method could initialize different parameters in the test method, so we get all tests passed!

before
image

after
image

Next, I will create a test to reproduce this bug, so that we can better verify the modified code logic.
Add a test(should correctly autolink headings in remote markdown content using rehypeAutolinkHeadings) to reproduce this bug

TypeError: Failed to parse Markdown file "undefined":
Cannot read properties of undefined (reading 'replace')
    at normalizePath (/Users/sgalcheung/Documents/....../starlight/packages/starlight/integrations/heading-links.ts:110:14)
    at transformer (/Users/sgalcheung/Documents/....../starlight/packages/starlight/integrations/heading-links.ts:34:8)
    at wrapped (file:///Users/sgalcheung/Documents/....../starlight/node_modules/.pnpm/trough@2.1.0/node_modules/trough/index.js:115:27)
    at next (file:///Users/sgalcheung/Documents/....../starlight/node_modules/.pnpm/trough@2.1.0/node_modules/trough/index.js:65:23)
    at done (file:///Users/sgalcheung/Documents/....../starlight/node_modules/.pnpm/trough@2.1.0/node_modules/trough/index.js:148:7)
    at then (file:///Users/sgalcheung/Documents/....../starlight/node_modules/.pnpm/trough@2.1.0/node_modules/trough/index.js:158:5)
    at wrapped (file:///Users/sgalcheung/Documents/....../starlight/node_modules/.pnpm/trough@2.1.0/node_modules/trough/index.js:136:9)
    at next (file:///Users/sgalcheung/Documents/....../starlight/node_modules/.pnpm/trough@2.1.0/node_modules/trough/index.js:65:23)
    at done (file:///Users/sgalcheung/Documents/....../starlight/node_modules/.pnpm/trough@2.1.0/node_modules/trough/index.js:148:7)
    at then (file:///Users/sgalcheung/Documents/....../starlight/node_modules/.pnpm/trough@2.1.0/node_modules/trough/index.js:158:5)

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Jun 11, 2025 •
edited
Loading

⚠️ No Changeset found

Latest commit: 2ed04f0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@netlify
Copy link
Copy Markdown

netlify bot commented Jun 11, 2025 •
edited
Loading

✅ Deploy Preview for astro-starlight ready!

Name Link
🔨 Latest commit 2ed04f0
🔍 Latest deploy log https://app.netlify.com/projects/astro-starlight/deploys/684aac575b25d40008a9f4d8
😎 Deploy Preview https://deploy-preview-3241--astro-starlight.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse		 1 paths audited
Performance: 100 (no change from production)
Accessibility: 100 (no change from production)
Best Practices: 100 (no change from production)
SEO: 100 (no change from production)
PWA: -
View the detailed breakdown and full score reports

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

@github-actions github-actions bot added the 🌟 core Changes to Starlight’s main package label Jun 11, 2025
@HiDeoo HiDeoo marked this pull request as draft June 11, 2025 16:53
@HiDeoo
Copy link
Copy Markdown
Member

HiDeoo commented Jun 11, 2025 •
edited
Loading

Thanks for the contribution.

I hope you won't mind, I converted your PR to a draft. I did not immediately notice your message at the end of the PR, thought it was ready for a review and got confused about why only tests were updated.

Make sure to switch it back to ready for review when needed 👍

@sgalcheung
Copy link
Copy Markdown
Contributor Author

sgalcheung commented Jun 12, 2025 •
edited
Loading

I found a clue as to why Astro set srcDir to undefined: they changed the Zod type definition of srcDir.

image image

This is influenced createTranslationSystemFromFs

starlight/packages/starlight/utils/translations-fs.ts

Line 21 in f98bad5

{ srcDir }: Pick<AstroConfig, 'srcDir'>,

The input before parsing is: string | undefined

The output is: URL

Looks like don't have to change type, just add an input constraint.

@sgalcheung
Copy link
Copy Markdown
Contributor Author

sgalcheung commented Jun 12, 2025 •
edited
Loading

I have an idea to support both local files and remote content. I think retrieving pageLang from the page’s LanguageSelect changed event is a better approach.

Maybe we can read URL routing instead of file routing. @delucis @HiDeoo
image

But this seems impossible to get the page route in the Vite context.

@HiDeoo
Copy link
Copy Markdown
Member

HiDeoo commented Jun 12, 2025

I did not have the time yet to take a close look at the issue, so I'm only guessing here but I assume the issue is related to the new renderMarkdown function available in content loader context. Starlight relies in many places in remark/rehype plugins reading the path of the file actually being processed, which I assume is not available when using the new API.

Again, still guessing here without having looked in detail into the issue, but I assume this could be a 2-step fixes:

  • Safe-guard our own code to handle such cases where the file path is not available and potentially skip some processing if we're not able to properly handle the content.
  • Maybe see if on the longer run, it wouldn't be possible from the Astro side to provide some extra metadata that could allow Starlight to process them just like regular content.

I think retrieving pageLang from the page’s LanguageSelect changed event is a better approach.

We are definitely in a different context when running such remark/rehype plugins so that would not be possible.

@sgalcheung
Copy link
Copy Markdown
Contributor Author

sgalcheung commented Jun 12, 2025

So, should I create another PR to temporarily skip rendering remote content in the next version?

@HiDeoo
Copy link
Copy Markdown
Member

HiDeoo commented Jun 12, 2025

This PR seems fine to address the linked issue at the moment I think 👍

@sgalcheung sgalcheung changed the title Support Astro 5.9 new API renderMarkdown Compatible Astro 5.9 new API renderMarkdown Jun 12, 2025
@sgalcheung
Copy link
Copy Markdown
Contributor Author

sgalcheung commented Jun 12, 2025

Fine, I changed this PR title, because it just skips the remote content, so the new API doesn't "support" it yet!

@sgalcheung sgalcheung marked this pull request as ready for review June 12, 2025 23:37
@HiDeoo HiDeoo mentioned this pull request Jul 3, 2025
Merged
@delucis
Copy link
Copy Markdown
Member

delucis commented Jul 11, 2025

Thanks again for the PR @sgalcheung — going to close this in favour of #3274 which will also fix this with a slightly different approach.

@delucis delucis closed this Jul 11, 2025
@sgalcheung sgalcheung mentioned this pull request Jul 14, 2025
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

🌟 core Changes to Starlight’s main package

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support Astro 5.9 new API renderMarkdown

3 participants

@sgalcheung @HiDeoo @delucis