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

An alternative “shortcode” (mdxType alternative) proposal #1385

Closed
wooorm opened this issue Dec 15, 2020 · 12 comments
Closed

An alternative “shortcode” (mdxType alternative) proposal #1385

wooorm opened this issue Dec 15, 2020 · 12 comments
Labels
🗄 area/interface This affects the public interface 🙉 open/needs-info This needs some more info 🦋 type/enhancement This is great to have

Comments

@wooorm
Copy link
Member

wooorm commented Dec 15, 2020

Currently, MDX is walking the whole JS program, and adding an mdxType prop to all JSX elements. It does this, so components can be overwritten at runtime.

Take this MDX:

* a
* b

Which turns into (simplified to get the point across, the actual is more complex):

var ol = () => throw "error"
var li = () => throw "error"

<ol mdxType="ol">
  <li mdxType="li" parentName="ol">a</li>
  <li mdxType="li" parentName="ol">b</li>
</ol>

After compiling the JSX away, you get:

var ol = () => throw "error"
var li = () => throw "error"

func(ol {mdxType: "ol"}, [
  func(li, {mdxType: "li", parentName: "ol"}, ["a"]),
  func(li, {mdxType: "li", parentName: "ol"}, ["b"]),
])

Then, the runtime, supports replacing things through components.

It has a func that ignores its first argument, instead taking mdxType (and parentName) to look whether it has a different component defined in components, and uses that instead of the either imported or defined throwing components.

The above is the simplified example for “normal” tags. The same happens for actual components (Capitalized):

var MyComponent = () => throw "error"

func(MyComponent, {mdxType: "MyComponent"}, [])

Which is then overwritten at runtime...

Even weirder is:

import MyComponent from "place"

func(MyComponent, {mdxType: "MyComponent"}, [])

// Passed in runtime:
{components: {"MyComponent": () => /* this is used instead of the imported component! */}}

This ✨ magic ✨ is complex and undocumented. I want to remove that complexity. Here’s one alternative.

We instead compile all standard markdown (html?) things to a namespace object: p becomes mdx.p, strong becomes mdx.strong, etc:

<mdx.ol>
  <mdx.li>a</mdx.li>
  <mdx.li>a</mdx.li>
</mdx.ol>

Then, in mdxContent (the wrapper function around all JSX components), we’re being passed runtime components. We take those and use them to fill mdx.p = components.p || 'p'.

This might solve #821, because the JSX <p>x</p> is not overwritten whereas the markdown paragraph x is, depending on where we implement the changing of p to mdx.p.

This then has no knowledge of JSX: there is no tree traversed for whether <MyComponent /> is imported or not. You have to import it, you can’t just pass it Youtube to components, you have to import it.

/cc @johno, @ChristianMurphy

@wooorm wooorm added 🦋 type/enhancement This is great to have 🗄 area/interface This affects the public interface 🔍 status/open labels Dec 15, 2020
@ChristianMurphy
Copy link
Member

Overall I prefer proposed approach, the idea of mdx.* components to allow for shortcodes.
It fits cleanly in a component based model of viewing the document, and the JSX translates directly to the hyperscript calls.
Intercepting the hyperscript calls is a powerful approach, but feels more unexpected, I usually wouldn't think to look for hyperscript calls being intercepted, and the wrapping could potentially be hidden by source mapping.

Some considerations:

  1. this would add another global identifier to the page mdx, some consideration of how that would interact with import mdx from 'some-package' would need to be done/documented.
  2. it would potentially remove the current ability to override components based off their parent with parent.child syntax. Personally I have seen that syntax as also being unexpected, something that may be a better fit for a plugin, and would be fine with removing it. But if removed, it would be a breaking change.

@johno
Copy link
Member

johno commented Dec 16, 2020

Personally I think that presentational context is a pretty powerful feature that allows MDX to be more flexible amongst rendering environments. MDX, as a format, isn't necessarily guaranteed to be coupled to code (consider a CMS). It also allows for more ergonomic composition when you want to override rendering at the sub-document level.

Forcing imports for additional components like Youtube sort of breaks the best part of shortcodes IMO because a parent or template can no longer expose commonly used globals. Part of the motivation behind the API is removing other forms of bespoke shortcode syntax and overrides/overloading of existing syntax.

