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

[ArmandPhilippot]

Fix #10643

Note: pnpm exec changeset detects the change as major. I think it is more of a patch. I still added the changeset.

Changes

The MDX integration was transforming images too early so it wasn't possible to use additional rehype plugins on images. So I converted the remark-images-to-component plugin with a rehype plugin (rehype-images-to-component).

Note: The widths attribute had numbers as value once converted to a MDX component. Now we have strings as value since we are receiving a string from node.properties. Not sure if it is important or if we need an extra parsing to convert the value to a number if the prop name is widths. Everything seems to work correctly.

Example

Before:

expression: {
  type: 'ArrayExpression',
  elements: [
    { type: 'Literal', value: 300, raw: '300' },
    { type: 'Literal', value: 600, raw: '600' },
  ],
},

Now:

expression: {
  type: 'ArrayExpression',
  elements: [
    { type: 'Literal', value: '300', raw: '300' },
    { type: 'Literal', value: '600', raw: '600' },
  ],
},

Testing

I wasn't sure how to add a test in the mdx package so I only tested with:

• pnpm run test
• a local project using pnpm link and by adding rehype-figure since it was mentioned in the issue.

Docs

I don't think an update to the doc is necessary. It was a bug that prevented the expected behavior. Now users can use rehype plugins for their images before the MDX integration convert the images to components.

[changeset-bot]

🦋 Changeset detected

Latest commit: 2940a6b

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

[Princesseuh]

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.

[ArmandPhilippot]

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.

[OliverSpeir]

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',
	};
}

[OliverSpeir]

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?

[ArmandPhilippot]

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 : "".

[OliverSpeir]

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

[Princesseuh]

Yeah, considering that remote images are not supported at this time, it's fine to not have the best support for inferSize.

[OliverSpeir]

Brilliant work, thank you for tackling this!

[Princesseuh]

Suggested change

	// Build a component that's equivalent to <Image src={importName} alt={node.alt} title={node.title} />
	// Build a component that's equivalent to <Image src={importName} {...attributes} />

[Princesseuh]

I hate this, but I guess there's no choices. As Oliver said, this needs to also handle densities.

I would like to see a comment on top of these to explain why it's hard coded

[ArmandPhilippot]

I just pushed without seeing the edit. I'm adding the comment now.

[OliverSpeir]

Excellent thanks for being so quick to respond @ArmandPhilippot

The only thing that's left, and I don't mind commit directly here if you'd rather that, but this PR #10754 is going into remark-images-to-component.ts and I think it will be merged before this one, as it is only a patch. So we probably want to get this functinality into our new rehype-images-to-component.ts

It's a pretty simple check for whether or not the url includes / and if it doesnt to append ./ and this would be done around line 92 of rehype-images-to-components.ts or if there's another way you'd rather it done that's also fine but we want to support img.png when img is in the same directory as whatever .mdx file is using it with ![](img.png)

[ArmandPhilippot]

@OliverSpeir I don't mind adding the changes to this PR. However its seems #10754 is not ready yet. Like bluwy says, it does not handle all the possible cases.

It's a pretty simple check for whether or not the url includes / and if it doesnt to append ./

Maybe I'm overthinking, but I think it is a little more complex than that. I see different possible paths format:

• ./a-relative-one.jpg or a-relative-one.jpg
• ./a-nested/relative-path.jpg or a-nested/relative-path.jpg
• ../a-relative-path-in-parent.jpg (or ../../a-relative-path-in-grand-parent.jpg and so on)
• /an/absolute/path.jpg
• ~/a-path-alias.jpg that can have different shape like @a-path-alias.jpg (or @a-nested/path-alias.jpg)

So src.includes('/') doesn't seem to be the right approach (it fails with a-nested/relative-path.jpg for example). I was thinking something like:

import { isAbsolute } from 'node:path';

// Not the best name since `image.jpg` is also a relative source.
const isRelativeSrc = (src: string) => src.startsWith('./') || src.startsWith('../')

const isPathAlias = (src: string) => "???"

const normalizedSrc = isRelativeSrc(src) || isAbsolute(src) || isPathAlias(src) ? src : `./${src}`;

But I'm having trouble finding a way to detect if it is a path alias since we cannot rely on the first character (~ could be @ or even another character).

Another approach could be to resolve the path (ie. resolve(file.dirname, src)) and to check if the file exist. If it does not, it could be a path alias since Node will not resolve it. But this doesn't seem ideal neither. So I keep thinking about it.

[OliverSpeir]

@ArmandPhilippot it looks like #10754 ended up moving the logic out of this plugin so we are off the hook, I think the actual resolve is definitely the right solution

So src.includes('/') doesn't seem to be the right approach (it fails with a-nested/relative-path.jpg for example).

Youre right, I definitely had under thought this solution originally

So moving forward when that PR is merged we can update the branch of this PR and have no worries, I'm going to request a review here from @bluwy as well then I think we're good

[bluwy]

From what I can tell, this looks good to me. However, I didn't know that we support serializing hast properties into MDX for the <Image /> component. Personally I don't think we should support it since that implies we endorse <img src="..." widths="...">, which widths is non-standard.

I would instead reserve a new space under node.data.astroImageProps (perhaps) so we can control what it means. Since this is a major, maybe worth doing now? But I'm not sure if that's worth changing now if we have an ecosystem that already depends on it.

