Injected custom elements and layouts result in crash #2123

calebeby · 2022-09-02T20:33:14Z

Previously, mdx generated const ;, which is not valid syntax. If you comment out the fix, the test fails.

In the test I didn't add an assertion for the HTML output since that is more tied to remark-shiki-twoslash, but if you would prefer me to add that I can.

@bholmesdev helped me narrow this down, thanks!

vercel · 2022-09-02T20:33:19Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
mdx	✅ Ready (Inspect)	Visit Preview	Oct 11, 2022 at 2:49PM (UTC)

codecov-commenter · 2022-09-02T20:39:53Z

Codecov Report

Merging #2123 (b1fc010) into main (996771a) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #2123   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         2044      2046    +2     
=========================================
+ Hits          2044      2046    +2

Impacted Files	Coverage Δ
packages/mdx/lib/plugin/recma-jsx-rewrite.js	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

wooorm

Hey, thanks!

So, I can’t reproduce your problem with your test. Can you think of a reproduction that does show your problem?

Additionally, this might just be due to remark-shiki-twoslash doing weird things:
a) remark is for markdown, not for HTML. rehype is for HTML.
b) Injecting strings of HTML into an AST is an bad idea, yes it can be fixed with rehype-raw but that essentially means parsing a file twice?
c) it has several long open issues

If possible, I’d suggest a different syntax highlighter or sending PRs to shiki twoslash to improve the situation

wooorm · 2022-09-03T13:48:20Z

packages/mdx/test/compile.js

@@ -1173,6 +1174,29 @@ test('remark-rehype options', async () => {
  )
 })

+test('remark-shiki-twoslash with layout function', async () => {
+  // Just test that the output can be generated correctly without JS syntax errors


This test passes locally for me, without your change to recma-jsx-rewrite?

Sorry about that. When I was cleaning up the PR I changed the import to get TS to work but that made the test no longer reproduce the problem. Should be fixed now (should pass when my code change is included, should fail with unexpected token ; when my code change is not included).

calebeby · 2022-09-05T16:42:42Z

Additionally, this might just be due to remark-shiki-twoslash doing weird things:
a) remark is for markdown, not for HTML. rehype is for HTML.
b) Injecting strings of HTML into an AST is an bad idea, yes it can be fixed with rehype-raw but that essentially means parsing a file twice?
c) it has several long open issues

If possible, I’d suggest a different syntax highlighter or sending PRs to shiki twoslash to improve the situation

I definitely agree with some of your concerns here, the double-parsing is definitely less-than-ideal. I'd like to improve the situation with twoslash, but that is a big project and for now I just wanted to get this bugfix through.

I would like to hear more about a and c if you are willing to clarify. Which long-open issues are more concerning to you? And I think it makes sense that remark-shiki-twoslash operates at the markdown phase, so it can process the pre-escaping code text, and also so that it can read from the metadata next to the language id after the opening backticks.

calebeby · 2022-09-05T16:43:53Z

Also, I am not familiar with the type-coverage check. Is it related to my casting to any to deal with TypeScript's CJS/ESM types interop? Is there a better way to do what I'm doing?

wooorm · 2022-09-06T18:32:30Z

(should pass when my code change is included, should fail with unexpected token ; when my code change is not included).

🤔🤔🤔 It still all just works for me with your test case without your code change, nothing broken 🤷‍♂️ Here’s my patch (excluding package lock changes):

+++ b/package.json
@@ -244,5 +244,8 @@
         false
       ]
     ]
+  },
+  "dependencies": {
+    "remark-shiki-twoslash": "^3.1.0"
   }
 }
diff --git a/packages/mdx/test/compile.js b/packages/mdx/test/compile.js
index d5a10ee0..aa8a5366 100644
--- a/packages/mdx/test/compile.js
+++ b/packages/mdx/test/compile.js
@@ -21,6 +21,7 @@ import remarkGfm from 'remark-gfm'
 import remarkMath from 'remark-math'
 import {VFile} from 'vfile'
 import {SourceMapGenerator} from 'source-map'
