-
-
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
fix(mdx): convert remark-images-to-component plugin to a rehype plugin #10697
Changes from all commits
ac58f34
aeb0df0
2940a6b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@astrojs/mdx": major | ||
--- | ||
|
||
Replace remark-images-to-component with rehype-images-to-component to let users use additional rehype plugins for images |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,166 @@ | ||
import type { MarkdownVFile } from '@astrojs/markdown-remark'; | ||
import type { Properties, Root } from 'hast'; | ||
import type { MdxJsxAttribute, MdxjsEsm } from 'mdast-util-mdx'; | ||
import type { MdxJsxFlowElementHast } from 'mdast-util-mdx-jsx'; | ||
import { visit } from 'unist-util-visit'; | ||
import { jsToTreeNode } from './utils.js'; | ||
|
||
export const ASTRO_IMAGE_ELEMENT = 'astro-image'; | ||
export const ASTRO_IMAGE_IMPORT = '__AstroImage__'; | ||
export const USES_ASTRO_IMAGE_FLAG = '__usesAstroImage'; | ||
|
||
function createArrayAttribute(name: string, values: (string | number)[]): MdxJsxAttribute { | ||
return { | ||
type: 'mdxJsxAttribute', | ||
name: name, | ||
value: { | ||
type: 'mdxJsxAttributeValueExpression', | ||
value: name, | ||
data: { | ||
estree: { | ||
type: 'Program', | ||
body: [ | ||
{ | ||
type: 'ExpressionStatement', | ||
expression: { | ||
type: 'ArrayExpression', | ||
elements: values.map((value) => ({ | ||
type: 'Literal', | ||
value: value, | ||
raw: String(value), | ||
})), | ||
}, | ||
}, | ||
], | ||
bluwy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
sourceType: 'module', | ||
comments: [], | ||
}, | ||
}, | ||
}, | ||
}; | ||
} | ||
|
||
/** | ||
* Convert the <img /> element properties (except `src`) to MDX JSX attributes. | ||
* | ||
* @param {Properties} props - The element properties | ||
* @returns {MdxJsxAttribute[]} The MDX attributes | ||
*/ | ||
function getImageComponentAttributes(props: Properties): MdxJsxAttribute[] { | ||
const attrs: MdxJsxAttribute[] = []; | ||
|
||
for (const [prop, value] of Object.entries(props)) { | ||
if (prop === 'src') continue; | ||
|
||
/* | ||
* <Image /> component expects an array for those attributes but the | ||
* received properties are sanitized as strings. So we need to convert them | ||
* back to an array. | ||
*/ | ||
if (prop === 'widths' || prop === 'densities') { | ||
attrs.push(createArrayAttribute(prop, String(value).split(' '))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't realize when creating the initial issue for this that properties were all turned into "sanitizied" strings I sort of figured if given a hProperty by a remark plugin like A condition will also need to be added for
Can be removed because it will never be true based on how unified is sanitizing things But the reason that we implemented this Array.isArray method was to empower other image services that might make use of their own custom properties that want to accept arrays, instead of hard coding special cases for widths and denisities, but I can't exactly see another way to do this and the imageService if they wanted to make use of these properties could handing that property receiving a string of values and just @Princesseuh What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changes are ready (not pushed yet, I'm waiting for any other changes). Isn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As of right now remote images are not processed by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, considering that remote images are not supported at this time, it's fine to not have the best support for inferSize. |
||
} else { | ||
attrs.push({ | ||
name: prop, | ||
type: 'mdxJsxAttribute', | ||
value: String(value), | ||
}); | ||
} | ||
} | ||
|
||
return attrs; | ||
} | ||
|
||
export function rehypeImageToComponent() { | ||
return function (tree: Root, file: MarkdownVFile) { | ||
if (!file.data.imagePaths) return; | ||
|
||
const importsStatements: MdxjsEsm[] = []; | ||
const importedImages = new Map<string, string>(); | ||
|
||
visit(tree, 'element', (node, index, parent) => { | ||
if (!file.data.imagePaths || node.tagName !== 'img' || !node.properties.src) return; | ||
|
||
const src = decodeURI(String(node.properties.src)); | ||
|
||
if (!file.data.imagePaths.has(src)) return; | ||
|
||
let importName = importedImages.get(src); | ||
|
||
if (!importName) { | ||
importName = `__${importedImages.size}_${src.replace(/\W/g, '_')}__`; | ||
|
||
importsStatements.push({ | ||
type: 'mdxjsEsm', | ||
value: '', | ||
data: { | ||
estree: { | ||
type: 'Program', | ||
sourceType: 'module', | ||
body: [ | ||
{ | ||
type: 'ImportDeclaration', | ||
source: { | ||
type: 'Literal', | ||
value: src, | ||
raw: JSON.stringify(src), | ||
}, | ||
specifiers: [ | ||
{ | ||
type: 'ImportDefaultSpecifier', | ||
local: { type: 'Identifier', name: importName }, | ||
}, | ||
], | ||
}, | ||
], | ||
}, | ||
}, | ||
}); | ||
importedImages.set(src, importName); | ||
} | ||
|
||
// Build a component that's equivalent to <Image src={importName} {...attributes} /> | ||
const componentElement: MdxJsxFlowElementHast = { | ||
name: ASTRO_IMAGE_ELEMENT, | ||
type: 'mdxJsxFlowElement', | ||
attributes: [ | ||
...getImageComponentAttributes(node.properties), | ||
{ | ||
name: 'src', | ||
type: 'mdxJsxAttribute', | ||
value: { | ||
type: 'mdxJsxAttributeValueExpression', | ||
value: importName, | ||
data: { | ||
estree: { | ||
type: 'Program', | ||
sourceType: 'module', | ||
comments: [], | ||
body: [ | ||
{ | ||
type: 'ExpressionStatement', | ||
expression: { type: 'Identifier', name: importName }, | ||
}, | ||
], | ||
}, | ||
}, | ||
}, | ||
}, | ||
], | ||
children: [], | ||
}; | ||
|
||
parent!.children.splice(index!, 1, componentElement); | ||
}); | ||
|
||
// Add all the import statements to the top of the file for the images | ||
tree.children.unshift(...importsStatements); | ||
|
||
tree.children.unshift( | ||
jsToTreeNode(`import { Image as ${ASTRO_IMAGE_IMPORT} } from "astro:assets";`) | ||
); | ||
// Export `__usesAstroImage` to pick up `astro:assets` usage in the module graph. | ||
// @see the '@astrojs/mdx-postprocess' plugin | ||
tree.children.push(jsToTreeNode(`export const ${USES_ASTRO_IMAGE_FLAG} = true`)); | ||
}; | ||
} |
This file was deleted.
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.
Hmm, that's a bit annoying and makes this breaking. Previously, it was possible for a rehype plugin to change the options passed to the component, whereas right now it's just impossible. Wonder if anyone depended on that.
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.
From my understanding (don't hesitate to correct me), the properties on a node could be unrelated with HTML attributes. So I think it is still possible to pass options to the
Image
component. Using a rehype plugin (or even a remark plugin), the consumer could add the options as properties on images. The properties are then mapped to attributes inrehype-images-to-component
(onlysrc
is hard coded) so theImage
component will receive those options. However, the consumer needs to targetimg
instead ofastro-image
as node name. So, yes I see the breaking change now (and themajor
detected by changeset is justified).However, I don't see how we could fix the linked issue without this change... From what I saw with a quick search (so I certainly missed some use cases), there are essentially two use cases:
astro-image
nodesastro-image
in addition toimg
So I think this change is beneficial (if my understanding is correct). But it is definitely a breaking change.
That said, perhaps we need to check the correct formatting of the options if they exist. In comparison with the remark plugin, I had to add a special case for
widths
because it was a string instead of an array (based on existing tests) and I addeddecodeUri
onsrc
to handle paths with spaces.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.
yes previously a rehype plugin could set width to 100 like this:
now after this change it will be like this: