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

ArmandPhilippot · 2024-04-05T14:28:33Z

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 · 2024-04-05T14:28:38Z

🦋 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 · 2024-04-05T20:48:23Z

packages/integrations/mdx/src/plugins.ts

@@ -74,7 +74,7 @@ function getRehypePlugins(mdxOptions: MdxOptions): PluggableList {
 		}
 	}

-	rehypePlugins.push(...mdxOptions.rehypePlugins);
+	rehypePlugins.push(...mdxOptions.rehypePlugins, rehypeImageToComponent);


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.

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.

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 · 2024-04-08T09:08:09Z

packages/integrations/mdx/src/rehype-images-to-component.ts

+		if (Array.isArray(value)) {
+			attrs.push(createArrayAttribute(prop, value));
+		} else if (prop === 'widths') {
+			attrs.push(createArrayAttribute(prop, String(value).split(' ')));


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?

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

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

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

OliverSpeir · 2024-04-08T09:09:17Z

Brilliant work, thank you for tackling this!

Princesseuh · 2024-04-11T09:52:04Z

packages/integrations/mdx/src/rehype-images-to-component.ts

+				importedImages.set(src, importName);
+			}
+
+			// Build a component that's equivalent to <Image src={importName} alt={node.alt} title={node.title} />


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 · 2024-04-11T10:21:12Z

packages/integrations/mdx/src/rehype-images-to-component.ts

+
+		if (Array.isArray(value)) {
+			attrs.push(createArrayAttribute(prop, value));
+		} else if (prop === 'widths') {


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

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

OliverSpeir · 2024-04-15T01:38:01Z

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 · 2024-04-15T13:36:25Z

@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 · 2024-04-16T13:50:46Z

@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

packages/integrations/mdx/src/rehype-images-to-component.ts

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 · 2024-04-16T15:27:12Z

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 · 2024-04-16T15:52:24Z

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 · 2024-04-17T03:49:35Z

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 · 2024-04-17T05:29:32Z

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 · 2024-04-17T06:10:34Z

I think we can handle that like this:

Generic remark plugins can attach to node.data.hProperties, and because it's generic, we know these properties will be standard.
Astro-specific rehype plugins can attach to node.data.astroImageProps, that contains the values as is, e.g. .widths = [100, 200] (plain JS arrays).
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 · 2024-04-17T06:17:54Z

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 · 2024-04-17T07:05:50Z

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 · 2024-04-17T08:45:10Z

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 · 2024-04-24T09:24:53Z

@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.

Image related rehype plugins was not working because the MDX integration used a remark plugin. Converting it to a rehype plugin allow us to keep the same behavior but after any user's plugins run. Fix withastro#10643

ArmandPhilippot · 2024-04-24T10:20:12Z

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

bluwy · 2024-04-24T11:46:38Z

Awesome thanks!

* fix(mdx): convert remark-images-to-component plugin to a rehype plugin (#10697) * Remove fs read for MDX transform (#10866) * Tag MDX component for faster checks when rendering (#10864) * Use unified plugin only for MDX transform (#10869) * Only traverse children and handle mdxJsxTextElement when optimizing (#10885) * Rename to `optimize.ignoreComponentNames` in MDX (#10884) * Allow remark/rehype plugins added after mdx to work (#10877) * Improve MDX optimize with sibling nodes (#10887) * Improve types in rehype-optimize-static.ts * Rename `ignoreComponentNames` to `ignoreElementNames` I think this better reflects what it's actually doing * Simplify plain MDX nodes in optimize option (#10934) * Format code * Minimize diff changes * Update .changeset/slimy-cobras-end.md Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca> --------- Co-authored-by: Armand Philippot <59021693+ArmandPhilippot@users.noreply.github.com> Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>

github-actions bot added the pkg: integration Related to any renderer integration (scope) label Apr 5, 2024

Princesseuh reviewed Apr 5, 2024

View reviewed changes

OliverSpeir reviewed Apr 8, 2024

View reviewed changes

OliverSpeir added pr: docs A PR that includes documentation for review pkg: mdx Issues pertaining to `@astrojs/mdx` integration labels Apr 8, 2024

Princesseuh reviewed Apr 11, 2024

View reviewed changes

github-actions bot removed the pr: docs A PR that includes documentation for review label Apr 11, 2024

OliverSpeir requested a review from bluwy April 16, 2024 13:50

bluwy reviewed Apr 16, 2024

View reviewed changes

packages/integrations/mdx/src/rehype-images-to-component.ts Show resolved Hide resolved

bluwy approved these changes Apr 16, 2024

View reviewed changes

ArmandPhilippot force-pushed the fix/convert-mdx-remark-to-rehype-plugin branch from 16de3ae to 0a8e8f2 Compare April 18, 2024 12:49

ArmandPhilippot added 3 commits April 24, 2024 11:54

fix(mdx): handle densities option in rehype-images-to-component

aeb0df0

chore(mdx): add a comment to explain the hard coded attributes

2940a6b

ArmandPhilippot force-pushed the fix/convert-mdx-remark-to-rehype-plugin branch from 0a8e8f2 to 2940a6b Compare April 24, 2024 10:04

ArmandPhilippot changed the base branch from main to mdx-v3 April 24, 2024 10:05

bluwy merged commit 20936a9 into withastro:mdx-v3 Apr 24, 2024
14 checks passed

ArmandPhilippot deleted the fix/convert-mdx-remark-to-rehype-plugin branch April 24, 2024 12:19

bluwy mentioned this pull request May 2, 2024

MDX integration v3 #10935

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ArmandPhilippot commented Apr 5, 2024

changeset-bot bot commented Apr 5, 2024 •

edited

Loading

Princesseuh Apr 5, 2024

ArmandPhilippot Apr 5, 2024

OliverSpeir Apr 8, 2024

OliverSpeir Apr 8, 2024

ArmandPhilippot Apr 8, 2024

OliverSpeir Apr 9, 2024

Princesseuh Apr 11, 2024

OliverSpeir commented Apr 8, 2024

Princesseuh Apr 11, 2024

Princesseuh Apr 11, 2024 •

edited

Loading

ArmandPhilippot Apr 11, 2024

OliverSpeir commented Apr 15, 2024

ArmandPhilippot commented Apr 15, 2024

OliverSpeir commented Apr 16, 2024

bluwy left a comment •

edited

Loading

OliverSpeir commented Apr 16, 2024

Princesseuh commented Apr 16, 2024 •

edited

Loading

bluwy commented Apr 17, 2024

Princesseuh commented Apr 17, 2024

bluwy commented Apr 17, 2024 •

edited

Loading

OliverSpeir commented Apr 17, 2024 •

edited

Loading

Princesseuh commented Apr 17, 2024

bluwy commented Apr 17, 2024 •

edited

Loading

bluwy commented Apr 24, 2024

ArmandPhilippot commented Apr 24, 2024

bluwy commented Apr 24, 2024

	// 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} />

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

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

Conversation

ArmandPhilippot commented Apr 5, 2024

Changes

Testing

Docs

changeset-bot bot commented Apr 5, 2024 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OliverSpeir commented Apr 8, 2024

Choose a reason for hiding this comment

Princesseuh Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OliverSpeir commented Apr 15, 2024

ArmandPhilippot commented Apr 15, 2024

OliverSpeir commented Apr 16, 2024

bluwy left a comment • edited Loading

Choose a reason for hiding this comment

OliverSpeir commented Apr 16, 2024

Princesseuh commented Apr 16, 2024 • edited Loading

bluwy commented Apr 17, 2024

Princesseuh commented Apr 17, 2024

bluwy commented Apr 17, 2024 • edited Loading

OliverSpeir commented Apr 17, 2024 • edited Loading

Princesseuh commented Apr 17, 2024

bluwy commented Apr 17, 2024 • edited Loading

bluwy commented Apr 24, 2024

ArmandPhilippot commented Apr 24, 2024

bluwy commented Apr 24, 2024

changeset-bot bot commented Apr 5, 2024 •

edited

Loading

Princesseuh Apr 11, 2024 •

edited

Loading

bluwy left a comment •

edited

Loading

Princesseuh commented Apr 16, 2024 •

edited

Loading

bluwy commented Apr 17, 2024 •

edited

Loading

OliverSpeir commented Apr 17, 2024 •

edited

Loading

bluwy commented Apr 17, 2024 •

edited

Loading