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

Support rehype plugins that inject namespaced attributes #6243

Merged
merged 2 commits into from
Feb 15, 2023

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Feb 14, 2023

Changes

Fix #5796

Thanks to mdx-js/mdx#2255, there's a way to properly render namespaced attributes injected by rehype plugins, without Astro needing to manually map camelCase namespaced attributes, to the normal ones.

With this PR, Astro should render <use xlink:href instead of <use xlinkhref

Testing

Added test to mdx integration

Docs

n/a. bug fix.

@changeset-bot
Copy link

changeset-bot bot commented Feb 14, 2023

🦋 Changeset detected

Latest commit: 98f6599

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 Feb 14, 2023
@@ -74,6 +74,7 @@ export default function mdx(partialMdxOptions: Partial<MdxOptions> = {}): AstroI
const { data: frontmatter, content: pageContent } = parseFrontmatter(code, id);
const compiled = await mdxCompile(new VFile({ value: pageContent, path: id }), {
...mdxPluginOpts,
elementAttributeNameCase: 'html',
Copy link

Choose a reason for hiding this comment

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

Wouldn’t this break React users?

Copy link
Member Author

Choose a reason for hiding this comment

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

The compiled MDX will be rendered by Astro's JSX runtime only so it would be fine. React JSX happens outside of MDX, where they are imported and compiled separately (without MDX).

Copy link

Choose a reason for hiding this comment

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

Oh right, that answers most of my Qs, but I do wonder whether there’s overlap.

How does the passing of props work between Astro’s own JSX runtime, and React’s JSX runtime? Maybe this point only relates to JSX written by the author tho, and the above option is for generated elements, not components.

Though, does Astro allow component overwrites? (h1 -> MyComponent)? In that case, the MyComponent currently gets className, and would now get class?

Copy link
Member Author

Choose a reason for hiding this comment

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

How does the passing of props work between Astro’s own JSX runtime, and React’s JSX runtime?

The props should be passed as-is from Astro's JSX to React. Yes this option only affects generated elements from rehype plugins.

Though, does Astro allow component overwrites? (h1 -> MyComponent)? In that case, the MyComponent currently gets className, and would now get class?

I believe you can with https://docs.astro.build/en/guides/markdown-content/#assigning-custom-components-to-html-elements. I'm not very sure about how the entire MDX flow works, but from a quick stackblitz test, it looks like className is passed down as is.

Copy link

@wooorm wooorm Feb 15, 2023

Choose a reason for hiding this comment

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

The stackblitz is on the stable release of astro right?
I am worried that this change breaks that (particularly when heading2.astro is instead a react component)

The props should be passed as-is from Astro's JSX to React

I think that's a problem. React expects className. This PR changes to pass class.

Copy link
Member Author

@bluwy bluwy Feb 15, 2023

Choose a reason for hiding this comment

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

Ah you're right. I've tested it locally now and it seems to be the case. We probably can't revert this as it's fixing a bug, I'll make another PR to make this a breaking minor.

It kinda is a bit awkward if the component is a React component, but I think it's a tradeoff to take for now, as the user can still workaround it.

EDIT: Just realized it has been released. We might need to revert, release, then release a new minor.

Copy link

Choose a reason for hiding this comment

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

This is likely breaking everyone that is using react, and "component overwrites".
This feature in MDX lets users specify their framework, through two options.
Those options need to be configured by the end user.
If Astro is in the middle, it would also need similar functionality: if it wants to hijack things, it needs to deal with different props expected by different frameworks

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's fine for Astro to take a stance on the default options since you can mix Astro and React components in component overwrites. It's also difficult to know the casing type ahead of time when compiling. Exposing another option isn't bulletproof too since casing wouldn't be right when using an Astro component override. So I think it's worth the tradeoff for now.

packages/integrations/mdx/test/mdx-plugins.test.js Outdated Show resolved Hide resolved
Copy link
Contributor

@bholmesdev bholmesdev left a comment

Choose a reason for hiding this comment

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

I'll trust MDX fixes this internally! Merge in the name of stability

@bluwy bluwy merged commit 4f6ecba into main Feb 15, 2023
@bluwy bluwy deleted the mdx-rehype-support branch February 15, 2023 09:06
@astrobot-houston astrobot-houston mentioned this pull request Feb 15, 2023
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.

MDX with rehype-mathjax creates xlinkhref attributes instead of xlink:href
4 participants