+import * as remarkShikiTwoslash from 'remark-shiki-twoslash'
 import {compile, compileSync, createProcessor, nodeTypes} from '../index.js'
 // @ts-expect-error: make sure a single react is used.
 import {renderToStaticMarkup as renderToStaticMarkup_} from '../../react/node_modules/react-dom/server.js'
@@ -1173,6 +1174,28 @@ test('remark-rehype options', async () => {
   )
 })
 
+test('remark-shiki-twoslash with layout function', async () => {
+  // Just test that the output can be generated correctly without JS syntax errors
+  renderToStaticMarkup(
+    React.createElement(
+      await run(
+        await compile(
+          `
+export default function ({ children }) { return <div>{children}</div>; }
+
+\`\`\`js twoslash
+console.log('hi')
+\`\`\`
+`,
+          {
+            remarkPlugins: [/** @type any */ (remarkShikiTwoslash).default],
+            rehypePlugins: [[rehypeRaw, {passThrough: nodeTypes}]]
+          }
+        )
+      )
+    )
+  )
+})
 test('MDX (JSX)', async () => {
   assert.equal(
     renderToStaticMarkup(

wooorm · 2022-09-06T18:55:01Z

double-parsing is definitely less-than-ideal. I'd like to improve the situation with twoslash, but that is a big project and for now I just wanted to get this bugfix through.

The main offenders are:

These are injecting raw strings of HTML into an AST.
Shiki can build an AST as far as I am aware.
The alternative in remark space for that is https://github.com/syntax-tree/mdast-util-to-hast#fields-on-nodes.
But the best place for dealing with HTML is in hast/rehype. That can also access code, language, meta, etc.

I would like to hear more about a and c if you are willing to clarify.

For a), see:

For c), (your more specific questions I asked in the previous quote answer), for specific issues, quickly looking through them these two come up:

Also, I am not familiar with the type-coverage check. Is it related to my casting to any to deal with TypeScript's CJS/ESM types interop? Is there a better way to do what I'm doing?

Correct, type-coverage prevents the use of any.
There are a couple ways around this, but I’d prefer to narrow down the problem and make a MVP without twoslash if possible. Let’s try and get that first and then hopefully this won’t be needed!

calebeby · 2022-09-06T22:38:26Z

It still all just works for me with your test case without your code change, nothing broken

I commented out the code fix and now it is failing as expected in CI. If you pull the latest commit it should fail locally in the same way hopefully?

https://github.com/mdx-js/mdx/runs/8217507997?check_suite_focus=true#step:5:40

calebeby · 2022-09-06T23:12:50Z

@wooorm can you help me understand why there is a distinction between a mdxJsxFlowElement and an element in the hast tree/rehype phase? Is it just because a mdxJsxFlowElement might be a component instead of an element? Or because its subtree might contain expressions/etc that is not valid html?

If I was making a rehype plugin that for example did something to h1 elements, and I wanted it to work as a rehypePlugin for mdx, or for rehype outside of mdx, should I look in the tree for both mdxJsxFlowElement and element with name/tagName of h1?

I'm asking this because I'm wondering whether rehype-raw is maybe not intended for use with MDX. Are nodes with type of element intended to be in a hast mdx tree?

calebeby · 2022-09-06T23:22:03Z

I think I have a reproduction that doesn't rely on shiki-twoslash!

If I put <custom-element/> in my MDX code, MDX deals with it just fine and the relevant node is like: { type: 'mdxJsxFlowElement', name: 'custom-element' }

If I inject <custom-element/> as { type: 'element', tagName: 'custom-element' } via a plugin (a raw node that rehype-raw parses) then the const ; bug happens.

So, depending on the answer to the question "are element nodes valid in MDX hast trees?":

If "no, mdxJsxFlowElement nodes should be used instead in MDX hast trees": Maybe there should be an option of rehype-raw to output mdxJsxFlowElement/etc instead of element nodes. Or a separate plugin that is essentially the same but with that change.

If "yes, element is valid in a MDX hast tree":

Then I can push a test like this that reproduces the bug without relying on remark-shiki-twoslash:

{
  remarkPlugins: [
    () => {
      const transform = async (markdownAST) => {
        markdownAST.children.push({
          type: 'html',
          value: `<custom-element />`,
          position: [],
          children: []
        })
      }
      return transform
    }
  ],
  rehypePlugins: [
    [rehypeRaw, {passThrough: nodeTypes}],
  ]
}

(or something similar that directly injects the element instead of injecting a raw node and pushing it through rehype-raw)

Does that sound right to you?

Thanks for your patience with me!

wooorm · 2022-09-08T18:38:10Z

Sweet!

The answer is: yes, `element` is valid in a MDX hast tree!

Hmm, I still have can’t reproduce an error with a test like that:

  renderToStaticMarkup(
    React.createElement(
      await run(
        await compile('', {
          remarkPlugins: [
            () => {
              const transform = async (markdownAST) => {
                markdownAST.children.push({
                  type: 'html',
                  value: '<custom-element />',
                  position: [],
                  children: []
                })
              }

              return transform
            }
          ],
          rehypePlugins: [[rehypeRaw, {passThrough: nodeTypes}]]
        })
      )
    )
  )

Note: the /> part on a custom element is not actually valid HTML, so if that’s what Shiki is injecting, that’s a bug there?

Do you have a more complete example that reproduces your problem?

@calebeby

used with remark-shiki-twoslash. @calebeby, who provided the workaround, is the king. - mdx-js/mdx#2123 - mdx-js/mdx#2112

wooorm · 2022-10-07T18:24:08Z

Friendly ping! :)

calebeby · 2022-10-09T01:48:37Z

Hey @wooorm, thanks for the ping. I'd really like to get back to this, it's on my list but school comes first right now :)

