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

Update to remark@13 (micromark) #1367

Merged
merged 2 commits into from
Dec 15, 2020
Merged

Update to remark@13 (micromark) #1367

merged 2 commits into from
Dec 15, 2020

Conversation

wooorm
Copy link
Member

@wooorm wooorm commented 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.

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 wooorm added the 🗄 area/interface This affects the public interface label Dec 11, 2020
@vercel
Copy link

vercel bot commented Dec 11, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/mdx/mdx/lk7gmby28
✅ Preview: Failed

[Deployment for e6062b1 failed]

@vercel vercel bot temporarily deployed to Preview December 11, 2020 11:15 Inactive
.remarkrc.js Show resolved Hide resolved
docs/advanced/typescript.mdx Show resolved Hide resolved
packages/mdx/mdx-ast-to-mdx-hast.js Show resolved Hide resolved
packages/mdx/mdx-hast-to-jsx.js Show resolved Hide resolved
packages/vue/test/test.js Show resolved Hide resolved
Comment on lines +86 to +87
const importsAndExports = []
.concat(importStatements, exportStatements)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this causing an issue?
Would spread syntax be appropriate here?

Suggested change
const importsAndExports = []
.concat(importStatements, exportStatements)
const importsAndExports = [...importStatements, ...exportStatements]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they used to be two strings, now they’re two arrays.
I’m fine with spread or whatnot but there doesn’t seem to be a well defined baseline on what is and isn’t allowed, so I just did what I was familiar with!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node 8-15 support spread syntax, as well as every major browser (except IE) https://kangax.github.io/compat-table/es6/

packages/remark-mdx/package.json Show resolved Hide resolved
@ChristianMurphy
Copy link
Member

ChristianMurphy commented Dec 11, 2020

question on intended behavior.
In mdx 1, the following mdx file:

{

would return { as a character with no errors.
In this PR the same file results in

{
  reason: 'Unexpected end of file in expression, expected a corresponding closing brace for `{`',
  line: 1,
  column: 2,
  location: {
    start: { line: 1, column: 2, offset: 1, _index: 1, _bufferIndex: -1 },
    end: { line: null, column: null }
  },
  source: 'micromark-extension-mdx-expression',
  ruleId: 'unexpected-eof'
}

Is this the expected behavior?
My impression was a JSX expression had to be inside a JSX tag/element block, is this no longer the case?
If it is the case, would it make sense to gracefully fallback to displaying the raw text similar to an unclosed link [](?


Similar question goes for a floating greater than in a file:

<

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Dec 11, 2020

Another open question, how should mdx expressions interact with other MDX extensions like frontmatter?
for example:

---
property: <>{"hello world"}</>
---

Should the value of the yaml attribute property be hello world or <>{"hello world"}</>?

@ChristianMurphy
Copy link
Member

Another curious edge case for you.
this is considered valid JSX/MDX:

<Icon name="hero
" />

this is not:

- <Icon name="hero
" />

failing with

{
  reason: 'Unexpected end of file in attribute value, expected a corresponding closing quote `"`',
  line: 2,
  column: 1,
  location: {
    start: { line: 2, column: 1, offset: 18, _index: 2, _bufferIndex: -1 },
    end: { line: null, column: null }
  },
  source: 'micromark-extension-mdx-jsx',
  ruleId: 'unexpected-eof'
}

@wooorm
Copy link
Member Author

wooorm commented Dec 12, 2020

{ and < on their own were already implemented and documented in the previous PR: #1039 (see “Most likely breaking changes from MDXjs 1”).

My impression was a JSX expression had to be inside a JSX tag/element block, is this no longer the case?

This was also implemented there and further refined in the micromark rewrite. It was a common request for folks to embed expression in text (such as ## My {prop} heading)

If it is the case, would it make sense to gracefully fallback to displaying the raw text similar to an unclosed link [](?

I think the clearest rule is: with < and { you’re in JSX. You get errors. Otherwise you’re in markdown, you don’t get errors.
If you’re going to be silent about some cases, then: where’s the line? < + EOF? < + whitespace + EOF? < + x + EOF?

Markdown and JSX are very different languages. With JSX, folks are programming and expect errors, for </x/> to throw an error instead of display as text.

@wooorm
Copy link
Member Author

wooorm commented Dec 12, 2020

Another open question, how should mdx expressions interact with other MDX extensions like frontmatter?

Frontmatter is just a buffer for markdown, not a data structure. Similar how footnotes aren’t added inside frontmatter, JSX / expressions / ESM shouldn’t be added there IMO either.

Folks coming from markdown seem to want to use frontmatter, but MDX also has its ESM for data. I believe some of the other MDX maintainers prefer using ESM instead of frontmatter with MDX.

@wooorm
Copy link
Member Author

wooorm commented Dec 12, 2020

For:

- <Icon name="hero
" />

This matches how other multiline blocks in markdown work, take this markdown:

- ```js
asdasd

Yields:

asdasd

@ChristianMurphy
Copy link
Member

This matches how other multiline blocks in markdown work, take this markdown

The reason I thought it interesting, it (seems) to conflict a bit with:

I think the clearest rule is: with < and { you’re in JSX. [...] Otherwise you’re in markdown,

here we're in < (jsx) but we're applying a markdown block rule to it.

@wooorm
Copy link
Member Author

wooorm commented Dec 12, 2020

The reason I thought it interesting, it (seems) to conflict a bit with: ...
here we're in < (jsx) but we're applying a markdown block rule to it.

I’m sure that’ll trip folks up yeah (although: it’s a bit of a weird case so it probably won’t occur much). Reason is that the list has to be dealt with before (or in fact, simultaneously as) the JSX tag. The list “knows” it’s not in a paragraph so therefore, when it sees lazy text, exits the list.
Alternative would be to “hack” something on top of markdown to allow lazy lines in both paragraphs and tags, but I don’t think that’s worth it.

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
Development

Successfully merging this pull request may close these issues.

2 participants