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

fix(mdx): convert remark-images-to-component plugin to a rehype plugin #10697

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/sweet-goats-own.md
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
2 changes: 2 additions & 0 deletions packages/integrations/mdx/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,15 @@
},
"devDependencies": {
"@types/estree": "^1.0.5",
"@types/hast": "^3.0.3",
"@types/mdast": "^4.0.3",
"@types/yargs-parser": "^21.0.3",
"astro": "workspace:*",
"astro-scripts": "workspace:*",
"cheerio": "1.0.0-rc.12",
"linkedom": "^0.16.11",
"mdast-util-mdx": "^3.0.0",
"mdast-util-mdx-jsx": "^3.1.2",
"mdast-util-to-string": "^4.0.0",
"reading-time": "^1.5.0",
"rehype-mathjax": "^6.0.0",
Expand Down
6 changes: 3 additions & 3 deletions packages/integrations/mdx/src/plugins.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { rehypeApplyFrontmatterExport } from './rehype-apply-frontmatter-export.
import { rehypeInjectHeadingsExport } from './rehype-collect-headings.js';
import rehypeMetaString from './rehype-meta-string.js';
import { rehypeOptimizeStatic } from './rehype-optimize-static.js';
import { remarkImageToComponent } from './remark-images-to-component.js';
import { rehypeImageToComponent } from './rehype-images-to-component.js';

// Skip nonessential plugins during performance benchmark runs
const isPerformanceBenchmark = Boolean(process.env.ASTRO_PERFORMANCE_BENCHMARK);
Expand Down Expand Up @@ -52,7 +52,7 @@ function getRemarkPlugins(mdxOptions: MdxOptions): PluggableList {
}
}

remarkPlugins.push(...mdxOptions.remarkPlugins, remarkCollectImages, remarkImageToComponent);
remarkPlugins.push(...mdxOptions.remarkPlugins, remarkCollectImages);

return remarkPlugins;
}
Expand All @@ -74,7 +74,7 @@ function getRehypePlugins(mdxOptions: MdxOptions): PluggableList {
}
}

rehypePlugins.push(...mdxOptions.rehypePlugins);
rehypePlugins.push(...mdxOptions.rehypePlugins, rehypeImageToComponent);
Copy link
Member

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.

Copy link
Contributor Author

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 in rehype-images-to-component (only src is hard coded) so the Image component will receive those options. However, the consumer needs to target img instead of astro-image as node name. So, yes I see the breaking change now (and the major 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:

  • those who replicate existing rehype plugins to handle astro-image nodes
  • those who duplicate their logic to handle astro-image in addition to img

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 added decodeUri on src to handle paths with spaces.

Copy link
Contributor

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:

if (node.name === 'astro-image') {
	node.attributes.push({
		name: 'width',
		type: 'mdxJsxAttribute',
		value: '100',
	});
}

now after this change it will be like this:

if (node.tagName === 'img') {
	node.properties = {
		...node.properties,
		width: '100',
	};
}


if (!isPerformanceBenchmark) {
// getHeadings() is guaranteed by TS, so this must be included.
Expand Down
166 changes: 166 additions & 0 deletions packages/integrations/mdx/src/rehype-images-to-component.ts
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(' ')));
Copy link
Contributor

Choose a reason for hiding this comment

The 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 width: [100,200] it would become width="[100,200]" but you've solved this how I initially implemented the remark version's ability to add custom properties to the image component, by putting a condition on widths.

A condition will also need to be added for denisities since this is also an array option, and

if (Array.isArray(value)) {
			attrs.push(createArrayAttribute(prop, value));
		}

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 .split(' ') it themselves for its users

@Princesseuh What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 inferSize likely to cause problems as well? Since it is supposed to be a boolean, "false" will be evaluated as true in imageService, right? However I don't see how to convert the attribute's value to a boolean. The value property in MdxJsxAttribute is typed as string | MdxJsxAttributeValueExpression | null | undefined. Maybe we could use an empty string if the value is not "true", something like value: value === "true" ? value : "".

Copy link
Contributor

Choose a reason for hiding this comment

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

As of right now remote images are not processed by ![]() syntax, but if we did allow remote images inferSize would most likely be turned on by default and it would the need to be able to be turned off in a granular way with a remark/rehype plugin youre right but I don't think that we need to handle that here now

Copy link
Member

Choose a reason for hiding this comment

The 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`));
};
}
156 changes: 0 additions & 156 deletions packages/integrations/mdx/src/remark-images-to-component.ts

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
ASTRO_IMAGE_ELEMENT,
ASTRO_IMAGE_IMPORT,
USES_ASTRO_IMAGE_FLAG,
} from './remark-images-to-component.js';
} from './rehype-images-to-component.js';
import { type FileInfo, getFileInfo } from './utils.js';

// These transforms must happen *after* JSX runtime transformations
Expand Down
Loading
Loading