This custom pragma functionality undoubtedly adds complexity and bit of magic/surprise (and some runtime weight), but I think it's worth it. Especially if we can eventually compile away the runtime when it isn't needed.

Would love for more folks to weigh in here, though!

@ChristopherBiscardi
Copy link
Member

You have to import it, you can’t just pass it Youtube to components, you have to import it.

That alone makes this a non-starter for me. full stop.

  • If we force imports then we tie MDX content written in a CMS to the bundling and filesystem details of all of the sites that use that content.
  • This is fine for someone who is writing the code for their own personal site, but not even close to ok for content authors who are used to shortcodes in other ecosystems (wordpress, hugo, etc) or large systems of MDX content.
  • Even for the person writing for their own site, the import paths now force them to use similar non-mdx technical implementations in each of site that accesses the content.

This change will result in the following user behavior:

User will tire of remembering how their gatsby/next/toast/etc site handles file paths and seek to use a webpack alias or similar to paper over this. Effectively changing import YouTube from '../../components/youtube.js'; to import YouTube from 'shortcodes/youtube' or import { YouTube } from 'shortcodes'.

on a technical level, the last example is problematic because we're forcing people into tree-shaking which is widely misunderstood and poorly implemented by users.

the ../../components/youtube.js example is technically problematic because it relies on implementation details of the library used to build mdx (such as gatsby-plugin-mdx, or next-mdx-remote) and how/where imports are sourced from (which changes per-library and across library versions).

would potentially remove the current ability to override components based off their parent with parent.child syntax.

How would you replace the paragraphs rendered inside a blockquote after this change? (without changing global paragraphs or unintentionally affecting other paragraph elements).


In general I disagree with the approach of introducing end-user complexity for the benefit of a simpler implementation. I'm open to other opinions but this seems like a change for the benefit of maintainers at the cost of users.

@wooorm
Copy link
Member Author

wooorm commented Dec 16, 2020

Thank you all for your feedback!

You have to import it, you can’t just pass it Youtube to components, you have to import it.

That alone makes this a non-starter for me. full stop.

Scratch the you. Replacing it with “someone [has to]” (at build time). And then I don’t think we’re far apart.

There are two completely different things going on here:

  1. Overwriting all “markdown” "h1"s to render component.h1 instead, which can be done by a programmer or some CMS system (which passes certain components without a user knowing)
  2. Editors being able to use JSX/markdown with some certain components magically being injected, like YouTube, which is done through components by a CMS, without the user knowing

For the first: that can be done nicely with (continued thought on) this proposal. In fact, this proposal would do away with the (p)react runtimes. So this use case would become smaller, less magical, and would include less bugs.

