Rename metaString to metastring to avoid React warning

[hugmanrique]

Currently, whenever a code block (pre > code) is rendered by MDX to the DOM, React prints this warning:

index.js:2178 Warning: React does not recognize the `metaString` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `metastring` instead. If you accidentally passed it from a parent component, remove it from the DOM element.
    in code (created by MDXTag)
    in MDXTag (created by Context.Consumer)
    in Unknown (at this-is-the-slug.mdx:17)
    in pre (created by MDXTag)
    in MDXTag (created by Context.Consumer)
    in Unknown (at this-is-the-slug.mdx:17)
    in main (at Layout.js:11)
    in div (at Layout.js:6)
    in Layout (at BlogLayout.js:4)
    in BlogLayout (at this-is-the-slug.mdx:14)
    in div (created by MDXTag)
    in MDXTag (created by Context.Consumer)
    in Unknown (at this-is-the-slug.mdx:12)
    in Unknown (at _app.js:15)
    in Container (at _app.js:14)
    in MyApp

I couldn't find any other usages of this prop besides a test that checked for it. Does any rehast plugin depend on this prop?

Fixes vercel/next-plugins#307

[vercel]

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

[silvenon]

Thanks! I think it would be better if this had a slightly different behavior, described in #284. Then instead of changes to the test, you add a new test which proves that setting a language won't also pass an empty metaString prop.

[silvenon]

Could you instead tell it to pass metaString as a prop only if it's not an empty string? By exposing the raw string we will give more power to the user in the long run. If people use the infostring, extracted props will also be unrecognized so the user would have to define the code key of components anyway and deal with those anyway.

Suggested change

	const metaString = node.lang && node.lang.replace(langRegex, '').trim()
	const metaString = node.lang && node.lang.replace(langRegex, '').trim()
	props.metaString = metaString !== '' ? metaString : null

(I'm not sure if this is how suggestions work with multiple lines.)

[hugmanrique]

React will still complain about the metaString prop. If we want it to be rendered to the DOM and be modifiable by rehast plugin we should change this to props.metastring (notice the lowercase s).

This of course is a breaking change, but fixes the usage of an illegal JSX prop.

[silvenon]

The goal is to get rid of the metaString warning in this the most case, which is just setting the language:

```js
const a = 5;
```

because in the cases where the string actually contains something:

```js exec=true
console.log(5)
```

the prop exec will also get passed to tag <code>. React won't complain, but I can't imagine many cases where you would want this behavior, you probably want to intercept exec as well as metaString and not pass it forward:

const components = {
  code: ({ metaString, exec, ...props }) => {
    // do something based on exec or metaString
    return <code {...props} /> // no React warning
  },
}

Infostring is a rarely used feature, most people getting this warning are not using it at all. When they are, I don't think it's much hassle to prevent passing it to the DOM node, I think the warning even helps with that by telling you that you probably want to define a code component, otherwise why are you passing an infostring if you're not using it.

The warning doesn't help when metaString is empty, which is most often the case, so to me it makes sense to omit it only in that case. That way the change is not breaking and gives people more power, win-win.

What do you think?

[silvenon]

Actually, I think I see what you mean. Give me a day to think about it.

[silvenon]

I think using props.metastring instead of props.metaString would be the best thing to do here. That way React won't complain and we still expose the meta string to rehype plugins. Thanks for that suggestion, good thinking!

[hugmanrique]

Alright, it should be all good now 😀 I kept the previous behavior (passing a null value is the same as an empty string if the prop is passed to the DOM by React), but this change should be documented somewhere, what are your thoughts?

[silvenon]

AFAIK meta string isn't documented at all yet. I'll probably publish a minor release describing this change and add the meta string documentation later at some point.

I'll let this PR sit for a day, then I'll merge it.

[silvenon]

Released in v0.16.0.

[mununki]

Gentlemen, I'm stumbling with metaString warning in my project. I'm building with next.js, mdx and remark-highlight.js.

[package.json]

"dependencies": {
    "@mdx-js/mdx": "^0.16.5",
    "@zeit/next-mdx": "^1.2.0",
    "next": "6.1",
    "react": "^16.6.0",
    "react-dom": "^16.6.0",
    "react-markdown": "^4.0.3",
    "remark-emoji": "^2.0.2",
    "remark-highlight.js": "^5.0.0",
    "remark-images": "^0.16.1"
  },

[next.config.js]

const images = require("remark-images");
const emoji = require("remark-emoji");
const highlight = require("remark-highlight.js");

const withMDX = require("@zeit/next-mdx")({
  extension: /\.(md|mdx)?$/,
  options: {
    mdPlugins: [images, emoji, highlight]
  }
});
module.exports = withMDX();

I successfully import *.md file as React component and render it. But, I found <code class="hljs language-js" metaString=""> in source page after rendered. I think it renders metaString="" still.

I couldn't find any clue to resolve this warning. Can you help me to find a clue?

[hugmanrique]

I can't think of other reason than remark-highlight.js adding the metaString prop. Other cause might be npm could be package locking the mdx dep, mind posting your package-lock.json on a Gist?

[mununki]

@hugmanrique Thank you for your comment. Actually, I already tried to remove remark-highlight.js out of next.config.js, but same result. It causes only removing .hljs class selector from rendering, otherwise metaString="" is still there.

And...actually, I don't have package-lock.json in my project yet. Do I have to have it to resolve this?

[johno]

If this issue is still occurring for folks could we get an issue opened and a minimal reproduction in a repo for debugging? Would be super helpful on getting this fixed!

[mununki]

@johno @hugmanrique @silvenon If this issue happens not only to me, but some folks. I'll love to make small repo to produce this issue for you guys to fix it. let me know.

[mununki]

@hugmanrique @silvenon @johno Please find my small sample repo which reproduce the warning so called metaString props.

https://github.com/mattdamon108/mdx_metaString

[hugmanrique]

Replicated the issue on https://codesandbox.io/s/github/mattdamon108/mdx_metaString with the following package versions:

The only related changed to the original code this PR modified is c26a03d#diff-fd729e637d80469ad8fcafd812c97b44 .

This commit changes the value of metastring to node.meta which, by looking at the mdast specification, it will be null (instead of undefined) if no meta string is present after the language. Attached screenshot of specific case:

React probably doesn't like converting a null prop to a DOM attribute, so it throws that error.