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.

Sign up for GitHub

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
Merged

Support rehype plugins that inject namespaced attributes #6243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5bb35f8
Support rehype plugins that inject namespaced attributes
bluwy Feb 14, 2023
98f6599
Fix rehype property casing
bluwy Feb 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/moody-cats-train.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@astrojs/mdx': patch
---

Support rehype plugins that inject namespaced attributes
4 changes: 2 additions & 2 deletions packages/integrations/mdx/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@
"dependencies": {
"@astrojs/markdown-remark": "^2.0.1",
"@astrojs/prism": "^2.0.0",
"@mdx-js/mdx": "^2.1.2",
"@mdx-js/rollup": "^2.1.1",
"@mdx-js/mdx": "^2.3.0",
"@mdx-js/rollup": "^2.3.0",
"acorn": "^8.8.0",
"es-module-lexer": "^0.10.5",
"estree-util-visit": "^1.2.0",
Expand Down
1 change: 1 addition & 0 deletions packages/integrations/mdx/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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.

remarkPlugins: [
// Ensure `data.astro` is available to all remark plugins
toRemarkInitializeAstroData({ userFrontmatter: frontmatter }),
Expand Down
35 changes: 35 additions & 0 deletions packages/integrations/mdx/test/mdx-plugins.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,20 @@ describe('MDX plugins', () => {
expect(selectRehypeExample(document)).to.not.be.null;
});

it('supports custom rehype plugins with namespaced attributes', async () => {
const fixture = await buildFixture({
integrations: [
mdx({
rehypePlugins: [rehypeSvgPlugin],
}),
],
});
const html = await fixture.readFile(FILE);
const { document } = parseHTML(html);

expect(selectRehypeSvg(document)).to.not.be.null;
});

it('extends markdown config by default', async () => {
const fixture = await buildFixture({
markdown: {
Expand Down Expand Up @@ -207,6 +221,23 @@ function rehypeExamplePlugin() {
};
}

function rehypeSvgPlugin() {
return (tree) => {
tree.children.push({
type: 'element',
tagName: 'svg',
properties: { xmlns:"http://www.w3.org/2000/svg" },
children: [
{
type: 'element',
tagName: 'use',
properties: { 'xLinkHref': '#icon' }
}
]
});
};
}

function recmaExamplePlugin() {
return (tree) => {
estreeVisit(tree, (node) => {
Expand Down Expand Up @@ -245,6 +276,10 @@ function selectRehypeExample(document) {
return document.querySelector('div[data-rehype-plugin-works]');
}

function selectRehypeSvg(document) {
return document.querySelector('svg > use[xlink\\:href]');
}

function selectRecmaExample(document) {
return document.querySelector('div[data-recma-plugin-works]');
}
18 changes: 9 additions & 9 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.