For the second, If some CMS wants to inject YouTube components, it can: we have a JavaScript AST (#1382). And wrapExport. And docs on how to do that in MDX@1.

And we have members: <Wordpress.Youtube />, <Wordpress.Vimeo /> can all be used with a single (user or injected) import.

the ../../components/youtube.js example is technically problematic because it relies on implementation details of the library used to build mdx (such as gatsby-plugin-mdx, or next-mdx-remote) and how/where imports are sourced from (which changes per-library and across library versions).

Expecting a Youtube component to be defined magically is just as problematic: it relies on the CMS/library/tool. Hence why I’d rather explicitly have folks import their dependencies: import Wordpress from "wordpress", where the CMS or whatnot could change that to whatever they want. Or have tools inject whatever they support.

How would you replace the paragraphs rendered inside a blockquote after this change? (without changing global paragraphs or unintentionally affecting other paragraph elements).

I propose compiling standard “markdown” things to something else:

Say you have this MDX file:

import MyComponet from "place"

<MyComponent id="123" />

Some *emphasis* and **strong**

This currently compiles to (roughly):

import MyComponet from "place"
function mdxContent(components) {
  return (
    <>
      <MyComponent id="123" />
      <p mdxType="p">Some <em mdxType="em" parentType="p">emphasis</em> and <strong mdxType="strong" parentType="p">strong</strong></p>
    </>
  )
}

And then there is a 5kb runtime specific to Vue, React, or Preact.

I think it’d be nice to instead compile to:

import MyComponet from "place"
function mdxContent(components) {
  var mdx = Object.assign({"p": "p", "em": "em", "strong": "strong"}, components)
  return (
    <>
      <MyComponent id="123" />
      <mdx.p>Some <mdx.em>emphasis</mdx.em> and <mdx.strong>strong</mdx.strong></mdx.p>
    </>
  )
}

This would remove all createElement stuff from the runtimes.

You could compile certain things to <mdx.blockquoteP>, being set as blockquoteP = components.blockquoteP || components.p || "p" s, and support a couple other parent child combos.


@ChristianMurphy
Copy link
Member

I think it’d be nice to instead compile to:

import MyComponet from "place"
function mdxContent(components) {
  var mdx = Object.assign({"p": "p", "em": "em", "strong": "strong"}, components)
  return (
    <>
      <MyComponent id="123" />
      <mdx.p>Some <mdx.em>emphasis</mdx.em> and <mdx.strong>strong</mdx.strong></mdx.p>
    </>
  )
}

with the notes that other props should still be able to be passed and context would/should also be a part of that assignment.
This pattern seems more inline with react conventions than wrapping createElement/hyperscript.

@wooorm
Copy link
Member Author

wooorm commented Dec 17, 2020

Yes, totally, abbreviated that just to be shorter, but it’s currently function MDXContent({components, ...props}) and I don’t intend changing that!

wooorm added a commit that referenced this issue Dec 18, 2020
This updates the dependencies and dev-dependencies in `packages/`.
Unfortunately, either updating to webpack 5 or updating to react 17 crash the
webpack loader with a react error, with an [invalid hook call
warning](https://reactjs.org/warnings/invalid-hook-call-warning.html) for
`useMDXComponents`:

<https://github.com/mdx-js/mdx/blob/dafdf6d70affa5dba0b3b7070f7a310b70bbf775/packages/react/src/context.js#L15>

Which might have to do with the magic of shortcodes (#1385), or something else,
I have no clue.

Furthermore, this loosens package dependencies instead of locking them,
which relates to GH-865, GH-1015, and GH-1267.
It was a long and divided discussion before and the reason for changing now is:
While the package currently doesn’t break easily (it was mentioned that unlocking
packages might cause that), we are currently *locked* on security vulnerabilities.
We’re not getting any patches, and MDX isn’t released that frequently or
maintained that actively, so MDX users are stuck.
If folks want to lock: npm and yarn have package locks.

Closes GH-1267.
Closes GH-1375.
wooorm added a commit that referenced this issue Dec 18, 2020
This updates the dependencies and dev-dependencies in `packages/`.
Unfortunately, either updating to webpack 5 or updating to react 17 crash the
webpack loader with a react error, with an [invalid hook call
warning](https://reactjs.org/warnings/invalid-hook-call-warning.html) for
`useMDXComponents`:

<https://github.com/mdx-js/mdx/blob/dafdf6d70affa5dba0b3b7070f7a310b70bbf775/packages/react/src/context.js#L15>

Which might have to do with the magic of shortcodes (#1385), or something else,
I have no clue.

Furthermore, this loosens package dependencies instead of locking them,
which relates to GH-865, GH-1015, and GH-1267.
It was a long and divided discussion before and the reason for changing now is:
While the package currently doesn’t break easily (it was mentioned that unlocking
packages might cause that), we are currently *locked* on security vulnerabilities.
We’re not getting any patches, and MDX isn’t released that frequently or
maintained that actively, so MDX users are stuck.
If folks want to lock: npm and yarn have package locks.

Closes GH-1267.
Closes GH-1375.
wooorm added a commit that referenced this issue Dec 20, 2020
This updates the dependencies and dev-dependencies in `packages/`.
Unfortunately, either updating to webpack 5 or updating to react 17 crash the
webpack loader with a react error, with an [invalid hook call
warning](https://reactjs.org/warnings/invalid-hook-call-warning.html) for
`useMDXComponents`:

<https://github.com/mdx-js/mdx/blob/dafdf6d70affa5dba0b3b7070f7a310b70bbf775/packages/react/src/context.js#L15>

Which might have to do with the magic of shortcodes (#1385), or something else,
I have no clue.

Furthermore, this loosens package dependencies instead of locking them,
which relates to GH-865, GH-1015, and GH-1267.
It was a long and divided discussion before and the reason for changing now is:
While the package currently doesn’t break easily (it was mentioned that unlocking
packages might cause that), we are currently *locked* on security vulnerabilities.
We’re not getting any patches, and MDX isn’t released that frequently or
maintained that actively, so MDX users are stuck.
If folks want to lock: npm and yarn have package locks.

Closes GH-1267.
Closes GH-1375.
Closes GH-1392.
@ChristopherBiscardi
Copy link
Member

There are two completely different things going on here:

  1. Overwriting all “markdown” "h1"s to render component.h1 instead, which can be done by a programmer or some CMS system (which passes certain components without a user knowing)
  2. Editors being able to use JSX/markdown with some certain components magically being injected, like YouTube, which is done through components by a CMS, without the user knowing

<h1/> is not a different user experience than <YouTube id=""/>. They were intentionally combined in implementation and user experience. This creates a single set of components (which is easier to understand and explain than namespacing or specially handling one group).

For the first: that can be done nicely with (continued thought on) this proposal. In fact, this proposal would do away with the (p)react runtimes. So this use case would become smaller, less magical, and would include less bugs.

You're using "magical" as a synonym for your personal architectural preference here and if it has fewer bugs it's because we're removing the code supporting functionality users have in production today.

We need to seriously consider how breaking this functionality would be (which doesn't look like it's been considered yet) and also consider how the hard-coding of elements at compile time would bring us closer to the pre-mdx implementations that existed and were generally insufficient for the purpose.

For the second, If some CMS wants to inject YouTube components, it can: we have a JavaScript AST (#1382). And wrapExport. And docs on how to do that in MDX@1.

This would lead to size bloat for MDX content that use common components. In the above case "a CMS system that passes components without a user knowing", that would require either deep CMS integration with the target system or including every usable component in the mdx ast.

Using the AST is purposefully reserved for tooling authors, not end-users. AST maniupulation is error-prone and a fundamentally worse UX compared to using components to achieve the same things. This kind of static analysis forces either us or the ecosystem to maintain code that we get for free from the respective frameworks we integrate with: which we can and should trust.

the ../../components/youtube.js example is technically problematic because it relies on implementation details of the library used to build mdx (such as gatsby-plugin-mdx, or next-mdx-remote) and how/where imports are sourced from (which changes per-library and across library versions).

Expecting a Youtube component to be defined magically is just as problematic: it relies on the CMS/library/tool. Hence why I’d rather explicitly have folks import their dependencies: import Wordpress from "wordpress", where the CMS or whatnot could change that to whatever they want. Or have tools inject whatever they support.

Again ignore your hand-waving "magic" comment, relying on tooling to supply a value is not the same as relying on the implementation details of those tools to provide a value. With the pragma/context the user has control over where and when a value is provided and MDX doesn't have an opinion on that. With the latter, the tooling developed by different teams of people that don't communicate (next, gatsby, etc) must work the same way across platforms which isn't going to happen and that differential burden falls on users.

import MyComponet from "place"
function mdxContent(components) {
  var mdx = Object.assign({"p": "p", "em": "em", "strong": "strong"}, components)
  return (
    <>
      <MyComponent id="123" />
      <mdx.p>Some <mdx.em>emphasis</mdx.em> and <mdx.strong>strong</mdx.strong></mdx.p>
    </>
  )
}

FYI this import gets stripped in the gatsby-plugin-mdx processing and included at the root of the application via context.

Passing differently rendered video players at different rendering positions in the application where MDX is rendered is also a valid usecase that wouldn't work with this proposal.

This approach locks a user into a single component everywhere that MDX document is used, reducing portability. What happens, for example, if a pre-compiled MDX component with this approach and AST modifications is transcluded with another MDX document? Different renderings should be effected via the declarative component-based approach, not an AOT AST.

This would remove all createElement stuff from the runtimes.

I don't see this as a useful goal unless it's enabling a better user experience and given the feature removal in this proposal, I don't think it is.

The createElement is currently opaque to the end-user (they don't have to worry about it at all) and most don't even know it exists. End-users leverage the flexibility the pragma gives them in their framework of choice and the code that implements it isn't complex or magic: it boils down to a single if statement. I'll also note that the code for the react pragma hasn't had to receive any major functional changes for over a year, it has been exceptionally stable.

You could compile certain things to <mdx.blockquoteP>, being set as blockquoteP = components.blockquoteP || components.p || "p" s, and support a couple other parent child combos.

Wouldn't this require an AST plugin?

I'll repeat that requiring end-users to interact with the remark/rehype/estree ASTs to achieve the functionality they need is both a step backwards and overly complicated when compared to the equivalent component based implementation. ASTs are for tooling authors, not end-users.

@wooorm
Copy link
Member Author

wooorm commented Jan 2, 2021

@ChristopherBiscardi Thanks for the detailed write up! ✨ I’ve been thinking and working on this the last week which I was about to push. I’ll push that now so you can take a look at it if you want, but know that it was written right before your post and that I’ll take your feedback int account later!

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.
@wooorm
Copy link
Member Author

wooorm commented Jan 2, 2021

I think all the above feedback is handled in #1425. No ASTs needed, MDXProvider is used, and there are benefits.

@ChristianMurphy
Copy link
Member

Personally I think that presentational context is a pretty powerful feature that allows MDX to be more flexible amongst rendering environments. MDX, as a format, isn't necessarily guaranteed to be coupled to code (consider a CMS). It also allows for more ergonomic composition when you want to override rendering at the sub-document level.

Agreed, my understanding is that setting components both via the props and react.Context presentation contexts would be preserved.

Expecting a Youtube component to be defined magically is just as problematic: it relies on the CMS/library/tool. Hence why I’d rather explicitly have folks import their dependencies: import Wordpress from "wordpress"

@wooorm my understanding is that this still allows YouTube to be passed via context and via props, but that the error message if it is missing would be different? Is that correct?

You're using "magical" as a synonym for your personal architectural preference here and if it has fewer bugs it's because we're removing the code supporting functionality users have in production today.

We need to seriously consider how breaking this functionality would be (which doesn't look like it's been considered yet) and also consider how the hard-coding of elements at compile time would bring us closer to the pre-mdx implementations that existed and were generally insufficient for the purpose.

@ChristopherBiscardi your insights are valued, could you give a more concrete example of what you see as breaking?
In it's current form your comment comes across more as FUD than as a technical objection.
My current understanding is that short codes, a feature you kindly brought to the ecosystem, would be preserved, and that this is a change to the internals to improve performance.

 import MyComponet from "place"
 function mdxContent(components) {
   var mdx = Object.assign({"p": "p", "em": "em", "strong": "strong"}, components)
   return (
     <>
       <MyComponent id="123" />
       <mdx.p>Some <mdx.em>emphasis</mdx.em> and <mdx.strong>strong</mdx.strong></mdx.p>
     </>
   )
 }

FYI this import gets stripped in the gatsby-plugin-mdx processing and included at the root of the application via context.

Passing differently rendered video players at different rendering positions in the application where MDX is rendered is also a valid usecase that wouldn't work with this proposal.

Am I interpreting correct, that if MyComponent was transformed to mdx.MyComponent, so that it could be instantiated by props and Context that this comment would be addressed?

@wooorm
Copy link
Member Author

wooorm commented Jan 2, 2021

@wooorm my understanding is that this still allows YouTube to be passed via context and via props, but that the error message if it is missing would be different? Is that correct?

All of that remains the way it is now in #1425, including the runtime warning

Am I interpreting correct, that if MyComponent was transformed to mdx.MyComponent, so that it could be instantiated by props and Context that this comment would be addressed?

Almost. Yes, MyComponent still works in #1425, but I instead add it to the scope:

const {YouTube} = Object.assign({}, componentsFromProvider, componentsGivenToMdxContent)

<Youtube />

The reason is that well, it’s a used identifier, so it’s more apt in my opinion to define it in the scope. Whereas for non-identifiers (such as p), it doesn’t make sense to define them in the scope, so there I make them members:

const __components = Object.assign({p: 'p'}, componentsFromProvider, componentsGivenToMdxContent)

<__components.p />

@wooorm
Copy link
Member Author

wooorm commented Oct 19, 2021

landed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 🙉 open/needs-info This needs some more info 🦋 type/enhancement This is great to have
Development

Successfully merging a pull request may close this issue.

4 participants