Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't replace literal HTML elements with MDXProvider #821

Closed
adamwathan opened this issue Oct 18, 2019 · 12 comments
Closed

Don't replace literal HTML elements with MDXProvider #821

adamwathan opened this issue Oct 18, 2019 · 12 comments
Labels
🦋 type/enhancement This is great to have 💎 v2 Issues related to v2 🙆 yes/confirmed This is confirmed and ready to be worked on
Milestone

Comments

@adamwathan
Copy link

Right now when you provide a component mapping to an MDXProvider, every matching markdown element is replaced, even if those elements are authored in HTML instead of markdown.

For example, given this configuration:

// App.js
import { MDXProvider } from '@mdx-js/react'

const components = {
  h2: () => (<span>Boo!</span>),
}

export default props =>
  <MDXProvider components={components}>
    <div {...props} />
  </MDXProvider>

...the following MDX:

// Page.mdx
# My Page

## Should become "Boo!"

<h2>But please leave this one alone</h2>

...is rendered as:

<h1>My Page</h1>

<span>Boo!</span>

<span>Boo!</span>

This can be very surprising and challenging to work with when you are embedding components in your MDX that receive children:

// Page.mdx
import MyComponent from '../components/MyComponent'

# My Page

## Should become "Boo!"

<MyComponent>
  <h2>Please leave this alone!</h2>
</MyComponent>

I would love a way to be able to tell MDX "please only transform markdown tags, not literal HTML tags" so that literal HTML can be used as an escape hatch.

Hope this makes sense, and thanks for the awesome work on MDX!

@wooorm
Copy link
Member

wooorm commented Apr 21, 2020

Hmm, my initial feeling is that it makes more sense that they’re treated the same, whether they’re written as JSX or as Markdown.

If you want them rendered differently, you should be able to use a <MyH2>Please leave this alone!</MyH2> in <MyComponent>, no?

This example clearly illustrates the problem, but it’s a bit weird to map h2 elements to spans, could you expand on what the you were actually doing?

@adamwathan
Copy link
Author

Hey thanks for following up on this with me! The specific use case was writing documentation with inline code samples (specifically I was working on a redesign of the Tailwind CSS docs), and I wanted to bake styles into my headings, paragraphs, etc., but didn't want those styles leaking into the examples:

// App.js
import { MDXProvider } from '@mdx-js/react'

const components = {
  h2: ({ children }) => (<h2 class="text-2xl font-bold mt-6 mb-4">{children}</h2),
  // ...
}

export default props =>
  <MDXProvider components={components}>
    <div {...props} />
  </MDXProvider>
// Page.mdx
# My Page

## Some documentation title

A paragraph explaining something.

<div>
  <h2>An embedded example that I don't want to have my markdown h2 styles.</h2>
</div>

Think stuff like this screenshot, where there I don't want my markdown styles interfering with my demo styles:

image

Historically I have solved this with a bunch of convoluted CSS that uses direct descendant selectors and stuff to make sure the markdown styles never leak down into the examples, but was excited about being able to ditch that with MDX.

@wooorm
Copy link
Member

wooorm commented Apr 23, 2020

Thanks! I think that makes sense, some form of sandboxing.

I can see how the current behavior also makes sense though, say you had a component where for whatever reason you’re not using a <h2> in a component instead of a Markdown heading, and do want them to be the same. 🤔

@remorses
Copy link

I agree with @adamwathan, if you want to use the markdown heading style you can simply import the react component used in the mdx provider.

The html and jsx elements should be rendered without any change in styles

@adamwathan
Copy link
Author

if you want to use the markdown heading style you can simply import the react component used in the mdx provider

Yeah I was going to say the same thing, there's a way to get the desired behavior if the default is "don't replace normal elements" but the reverse is not true.

My mental model when first using MDX was that it was interpreting markdown and translating it to components, and I had the ability to control the components. I don't think of a raw <h2> element in a markdown file as actual markdown so I think that's why I was surprised that it was replacing those elements.

Thanks again for being open to the discussion, MDX is such cool tech 🙌

@johno
Copy link
Member

johno commented Apr 30, 2020

We've definitely got this requested a few times so it's something we plan on supporting as an option in v2.