bholmesdev · 2022-10-10T18:45:36Z

@wooorm @calebeby No worries! I'll take it from here :) I was able to reproduce Caleb's bug on my end as well when using Shiki Twoslash. Working on a test case that's more minimal!

bholmesdev · 2022-10-11T14:37:56Z

~~@wooorm Hm, I'm getting a test-coverage threshold failure after rebasing that I didn't see before. Even reverting the code change, I still get a coverage error locally. Any idea what's causing this?~~
Saw your message! Confirmed it's the test itself. Not sure what I'm missing...

wooorm · 2022-10-11T14:45:43Z

packages/mdx/test/compile.js

@@ -1173,6 +1173,41 @@ test('remark-rehype options', async () => {
  )
 })

+/** @see https://github.com/mdx-js/mdx/issues/2112 */
+test.only('layout function with passthrough HTML', async () => {


@bholmesdev ^-- this! You have an only here.

Ah! Didn't realize. Will remove 👏

bholmesdev · 2022-10-11T14:53:05Z

Alright @wooorm, should be ready to review 👍

wooorm · 2022-10-11T17:07:08Z

Thanks everyone, particularly @calebeby and @bholmesdev, the reduced case helped a lot.
I reduced it some more, uncovered the underlying problem, which I also fixed. And kept this length check in because it doesn’t hurt to have it!

See the linked commit message for much more information on what was going on and how it was solved.

wooorm · 2022-10-11T17:20:20Z

Released in 2.1.5!

vercel bot deployed to Preview September 2, 2022 20:35 View deployment

wooorm reviewed Sep 3, 2022

View reviewed changes

vercel bot deployed to Preview September 5, 2022 16:37 View deployment

vercel bot deployed to Preview September 6, 2022 22:38 View deployment

wooorm mentioned this pull request Sep 22, 2022

Generates empty const statement #2112

Closed

4 tasks

hasparus added a commit to hasparus/zaduma that referenced this pull request Oct 5, 2022

Patch @mdx-js/mdx to stop generating invalid JSX when

1e57b94

used with remark-shiki-twoslash. @calebeby, who provided the workaround, is the king. - mdx-js/mdx#2123 - mdx-js/mdx#2112

vercel bot deployed to Preview October 10, 2022 19:54 View deployment

calebeby and others added 6 commits October 10, 2022 15:57

Fix edge case with remark-shiki-twoslash + custom layout

ccef969

Add comment

9a98439

Fix import

e205840

Try pushing to CI without fix

28dac78

fix: reproduce with minimal test case

99abb8c

deps: uninstall shiki-twoslash

5304861

bholmesdev force-pushed the edge-case-invalid-output branch from 30844ea to 5304861 Compare October 10, 2022 19:58

vercel bot deployed to Preview October 10, 2022 20:00 View deployment

wooorm reviewed Oct 11, 2022

View reviewed changes

fix: remove only from test

da6f072

vercel bot deployed to Preview October 11, 2022 14:49 View deployment

wooorm closed this in 90fa493 Oct 11, 2022

wooorm changed the title ~~Fix invalid syntax output~~ Injected custom elements and layouts result in crash Oct 11, 2022

wooorm added 🐛 type/bug This is a problem 🗄 area/interface This affects the public interface 💪 phase/solved Post is done labels Oct 11, 2022

djfarrelly mentioned this pull request Jan 5, 2023

Remove twoslash, fix other deps inngest/website#289

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Injected custom elements and layouts result in crash #2123

Injected custom elements and layouts result in crash #2123

calebeby commented Sep 2, 2022

vercel bot commented Sep 2, 2022 •

edited

Loading

codecov-commenter commented Sep 2, 2022

wooorm left a comment

wooorm Sep 3, 2022

calebeby Sep 5, 2022

calebeby commented Sep 5, 2022

calebeby commented Sep 5, 2022

wooorm commented Sep 6, 2022

wooorm commented Sep 6, 2022 •

edited

Loading

calebeby commented Sep 6, 2022 •

edited

Loading

calebeby commented Sep 6, 2022 •

edited

Loading

calebeby commented Sep 6, 2022 •

edited

Loading

wooorm commented Sep 8, 2022 •

edited

Loading

wooorm commented Oct 7, 2022

calebeby commented Oct 9, 2022

bholmesdev commented Oct 10, 2022

bholmesdev commented Oct 11, 2022 •

edited

Loading

wooorm Oct 11, 2022 •

edited

Loading

bholmesdev Oct 11, 2022

bholmesdev commented Oct 11, 2022

wooorm commented Oct 11, 2022

wooorm commented Oct 11, 2022

Injected custom elements and layouts result in crash #2123

Injected custom elements and layouts result in crash #2123

Conversation

calebeby commented Sep 2, 2022

vercel bot commented Sep 2, 2022 • edited Loading

codecov-commenter commented Sep 2, 2022

Codecov Report

wooorm left a comment

Choose a reason for hiding this comment

wooorm Sep 3, 2022

Choose a reason for hiding this comment

calebeby Sep 5, 2022

Choose a reason for hiding this comment

calebeby commented Sep 5, 2022

calebeby commented Sep 5, 2022

wooorm commented Sep 6, 2022

wooorm commented Sep 6, 2022 • edited Loading

calebeby commented Sep 6, 2022 • edited Loading

calebeby commented Sep 6, 2022 • edited Loading

calebeby commented Sep 6, 2022 • edited Loading

wooorm commented Sep 8, 2022 • edited Loading

wooorm commented Oct 7, 2022

calebeby commented Oct 9, 2022

bholmesdev commented Oct 10, 2022

bholmesdev commented Oct 11, 2022 • edited Loading

wooorm Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

bholmesdev Oct 11, 2022

Choose a reason for hiding this comment

bholmesdev commented Oct 11, 2022

wooorm commented Oct 11, 2022

wooorm commented Oct 11, 2022

vercel bot commented Sep 2, 2022 •

edited

Loading

wooorm commented Sep 6, 2022 •

edited

Loading

calebeby commented Sep 6, 2022 •

edited

Loading

calebeby commented Sep 6, 2022 •

edited

Loading

calebeby commented Sep 6, 2022 •

edited

Loading

wooorm commented Sep 8, 2022 •

edited

Loading

bholmesdev commented Oct 11, 2022 •

edited

Loading

wooorm Oct 11, 2022 •

edited

Loading