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

Allow MDX users to choose when rehypeCollectHeadings runs #5646

Closed
wants to merge 9 commits into from

Conversation

delucis
Copy link
Member

@delucis delucis commented Dec 19, 2022

Changes

Astro processes Markdown & MDX files with a rehype plugin that injects and collects IDs for all heading elements. We can then make these available using the getHeadings() method when these files are used.

For historical reasons (#4181, #4248), the MDX integration ended up running this plugin first, before user plugins, where we run it last, after user plugins, for Markdown files. This makes heading IDs available to user rehype plugins that need them, but also makes it impossible for a user to override ID generation like they can for a Markdown file.

This PR adds a new collectHeadings option to the MDX integration options, offering users the option of when they’d like headings to be injected:

// astro.config.mjs

import mdx from '@astrojs/mdx';

export default {
  integrations: [
    mdx({
      collectHeadings: 'before', // default: 'after'
    }),
  ],
}

By default, this is set to 'after' matching the current Markdown behaviour. This makes this a breaking change. Users who want to preserve the current behaviour would need to set collectHeadings: 'before'.

Alternatives considered

  1. Flip the execution order to last and leave users to figure out how to get IDs for themselves like we do for Markdown. This is an option but because common solutions like rehype-slug may not work entirely correctly for MDX didn’t seem very considerate.

  2. Expose the rehypeCollectHeadings plugin directly and let users choose where to add it in their rehypePlugins. This would be the most flexible — users could even run it in the middle of their plugin pipeline — and could be done safely. But feels a little like exposing internals. Happy to return to this model if people think it could be preferable.

Testing

Added tests for:

  • being able to override ID generation with the default config
  • being able to intercept Astro-generated IDs with collectHeadings: 'before'

Docs

Added an entry for the new config item in the README — /cc @withastro/maintainers-docs

@delucis delucis requested a review from bholmesdev December 19, 2022 19:20
@delucis delucis requested a review from a team as a code owner December 19, 2022 19:20
@changeset-bot
Copy link

changeset-bot bot commented Dec 19, 2022

🦋 Changeset detected

Latest commit: 63aa87f

The changes in this PR will be included in the next version bump.

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

@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Dec 19, 2022
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Code LGTM! I think this is a fair change given all the thought that has gone into this. Would also love @bholmesdev's stamp of approval just in case.

@delucis delucis marked this pull request as draft December 19, 2022 23:50
@delucis
Copy link
Member Author

delucis commented Dec 19, 2022

Converting to a draft to prevent merging until @bholmesdev & I chat about our options.

@delucis
Copy link
Member Author

delucis commented Dec 20, 2022

Chatted with Ben and we settled on an alternative approach. Will open a new PR.

@delucis delucis closed this Dec 20, 2022
@delucis delucis mentioned this pull request Dec 20, 2022
5 tasks
@delucis delucis deleted the chris/mdx-slugs branch January 16, 2023 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants