gatsby: `gatsby-plugin-mdx`: `remark-external-links` generates invalid `rel`

Yes, agree it’s unfortunate.

Wonder if the Gatsby team would be open to changing this for Remark plugins (the wrapper bit can also be absorbed into gatsby-transformer-remark, I guess?).

Ah it looks like publishing additional “wrapper” npm packages to wrap every remark plugin is the recommended way of doing things 😞 At least in 2018 it was like this.

References:

• @KyleAMathews in https://github.com/gatsbyjs/gatsby/issues/2322#issuecomment-381345998
• @pieh in https://github.com/gatsbyjs/gatsby/issues/9103#issuecomment-429670139
• More background https://github.com/gatsbyjs/gatsby/issues/8009

So @wooorm I guess the process of writing extra wrapper packages you described the downsides about in https://github.com/gatsbyjs/gatsby/issues/22719#issuecomment-622305670 is actually recommended 😞

Ok, this has been resolved (CodeSandbox: https://codesandbox.io/s/serverless-snowflake-yo8mt?file=/package.json):

I’ve also closed #23505.

Thanks @pieh and @wooorm !

Ok, open! https://github.com/mdx-js/mdx/pull/1048

Edit: Great, that’s been merged now, so as soon as it’s published, gatsby-plugin-mdx can have its version bumped and hopefully everything just works!

Then this issue can be closed 🎉

Edit 2: PR in Gatsby open! #23700

Just thinking, maybe there’s a way to get correctness now and close this issue after all - maybe mdx would accept a PR for v1 for a rel fix similar to my https://github.com/mdx-js/mdx/pull/715 pull request?

That would allow gatsby-plugin-mdx to upgrade to this version without a breaking change and close this issue. Would also be ok to close / revert https://github.com/gatsbyjs/gatsby/pull/23505 at that time too I suppose!

a separate PR when it lands with more integration tests is a good idea, and a very welcome addition!

Alright sounds good, let’s do it then 👍

the quality of gatsby-remark plugins is sometimes lacking

I’d want to push towards cooperation instead of separation

“wrapper” or “copy-paste” plugins leads to the same problems grunt/gulp had, where plugins are out of date leading to issues that are already solved

…leads to doing the same things twice in silos

Agree on all points, would be nice if the API of Gatsby plugins such as gatsby-plugin-mdx nudged developers towards this - maybe with a different way to hook remark plugins into Gatsby (instead of writing a new plugin).

I’m not sure pointing people to an unmaintained wrapper plugin at 0.0.4 with other issues is a great solution either

Agree, it’s not a great solution. But having it break with no recourse for those affected is even worse.

I’ve experienced a lot of pain and incompatibility in Gatsby already, and I feel strongly about doing my part so that users don’t have to go through the same grief that I did.

Again, I totally prefer the method of pushing towards more correct and simple solutions, but if those solutions do not yet exist (or if a project is stuck on an old version for some reason), then it’s important to have a way of getting around it until the project can upgrade.

That PR is pretty big and hard to read already, so I think a separate PR when it lands with more integration tests is a good idea, and a very welcome addition! (I’d like to do more of that too, e.g., SVG/MathML and more integration tests!)

I’m heavily biased of course, but I think the quality of gatsby-remark plugins is sometimes lacking, and I’d want to push towards cooperation instead of separation. Some combination plugins of course make lots of sense (if the plugin is really integrating with Gatsby), but “wrapper” or “copy-paste” plugins leads to the same problems grunt/gulp had, where plugins are out of date leading to issues that are already solved (such as this one), and plugins that implement something in gatsby-remark where it could’ve been a remark plugin too, leads to doing the same things twice in silos.

So, while in this case technically a solution for this issue, I’m not sure pointing people to an unmaintained wrapper plugin at 0.0.4 with other issues is a great solution either.

Ah right, cool! Yeah I forgot about this too 😅

Thanks for the update! If you would guide me, I’d be happy to add this as a test case to your PR.

I guess that this PR can be closed when https://github.com/mdx-js/mdx/pull/1039 lands.

I’ve also updated #23505 to note that this applies to versions of MDX before v2.

I wasn’t 100% that it was MDX at first, but now I definitely am. I forgot about it, but you reported https://github.com/mdx-js/mdx/pull/715 which lead to https://github.com/mdx-js/mdx/issues/716. Problematic code is here: https://github.com/mdx-js/mdx/blob/9b4f192afd3255ac48780934dd848806912bff44/packages/mdx/mdx-hast-to-jsx.js#L19-L45.

The proper solution will be in MDX2, which I’m working on now!

@wooorm If you’re certain of this, I’m happy to open an issue in mdx-js/mdx with the details that you can provide. I got the impression that you weren’t certain what exactly the issue could be and that it needs some further investigation. That’s the only reason I haven’t reported yet.

So if you can provide me with the technical details of a reportable issue, I will do the reporting 😃

Hmm, maybe it’s a good idea instead to fix it in MDX? Also, why does gatsby-remark-external-links work? It’s a couple lines that essentially packs the normal remark plugin, and passes the same default rel: https://github.com/JLongley/gatsby-remark-external-links/blob/dc1bc39849a5ecd23e24ca95e954dec6cef8ee48/src/index.js. I don’t see why it would work.

Interestingly the code that MDX produces is this:

/* @jsx mdx */
import { mdx } from '@mdx-js/react';
/* @jsx mdx */
import DefaultLayout from "/Users/misiek/test/i22719/src/components/Layout.js"
export const _frontmatter = {};
const makeShortcode = name => function MDXDefaultShortcode(props) {
  console.warn("Component " + name + " was not imported, exported, or provided by MDXProvider as global scope")
  return <div {...props}/>
};

const layoutProps = {
  _frontmatter
};
const MDXLayout = DefaultLayout
export default function MDXContent({
  components,
  ...props
}) {
  return <MDXLayout {...layoutProps} {...props} components={components} mdxType="MDXLayout">


    <h1>{`Hello, world!`}</h1>
    <p><a parentName="p" {...{
        "href": "https://github.com/remarkjs/remark",
        "target": "_blank",
        "rel": ["nofollow", "noopener", "noreferrer"]
      }}>{`remark`}</a></p>

    </MDXLayout>;
}

;
MDXContent.isMDXComponent = true;

So it didn’t do anything to rel props - it kept it as is, and it looks like it’s React that insert , between items?

remark-html does make use of this list https://github.com/wooorm/property-information/blob/ac3452348fc694ec8bb2d3f0080b25a9dfa7f8e2/lib/html.js#L206 to know that rel need to be space separated values. I don’t know how much work it would need to make mdx handle cases like that similarly, but perhaps as workaround you could fork or vendor that remark plugin and make following change (just make it a string up front):

- props.rel = (rel || defaultRel).concat()
+ props.rel = (rel || defaultRel).join(` `)

Preliminary check on this:

The remark plugin does set rel “prop”/“attribute” to array of things ( https://github.com/remarkjs/remark-external-links/blob/0ba573ae3630a3f2f377d07b4295dfb964b24e8a/index.js#L54 )

I guess it’s the AST “stringifier” that is different for mdx here (it joins props with commas , and not spaces)

Will have to dig a bit into gatsby-plugin-mdx to where this is actually happening - is it in gatsby plugin or is it in mdx packages/dependencies