[OliverSpeir]

implies we endorse <img src="..." widths="...">, which widths is non-standard.

The actual getImage() call that is used to optimize the images will sanitize these properties

[Princesseuh]

From what I can tell, this looks good to me. However, I didn't know that we support serializing hast properties into MDX for the <Image /> component. Personally I don't think we should support it since that implies we endorse <img src="..." widths="...">, which widths is non-standard.

I would instead reserve a new space under node.data.astroImageProps (perhaps) so we can control what it means. Since this is a major, maybe worth doing now? But I'm not sure if that's worth changing now if we have an ecosystem that already depends on it.

The reason for doing it this way is that people can use already existing plugins to add attributes vs an Astro specific one. It wasn't like this a few versions ago (it was closer to what you suggested) and people often complained about remark plugins not working. I wanted to avoid needing a thousand astro-remark-image-attributes like Gatsby ended up having, ha.

[bluwy]

The actual getImage() call that is used to optimize the images will sanitize these properties

My point is that by allowing these properties to exist in hProperties in the first place (and later hast properties), we're saying that users can put non-standard attributes on img elements, which we later give a different meaning by transforming it for getImage/<Image />. I don't think we should overload hProperties and properties like this.

The reason for doing it this way is that people can use already existing plugins to add attributes vs an Astro specific one. It wasn't like this a few versions ago (it was closer to what you suggested) and people often complained about remark plugins not working. I wanted to avoid needing a thousand astro-remark-image-attributes like Gatsby ended up having, ha.

I think we should continue to support remark/rehype plugins that add additional attributes. But if the attribute is an Astro specific one (which means the remark/rehype plugin isn't general purpose in the first place), I think we should have the Astro specific props in a different place instead of overloading hProperties/properties. That way, we also don't need to deal with hast sanitizing/serializing things differently, we can use the values as is.

[Princesseuh]

Hmm, how would we handle this? The situation is the following:

• A generic remark plugin to add attributes is used (ex: remark-image-attributes, remark-img-attr etc)
• Any property can be an image service attribute (not just quality, format, widths, densities etc, it's up to the image service)
• The image service handles which attributes to use in rendering the img tag vs which to use for transformation (Image allows all attributes that img allows)

I don't see how we can distinguish outside of having a list of all attributes that img supports, but that'd still require fishing them out of the node's attributes, so they're still at the wrong place.

The only solution I see is having a astro-remark-image-attributes that puts all attributes in a special property, which is definitely fine, but annoying to need a specific plugin (which we should most likely maintain ourselves)

[bluwy]

I think we can handle that like this:

1. Generic remark plugins can attach to node.data.hProperties, and because it's generic, we know these properties will be standard.
2. Astro-specific rehype plugins can attach to node.data.astroImageProps, that contains the values as is, e.g. .widths = [100, 200] (plain JS arrays).
3. When constructing the <Image ...>, we do something like this (or better optimized):

const str = `<Image
  {...{
    ...${JSON.stringify(node.properties)},
    ...${JSON.stringify(node.data.astroImageProps}
  }}
  src="..."
/>`

This way we don't need to check how node.properties serializes properties, and we don't have Astro-specific rehype plugins confused of if it should assign node.properties.widths: "100 200" or node.properties.widths: [100, 200]. node.data.astroImageProps will contain the special attributes we support and any overlap-standard properties if we want.

[OliverSpeir]

Astro-specific rehype plugins can attach to node.data.astroImageProps, that contains the values as is, e.g. .widths = [100, 200] (plain JS arrays).

Brilliant, I love it.

The only solution I see is having a astro-remark-image-attributes that puts all attributes in a special property, which is definitely fine, but annoying to need a specific plugin (which we should most likely maintain ourselves)

Also on board for this too, as the current plugin I have can be forked very easily

[Princesseuh]

But then generic plugins cannot add Astro specific properties. What people want to do is be able to use plugins that allow this kind of syntax:

![]() {widths=[200, 500], class="some-class"}

where they'll mix both attributes specific to Astro and generic ones per image. I'm fine with this solution, to be honest, but I'm not sure it quite answer user need

[bluwy]

Hmm, I didn't expect we want a generic remark plugin to pass along attributes like this. It makes the interpretation of widths=[200, 500] ambigious, e.g. Should it assign hProperties.widths [200, 500], or "[200, 500]", or "200 500", or "[200"? (Maybe there's a syntax to assign it the way we want, but the line starts to be blurry)

I agree that it's nice if we can rely on generic plugins to pass on to our <Image> component, but I feel like we may be stretching it and it's better to be clear when you're opting into a Astro specific feature. Maybe we should look into making some syntax like this first class too?

I think overall, I'm concern of the current design, but not blocking the PR if this is the design we want to go for now. We can re-think about it again if there are issues.

[bluwy]

@ArmandPhilippot I've discussed this with Erika and the team, and I'm ok with moving forward with this for now. My comments aren't quite a blocker but perhaps we can implement a few of the ideas non-breakingly in minors.

Can you help resolve the conflicts, and then change the base branch to https://github.com/withastro/astro/tree/mdx-v3 ? I'm also planning to do more breaking changes so I created a new feature branch for it.

[ArmandPhilippot]

@bluwy All done, the merge conflict has been resolved and I updated the base branch!

[bluwy]

Awesome thanks!