My mental model when first using MDX was that it was interpreting markdown and translating it to components, and I had the ability to control the components.

Yeah, I can empathize with this mental model. Interestingly we've found a lot of users with opposite mental model as well, and would end up confused when a h2 or p tag in an MDX document had different styling.

So, perhaps we could support opting out of HTML element rendering with an API similar to?

<MDXProvider components={myComponents} ignoreInlineTags={true}>
  {/* stuff */}
</MDXProvider>

I imagine there's probably a better prop name than ignoreInlineTags but I haven't given it a lot of thought yet. Definitely open to thoughts and suggestions!

@johno johno added 💎 v2 Issues related to v2 🙆 yes/confirmed This is confirmed and ready to be worked on 🦋 type/enhancement This is great to have and removed 🙉 open/needs-info This needs some more info 🦋 type/enhancement This is great to have labels Apr 30, 2020
@remorses
Copy link

remorses commented Apr 30, 2020

I think skipJsxTagsStyling is a better name, but everyone knows naming is the hard part

@johno
Copy link
Member

johno commented May 1, 2020

I think skipJsxTagsStyling is a better name

Yeahhhh, something to think about here. Styling is a bit too specific because components in MDXProvider can add more than styles (functionality and dynamic behavior). But we have time to think about naming while we work on implementation and the rest of v2 this next month.

but everyone knows naming is the hard part

✅🤣

@wooorm
Copy link
Member

wooorm commented May 1, 2020

Something like sandbox maybe?

@adamwathan
Copy link
Author

Honestly even a Sandbox component could be an interesting angle (I like the config prop too though):

// Page.mdx
# My Page

## Some documentation title

A paragraph explaining something.

<MDXSandbox>
  <div>
    <h2>An embedded example that I don't want to have my markdown h2 styles.</h2>
  </div>
</MDXSandbox>

@johno
Copy link
Member

johno commented May 1, 2020

I really like the idea of an MDXSandbox component since it is more composable and explicit.

@johno
Copy link
Member

johno commented Jul 30, 2020

Going to close this now since it's implemented in the canary release of v2. You can take it for a spin with yarn add @mdx-js/mdx@next @mdx-js/react@next.

And then use it like so:

<MDXProvider disableParentContext={true}>
  {/* All MDX contained within will render as literal HTML elements */}
</MDXProvider>

Additionally, you can create a component if you wanted to "sandbox" elements:

export const MDXSandbox = props => <MDXProvider {...props} disableParentContext={true} />

# I'm using the `h1` from MDXProvider!

<MDXSandbox>
  <h1>I'm a literal h1!</h1>
</MDXSanbox>

Thanks for the input all!

@johno johno closed this as completed Jul 30, 2020
wooorm added a commit that referenced this issue Jan 2, 2021
This PR moves most of the runtime to the compile time.

This issue has nothing to do with `@mdx-js/runtime`. It’s about
`@mdx-js/mdx` being compile time, and moving most work there, from the
“runtimes” `@mdx-js/react`, `@mdx-js/preact`, `@mdx-js/vue`.

Most of the runtime is undocumented features that allow amazing things,
but those are in my opinion *too magical*, more powerful than needed,
complex to reason about, and again: undocumented.
These features are added by overwriting an actual renderer (such as
react, preact, or vue). Doing so makes it hard to combine MDX with for
example Emotion or theme-ui, to opt into a new JSX transform when React
introduces one, to support other hyperscripts, or to add features such
as members (`<Foo.Bar />`). Removing these runtime features does what
MDX says in the readme: “**🔥 Blazingly blazing fast: MDX has no
runtime […]**”

This does remove the ability to overwrite *anything* at runtime. This
brings back the project to what is documented: users can still
overwrite markdown things (e.g., blockquotes) to become components and
pass components in at runtime without importing them. And it does still
allow undocumented parent-child combos (`blockquote.p`).

* Remove runtime renderers (`createElement`s hijacking) from
  `@mdx-js/react`, `@mdx-js/preact`, `@mdx-js/vue`
* Add `jsxRuntime` option to switch to the modern automatic JSX runtime
* Add `jsxImportSource` option to switch to a modern non-React JSX
  runtime
