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

[karlhorky]

Description

Using remark-external-links with gatsby-plugin-mdx generates an invalid rel attribute: nofollow,noopener,noreferrer instead of nofollow noopener noreferrer, how it is supposed to be.

Usage with gatsby-plugin-mdx and remarkPlugins

Usage alone

Steps to reproduce

Add the following code to Runkit (you can use this link: https://npm.runkit.com/remark-external-links). This generates the correct rel.

var remark = require('remark')
var externalLinks = require("remark-external-links")
var html = require('remark-html')
var input = '[remark](https://github.com/remarkjs/remark)'
remark().use(externalLinks).use(html).processSync(input).toString()

Try out my gatsby-plugin-mdx sandbox at the link below. This generates an invalid rel:

gatsby-plugin-mdx Sandbox: https://codesandbox.io/s/empty-cdn-u31qb

Expected result

The correct rel should be generated.

Actual result

An invalid rel is generated.

Environment

System:
    OS: Linux 4.19 Debian GNU/Linux 10 (buster) 10 (buster)
    CPU: (12) x64 Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz
    Shell: 5.0.3 - /bin/bash
  Binaries:
    Node: 10.18.1 - /tmp/yarn--1585757330032-0.1053501887919337/node
    Yarn: 1.21.1 - /tmp/yarn--1585757330032-0.1053501887919337/yarn
    npm: 6.13.4 - /usr/local/bin/npm
  Languages:
    Python: 2.7.16 - /usr/bin/python
  npmPackages:
    gatsby: 2.20.9 => 2.20.9
    gatsby-plugin-mdx: ^1.0.23 => 1.0.23

cc @ChristopherBiscardi

[pieh]

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

[pieh]

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(` `)

[github-actions[bot]]

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 30 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

[karlhorky]

@wooorm or @johno, you wouldn't happen to be able to comment on this, would you?

Is this something to fix in gatsby-plugin-mdx or does this belong as an issue in another project?

[wooorm]

Think it's something in mdx. Hast uses arrays for attributes with lists, mdx naively assumes those are all space separated (they often are, but this one is comma separated?)

[wooorm]

Or: mdx dos not support space separated lists at all 🤔

[karlhorky]

Workaround

Use gatsby-remark-external-links instead of remark-external-links, under the gatsbyRemarkPlugins options key of gatsby-plugin-mdx:

// In your gatsby-config.js
plugins: [
  {
    resolve: `gatsby-plugin-mdx`,
    options: {
      gatsbyRemarkPlugins: [
        {
          resolve: `gatsby-remark-external-links`,
        },
      ],
    },
  },
],

[karlhorky]

I have opened a pull request warning of this behavior (#23505) in the gatsby-plugin-mdx readme.

[wooorm]

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.

[pieh]

It's bit weird - seems like github repository does not match to what is released on npm. On npm that package is using string and not array ( https://unpkg.com/browse/[email protected]/index.js ) and I guess that's why it actually "works"

[karlhorky]

Interesting, seems like https://github.com/JLongley/gatsby-remark-external-links/pull/6 introduced remark-external-links.

I've asked them to publish, would be interesting to see if it still works afterwards.

[wooorm]

It won’t, https://github.com/gatsbyjs/gatsby/issues/22719#issuecomment-619589827, it’s a bug in MDX

[karlhorky]

@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 :)

[wooorm]

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]

Here’s the fix on my PR for MDX 2

[karlhorky]

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.

[wooorm]

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.

[karlhorky]

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.

[karlhorky]

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!

[wooorm]

That should be possible! I’m sure John would be happy to cut a release early for that? Either adding the prop to the list of exceptions, or using hast-to-hyperscript like so to solve everything! (which looks a bit ugly but I wanted to do cleaning up later)

[karlhorky]

Looking at your comments in mdx-js/mdx#715 and considering pulling in property-information for this - specifically, the properties that are spaceSeparated in this file: lib/html.js

Then I can create the shouldBeStrings array from that list pretty easily.

Any downsides with this? If not, I'll give it a shot.

[wooorm]

I’d suggest using prop info instead of hardcoding the list for maintainability’s sake, property information is relatively frequently updated with new HTML attributes.

[wooorm]

But as the PR will land soonish, hardcoding for now is fine!

[karlhorky]

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

[karlhorky]

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 !

[karlhorky]

I've also opened an issue in gatsby-remark-external-links to be removed from Gatsby Plugins and/or marked as deprecated:

https://github.com/JLongley/gatsby-remark-external-links/issues/22

[karlhorky]

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 😞

[wooorm]

Ohh. I think that’s unfortunate. I see some upsides in doing that, but mostly short term, I feel like in the long run it would be better to not do wrappers.

[karlhorky]

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?).