-
-
Notifications
You must be signed in to change notification settings - Fork 832
Relax remark/rehype plugins usage #3332
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: f484b10 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for astro-starlight ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
|
|
|
Is this still being worked on? We depend on it for our documentation, because we would like to make our guides not so much hidden inside the starlight folder, to make contribution more accessible :) Right now we are working around it using symlinks, but it also creates problems of their own, see: #3470 |
Yes, this is not something we've been able to prioritize recently, but we do plan to get back to it and ship it in the next minor release hopefully. |
|
That sounds amazing, thank you so much :) |
* main: (87 commits) i18n(ja): sync outdated aside page (withastro#3465) i18n(ja): sync outdated icon reference (withastro#3467) chore(deps): update pnpm/action-setup action to v4.2.0 (withastro#3460) Showcase: Add Saucer (withastro#3454) i18n(ja): resources/themes (withastro#3442) add openstatus showcase (withastro#3436) [ci] release (withastro#3434) Add `head` config validation for `meta` tags with `content` (withastro#3380) Fix Astro Island margins in Markdown (withastro#3340) Ensure tab links span the full tab height (withastro#3427) Move `<summary>` to top of `<details>` for validity (withastro#3423) Run all tests on CI workflow changes (withastro#3433) [ci] format i18n(fr): small rewording and fixes in top-level pages (withastro#3432) i18n(fr): small rewording and fixes in `guides` (withastro#3431) i18n(fr): fix typo and links in `reference` files (withastro#3429) i18n(fr): small rewording in `components` (withastro#3430) [ci] release (withastro#3384) i18n(de): Update `page.lastUpdated` translation to be more in‐line with the English translation (withastro#3416) Remove invalid value attribute from `<select>` (withastro#3421) ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great and also nice to see the cleaned up code with the file path checks all centralised.
Left a few quick nots from stuff I spotted, but nothing major from me I don’t think.
-
We’d discussed glob support, but I agree that the simple directories approach makes sense for now.
-
I don’t have any particularly good ideas for the option naming. I tried to think of something that didn’t use the abbreviated “dirs”, but “directories” is long and “dir” is very common usage already, so I think it’s already quite good. It think the “processed” part of the name is nicely explicit too.
Thanks @HiDeoo!
packages/starlight/__e2e__/fixtures/basics/src/content.config.ts
Outdated
Show resolved
Hide resolved
| markdown: { | ||
| remarkPlugins: [ | ||
| ...starlightAsides({ | ||
| starlightConfig, | ||
| astroConfig: config, | ||
| useTranslations, | ||
| absolutePathToLang, | ||
| }), | ||
| ], | ||
| rehypePlugins: [ | ||
| rehypeRtlCodeSupport({ astroConfig: config }), | ||
| // Process headings and add anchor links. | ||
| ...starlightAutolinkHeadings({ | ||
| starlightConfig, | ||
| astroConfig: config, | ||
| useTranslations, | ||
| absolutePathToLang, | ||
| }), | ||
| ], | ||
| remarkPlugins: [...starlightRemarkPlugins(remarkRehypeOptions)], | ||
| rehypePlugins: [...starlightRehypePlugins(remarkRehypeOptions)], | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given we’re already abstracting these, do you think it might make sense to move the whole config behind an abstraction?
i.e. markdown: starlightMarkdownConfig(remarkRehypeOptions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, did not think of that. Just tried it locally and it definitely simplifies the code a bit in this file (packages/starlight/index.ts), I am not a big fan of the impact on tests.
For example, in some tests, we dynamically add plugins after Starlight's own plugins. With this change, this now means we need to first get the full markdown config from starlightMarkdownConfig(), then spread specific type of plugins and add our own:
const markdownConfig = starlightMarkdownConfig(
await createRemarkRehypePluginTestOptions(starlightConfig)
);
const processor = await createMarkdownProcessor({
...markdownConfig,
remarkPlugins: [
...markdownConfig.remarkPlugins,
// The restoration plugin is run after the asides and any other plugin that may have been
// injected by Starlight plugins.
remarkDirectivesRestoration,
],
});We also have some specific tests that currently focus on a type of plugin and expect to not run with the other type, e.g. we have a test only running remark plugins with the ## Environment variables (astro:env) input to test the aside directive handling. In this case, we expect the output to be <h2 id="environment-variables-astroenv">Environment variables (astro:env)</h2> which is simple enough to assert. But if we run with the full markdown config, the rehype plugins will also run and add heading links.
We could of course adjust the tests to account for that, but it makes the assertions more complex and less focused on the specific feature being tested. We could also override the rehype plugins to an empty array in these specific tests, but that feels a bit less practical.
Another alternative, instead of only exporting starlightMarkdownConfig(), would be also to keep the current exports of starlightRemarkPlugins() and starlightRehypePlugins() for tests, but this feels like we're not fully committing and testing this abstraction.
I have kept the exploration of this suggestion in a stash if ever needed, but I'm not sure it's worth it given the above. What do you think?
* main: (47 commits) feat: add some of trueberryless' new Starlight blogs to community content (withastro#3525) i18n(fr): update `resources/themes` (withastro#3526) Fix file filters added in withastro#3520 (withastro#3522) i18n(fr): update `guides/site-search.mdx` (withastro#3524) i18n(fr): update `resources/plugins.mdx` (withastro#3523) docs: add starlight-themes link (withastro#3510) i18n(de): update translation resources/plugins.mdx (withastro#3519) i18n(de): update translation guides/site-search.mdx (withastro#3518) Skip a11y CI checks for docs changes that aren’t tested anyway (withastro#3520) Speed up Linux e2e tests (withastro#3517) i18n(ru): update translations (withastro#3512) Tweak a11y e2e tests (withastro#3507) [ci] format docs(plugin): starlight-docsearch-typesense (withastro#3504) Add starlight-page-actions (withastro#3515) ci: update file icons (withastro#3511) [i18nIgnore] Match title case convention for components/using-components (withastro#3508) Disable Prettier on GitHub Actions workflow files (withastro#3509) [ci] release (withastro#3490) Update release workflow for OIDC (withastro#3500) ...
Co-authored-by: delucis <[email protected]>
Co-authored-by: delucis <[email protected]>
Co-authored-by: delucis <[email protected]>
|
Thanks for the review 🙌 Updated the PR with the suggested changes, and commented on one of them but happy to discuss further! I've also added a changeset for this new configuration option. |

Description
:::noteannotation not working with symlink #3470This draft PR is a follow-up to #3274 which restricted the usage of Starlight remark and rehype plugins to only Markdown and MDX content loaded using Starlight's
docsLoader(). As initially discussed and based on feedback, this PR relaxes that restriction using a newmarkdown.processedDirs(temporary name to be discussed) option that allows specifying additional directories where Markdown content should be processed by Starlight's remark and rehype plugins.A few key points:
file.data.starlight.skipor something like that with a symbol, which plugins running after could check and decide if processing should continue or not (this means 1 check for both remark and rehype).