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

Serializing interleaved Markdown in JSX can lead to a few quirks #1193

Closed
johno opened this issue Jul 28, 2020 · 4 comments
Closed

Serializing interleaved Markdown in JSX can lead to a few quirks #1193

johno opened this issue Jul 28, 2020 · 4 comments
Labels
🗄 area/interface This affects the public interface 💪 phase/solved Post is done 🐛 type/bug This is a problem 💎 v2 Issues related to v2

Comments

@johno
Copy link
Member

johno commented Jul 28, 2020

In Gatsby Recipes we wrap blocks of Markdown with JSX tags in a few spots. After manipulating the AST we then serialize back to MDX. This can cause a few quirks.

Consider the following input:

**Success**!

You're ready to get started!

-   Read the docs: [https://theme-ui.com](https://theme-ui.com)
-   Learn about the theme specification: [https://system-ui.com](https://system-ui.com)

_note:_ if you're running this recipe on the default starter (or any other starter with
base css), you'll need to remove the require to `layout.css` in the `components/layout.js` file
as otherwise they'll override some theme-ui styles.

We manipulate the AST and wrap it up like so:

    const wrappedResourceSteps = resourceSteps.map((step, i) => {
      return {
        type: `mdxBlockElement`,
        name: `RecipeStep`,
        attributes: [
          {
            type: `mdxAttribute`,
            name: `step`,
            value: String(i + 1),
          },
          {
            type: `mdxAttribute`,
            name: `totalSteps`,
            value: String(resourceSteps.length),
          },
        ],
        children: step,
      }
    })

We then result in:

<RecipeStep
  step="4"
  totalSteps="4"
>
  **Success**!

  You're ready to get started!

  -   Read the docs: [https://theme-ui.com](https://theme-ui.com)
  -   Learn about the theme specification: [https://system-ui.com](https://system-ui.com)

  _note:_ if you're running this recipe on the default starter (or any other starter with
  base css), you'll need to remove the require to `layout.css` in the `components/layout.js` file
  as otherwise they'll override some theme-ui styles.
</RecipeStep>

The indention of the "note" paragraph below causes it to be parsed as part of the li so we end up with:

    <ul>
      <li {...{
        "parentName": "ul"
      }}>
        <p {...{
          "parentName": "li"
        }}>{`Read the docs: `}
          <a {...{
            "href": "https://theme-ui.com",
            "parentName": "p"
          }}>{`https://theme-ui.com`}</a></p>
      </li>
      <li {...{
        "parentName": "ul"
      }}>
        <p {...{
          "parentName": "li"
        }}>{`Learn about the theme specification: `}
          <a {...{
            "href": "https://system-ui.com",
            "parentName": "p"
          }}>{`https://system-ui.com`}</a></p>
        <p {...{
          "parentName": "li"
        }}><em {...{
            "parentName": "p"
          }}>{`note:`}</em>
          {` if you're running this recipe on the default starter (or any other starter with
base css), you'll need to remove the require to `}
          <inlineCode {...{
            "parentName": "p"
          }}>{`layout.css`}</inlineCode>
          {` in the `}
          <inlineCode {...{
            "parentName": "p"
          }}>{`components/layout.js`}</inlineCode>
          {` file
as otherwise they'll override some theme-ui styles.`}</p>
      </li>
    </ul>

It seems like when we serialize to MDX we will need to dedent block-based Markdown inside JSX tags, otherwise significant whitespace will be injected that might not be intended. Any thoughts on how to best do this @wooorm?

@johno johno added 🐛 type/bug This is a problem 🙆 yes/confirmed This is confirmed and ready to be worked on 💎 v2 Issues related to v2 labels Jul 28, 2020
@wooorm
Copy link
Member

wooorm commented Jul 29, 2020

Pretty cool that the AST and serializing mostly works! 😅

I decided to allow folks to indent (and remove indented code), because I saw a couple of folks doing indents and expecting it to work. Hence, for the serializing, I also applied an indent.
For the second parse, are there any options on? It may be that pedantic: false (default, but gatsby-remark has it on for some reason) or commonmark: true (future) will help here?

@johno
Copy link
Member Author

johno commented Jul 29, 2020

Yeah! It's pretty robust so far! 🙏 💟

The second parse is using whatever the defaults are for MDX, so the parse => wrap with JSX => serialization => parse produces a different AST in terms of the Markdown content.

You can parse this with remark's default options and get the same thing:

  **Success**!

  You're ready to get started!

  -   Read the docs: [https://theme-ui.com](https://theme-ui.com)
  -   Learn about the theme specification: [https://system-ui.com](https://system-ui.com)

  _note:_ if you're running this recipe on the default starter (or any other starter with
  base css), you'll need to remove the require to `layout.css` in the `components/layout.js` file
  as otherwise they'll override some theme-ui styles.

Perhaps in MDX we should have commonmark on by default? Would that make sense?

@wooorm
Copy link
Member

wooorm commented Aug 23, 2020

It’s probably a bug in remark somewhere. commonmark: true, pedantic: false, might fix it. Or maybe not haha. micromark would definitely fix it, though!

wooorm added a commit that referenced this issue Dec 11, 2020
This updates MDX to use and support remark@13, which comes with a new internal
parser (micromark), and supports CommonMark.
See <https://github.com/remarkjs/remark/releases/tag/13.0.0> for more
information.
In short, this means MDX parses markdown closer to what folks expect.
And it means all latest plugins work (again).

But it also means that parsing MDX syntax (JSX, expressions, ESM) got an update.
See: <https://github.com/micromark/micromark-extension-mdxjs> and
<https://github.com/syntax-tree/mdast-util-mdx> for the syntax.
In short, this means MDX parsing is now JavaScript-aware: import/exports are
actually parsed for being valid JavaScript.
Expressions previously counted braces, but now can include braces in strings or
comments or whatnot.
This also means we can drop Babel (in a future PR) because we already have a
JavaScript AST.

This also deprecates the packages `remark-mdxjs` (which is now the default in
`remark-mdx`), `remark-mdx-remove-exports`, and `remark-mdx-remove-imports`.

Related to GH-704.
Related to GH-1041.

Closes GH-720.
Closes GH-1028.
Closes GH-1050.
Closes GH-1081.
Closes GH-1193.
Closes GH-1238.
Closes GH-1283.
Closes GH-1316.
Closes GH-1318.
Closes GH-1341.
wooorm added a commit that referenced this issue Dec 15, 2020
This updates MDX to use and support remark@13, which comes with a new internal
parser (micromark), and supports CommonMark.
See <https://github.com/remarkjs/remark/releases/tag/13.0.0> for more
information.
In short, this means MDX parses markdown closer to what folks expect.
And it means all latest plugins work (again).

But it also means that parsing MDX syntax (JSX, expressions, ESM) got an update.
See: <https://github.com/micromark/micromark-extension-mdxjs> and
<https://github.com/syntax-tree/mdast-util-mdx> for the syntax.
In short, this means MDX parsing is now JavaScript-aware: import/exports are
actually parsed for being valid JavaScript.
Expressions previously counted braces, but now can include braces in strings or
comments or whatnot.
This also means we can drop Babel (in a future PR) because we already have a
JavaScript AST.

This also deprecates the packages `remark-mdxjs` (which is now the default in
`remark-mdx`), `remark-mdx-remove-exports`, and `remark-mdx-remove-imports`.

Related to GH-704.
Related to GH-1041.

Closes GH-720.
Closes GH-1028.
Closes GH-1050.
Closes GH-1081.
Closes GH-1193.
Closes GH-1238.
Closes GH-1283.
Closes GH-1316.
Closes GH-1318.
Closes GH-1341.
Closes GH-1367.

Reviewed-by: Christian Murphy <[email protected]>
@wooorm
Copy link
Member

wooorm commented Dec 18, 2020

Hi all! I’m going to close this as it landed on the main, the (now default) branch we’re using to gear up to the MDX@2 release.

The reason is so that it’s more clear which issues still need to be solved already and don’t require further work.

Expect another next release soonish, which might include it, or maybe it’s already released as a beta!

For more info, see #1041.

@wooorm wooorm closed this as completed Dec 18, 2020
@wooorm wooorm added ⛵️ status/released 🗄 area/interface This affects the public interface and removed 🙆 yes/confirmed This is confirmed and ready to be worked on labels Dec 18, 2020
@wooorm wooorm added the 💪 phase/solved Post is done label Dec 24, 2021
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 💪 phase/solved Post is done 🐛 type/bug This is a problem 💎 v2 Issues related to v2
Development

No branches or pull requests

2 participants