* Add `pragma` option to define a classic JSX pragma
* Add `pragmaFrag` option to define a classic JSX fragment
* Add `mdxProviderImportSource` option to load an optional runtime
  provider
* Add tests for automatic React JSX runtime
* Add tests for `@mdx-js/mdx` combined with `emotion`
* Add support and test members as “tag names” of elements
* Add support and test qualified names (namespaces) as “tag names” of
  elements
* Add tests for parent-child combos
* Add tests to assert explicit (inline) components precede over
  provided/given components
* Add tests for `mdxFragment: false` (runtime renderers w/o fragment
  support)
* Fix and test double quotes in attribute values

This PR removes the runtime renderers and related things such as the
`mdxType` and `parentName` props while keeping the `MDXProvider` in
tact.

This improves runtime performance, because all that runs at runtime is
plain vanilla React/preact/vue code.

This reduces the surface of the MDX API while being identical to what
is documented and hence to user expectations (except perhaps to some
power users).

This also makes it easier to support other renderers without having to
maintain projects like `@mdx-js/react`, `@mdx-js/preact`, `@mdx-js/vue`:
anything that can be used as a JSX pragma (including the [automatic
runtime](https://reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html))
is now supported.
A related benefit is that it’s easier to integrate with
[emotion](https://github.com/emotion-js/emotion/blob/master/packages/react/src/jsx.js#L7)
(including through `theme-ui`) and similar projects which also
overwrite the renderer: as it’s not possible to have two runtimes, they
were hard to combine; because with this PR MDX is no longer a renderer,
there’s no conflict anymore.

This is done by the compile time (`@mdx-js/mdx`) knowing about an
(**optional**) runtime for an `MDXProvider` (such as `@mdx-js/react`,
`@mdx-js/preact`). Importantly, it’s not required for other
hyperscript interfaces to have a provider: `MDXContent` exported from
a compiled MDX file *also* accepts components (it already did), and Vue
comes with component passing out of the box.

In short, the runtime looked like this:

```js
function mdx(thing, props, ...children) {
  const overwrites = getOverwritesSomeWay()
  return React.createElement(overwrites[props.mdxType] || thing, props, ...children)
}
```

And we had a compile time, which added that `mdxType` prop. So:

```mdx
<Youtube />
```

Became:

```js
const Youtube = () => throw new Error('Youtube is not loaded!')

<Youtube mdxType="Youtube" />
```

Which in plain JS looks like:

```js
const Youtube = () => throw new Error('Youtube is not loaded!')

React.createElement(Youtube, {mdxType: 'Youtube'})
```

Instead, this now compiles to:

```js
const {Youtube} = Object.assign({Youtube: () => throw new Error('Youtube is not loaded!')}, getOverwritesSomeWay())

React.createElement(Youtube)
```

The previous example shows what is sometimes called a “shortcode”: a
way to inject components as identifiers into the MDX file, which was
introduced in [MDX 1](https://mdxjs.com/blog/shortcodes)

A different use case for the runtime was overwriting “defaults”. This
is documented on the website as the “[Table of
components](https://mdxjs.com/table-of-components)”. This MDX:

```mdx
Hello, *world*!
```

Became:

```js
<p mdxType="p">Hello, <em mdxType="em">world</em>!</p>
```

This now compiles to:

```js
const overwrites = Object.assign({p: 'p', em: 'em'}, getOverwritesSomeWay())

<overwrites.p>Hello, <overwrites.em>world</overwrites.em>!</overwrites.p>
```

This MDX:

```mdx
export const Video = () => <Vimeo />

<Video />
```

Used like so:

```jsx
<MDXProvider components={{Video: () => <Youtube />}}>
  <Content />
</MDXProvider>
```

Would result in a `Youtube` component being rendered. It no longer
does. I see the previous behavior as a bug and hence this as a fix.

A subset of the above point is that:

```mdx
export default props => <main {...props} />

x
```

Used like so:

```jsx
<MDXProvider components={{wrapper: props => <article {...props} />}}>
  <Content />
</MDXProvider>
```

Would result in an `article` instead of the explicit `main`. It no
longer does. I see the previous behavior as a bug and hence this as a
fix.

(#821)

```mdx

<h2>World</h2>
```

Used like so:

```jsx
<MDXProvider components={{h2: () => <SomethingElse />}}>
  <Content />
</MDXProvider>
```

Would result in a `SomethingElse` for both. This PR **does not** change
that. But it could more easily be changed if we want to, because at
compile time we know whether something was a tag or not.

An undocumented feature of the current MDX runtime renderer is that
it’s possible to overwrite anything:

```mdx
<span />
```

Used like so:

```jsx
<MDXProvider components={{span: props => <b>{props.children}</b>}}>
  <Content />
</MDXProvider>
```

Would overwrite to become bold, even though it’s not documented
anywhere. This PR changes that: only allowed markdown “tag names” can
be changed (`p`, `li`, ...). **This list could be expanded.**

Another undocumented feature is that parent–child combos can be
overwritten. A `li` in an `ol` can be treated differently from one in
an `ul` by passing `'ol.li': () => <SomethingElse />`.

This PR no longer lets users “nest” arbitrary parent–child combos
except for `ol.li`, `ul.li`, and `blockquote.p`. **This list could
be expanded.**

It was not possible to use members (`<foo.bar />`, `<Foo.bar.baz />`,
<#953>) and supporting it previously
would be complex. This PR adds support for them.

Previously, `mdxType` and `parentName` attributes were added to all
elements. And a `components` prop was accepted on **all** elements to
change the provider. These are no longer passed and no longer accepted.
Lastly, `components`, `props` were in scope for all JSX tags defined in
the “markdown” section (not the import/exports) of each document.

This adds identifiers to the scope prefixed with double underscores:
`__provideComponents`, `__components`, and `__props`.

A single 1mb MDX file, about 20k lines and 135k words (basically 3
books). Heavy on the “markdown”, few tags, no import/exports.
322kb gzipped.

* v1: 2895.122856
* 2.0.0-next.8: 3187.4684129999996
* main: 4058.917152000001
* this pr: 4066.642403

* v1: raw: 1.5mb, gzip: 348kb
* 2.0.0-next.8: raw: 1.4mb, gzip: 347kb
* main: raw: 1.3mb, gzip: 342kb
* this pr: raw: 1.8mb, gzip: 353kb
* this pr, automatic runtime: raw: 1.7mb, gzip: 355kb

* v1: 321.761208
* 2.0.0-next.8: 321.79749599999997
* main: 162.412757
* this pr: 107.28038599999996
* this pr, automatic runtime: 123.73588899999999

This PR is much faster on giant markdown-esque documents during runtime.
The win over the current `main` branch is 34%, the win over the last
beta and v1 is 66%.

For output size, the raw value increases with this PR, which is because
the output is now `/*#__PURE__*/React.createElement(__components.span…)`
or `/*#__PURE__*/_jsx(__components.span…)`, instead of `mdx("span",
{mdxType: "span"…})`. The change is more repetition, as can be seen by
the roughly same gzip sizes.

That the build time of `main` and this PR is slower than v1 and the
last beta does surprise me a lot. I benchmarked earlier with 1000 small
simple MDX files, totalling 1mb, [where the results were the
inverse](#1399 (comment)). So
it looks like we have a problem with giant files. Still, this PR has no
effect on build time performance, because the results are the same as
currently on `main`.

This PR makes MDX faster, adds support for the modern automatic JSX
runtime, and makes it easier to combine with Emotion and similar
projects.

---

Some of what this PR does has been discussed over the years:

Related-to: GH-166.
Related-to: GH-197.
Related-to: GH-466 (very similar).
Related-to: GH-714.
Related-to: GH-938.
Related-to: GH-1327.

This PR solves some of the items outlined in these issues:

Related-to: GH-1152.
Related-to: #1014 (comment).

This PR solves:

Closes GH-591.
Closes GH-638.
Closes GH-785.
Closes GH-953.
Closes GH-1084.
Closes GH-1385.
@y-nk y-nk mentioned this issue Feb 19, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🦋 type/enhancement This is great to have 💎 v2 Issues related to v2 🙆 yes/confirmed This is confirmed and ready to be worked on
Development

No branches or pull requests

4 participants