-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[MDX] Support remark-rehype options from Astro Markdown config #5427
Conversation
🦋 Changeset detectedLatest commit: 4151592 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 |
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.
Left a note on adding remarkRehype
to the MDX config. Great PR though, with tests to boot 🙌
@@ -71,6 +71,7 @@ export default function mdx(mdxOptions: MdxOptions = {}): AstroIntegration { | |||
// Note: disable `.md` (and other alternative extensions for markdown files like `.markdown`) support | |||
format: 'mdx', | |||
mdExtensions: [], | |||
remarkRehypeOptions: config.markdown.remarkRehype, |
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.
I'm noticing this applies remarkRehype
in all cases. To stay consistent with remark
and rehype
plugin configs, we should only apply this when mdxOptions.extendPlugins === 'markdown'
. I'd add a check for that here.
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.
I'd probably add remarkRehype
to the MdxOptions
as well, in keeping with how we mirror the markdown config for the MDX integration. A couple changes for this:
- Add
remarkRehype
to theMdxOptions
type - Spread
mdxOptions.remarkRehype
withconfig.markdown.remarkRehype
, checking thatextendPlugins
option I noted above - Add
remarkRehype
to the MDX README. Feel free to link to our existingremarkRehype
docs for simplicity.
A bit complex I know, and we will simplify these APIs in the future! Just want to make sure we stay consistent with extending Markdown configs into MDX.
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.
@bholmesdev, thanks for the feedback! See latest commits for an attempt to take care of this.
Three things I'm unsure of:
- Is
remarkRehype
of typeRemarkRehype
orRemarkRehypeOptions
? I would assume the latter, but themarkdown
options assume the former. - Should
mdxOptions.remarkRehype
be extended withconfig.markdown.remarkRehype
or the other way round? - Is the README helpful enough?
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.
Good questions!
- I'd go with whatever the
markdown
options assume. If the types don't line up with the MDX config, tryRemarkRehypeOptions
! - I think you would spread
config.markdown.remarkRehype
first,mdxOptions.remarkRehype
second. So MDX options override Markdown options if there's conflicts - With Sarah's suggestion, you're golden I think
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.
@bholmesdev, sounds good!
- After finding https://github.com/mdx-js/mdx/blob/aff6de4fad5955b804d51cd109248425d786fb1c/packages/mdx/lib/core.js#L7 I have kept
RemarkRehypeOptions
. - Fixed and additional tests added.
- Fixed. Thanks a lot, @sarah11918!
Co-authored-by: Sarah Rainsberger <[email protected]>
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.
These changes look great! Thanks so much 👍
Also looks like the automated test failures are unrelated. I'll try rerunning, or just merge if we feel confident enough.
Changes
Testing
See #4138 and
test/fixtures/mdx-astro-markdown-remarkRehype/src/pages/index.mdx
Docs
/cc @withastro/maintainers-docs for feedback!