mdx icon indicating copy to clipboard operation
mdx copied to clipboard

Generates empty `const` statement

Open calebeby opened this issue 3 years ago • 2 comments

Initial checklist

Affected packages and versions

@mdx-js/[email protected]

Link to runnable example

No response

Steps to reproduce

My actual use case is that I have an astro site with an mdx file that uses shiki twoslash to highlight code snippets in the mdx. I haven't been able to narrow it down to a smaller reproducible code example, but I do have a fix!

https://github.com/mdx-js/mdx/blob/main/packages/mdx/lib/plugin/recma-jsx-rewrite.js#L390 This line is generating an empty const statement, which is invalid JS. It should not generate a const statement if there are no declarations.

This fix works:

if (declarations.length > 0) {
  statements.push({
    type: 'VariableDeclaration',
    kind: 'const',
    declarations
  })
}

I can send a PR with this change, but I'm not sure how to write a test for this, since I'm not sure how to narrow it down. Would you be able to help with a test, or would you accept a PR without a test?

Expected behavior

Don't emit an empty const declaration

function MDXContent(props = {}) {
  return <MDXLayout {...props}><_createMdxContent {...props} /></MDXLayout>;
}

Actual behavior

It is generating this :(

function MDXContent(props = {}) {
  const ;
  return <MDXLayout {...props}><_createMdxContent {...props} /></MDXLayout>;
}

Runtime

Node v16

Package manager

npm v8

OS

macOS

Build and bundle tools

Astro

calebeby avatar Aug 24 '22 22:08 calebeby

It would be very nice if there’s a reproduction/test case of this, to prevent it from reoccurring in the future

wooorm avatar Aug 25 '22 08:08 wooorm

You might be able to ping the people that work on these tools (astro, orta) to help reduce the test case?

wooorm avatar Aug 25 '22 17:08 wooorm

Coming from this issue, I can provide a minimal reproduction of this bug (Stackblitz).

I independently came to the same conclusion as @calebeby and fixed my problem in the same way. If anyone else is experiencing this issue, you can temporarily resolve it by applying this patch:

diff --git a/lib/plugin/recma-jsx-rewrite.js b/lib/plugin/recma-jsx-rewrite.js
index 1e82a1e41b18b16cb5d2f2a66b07e2d9544c55db..eb5286a273f095bedb693ec587917d6162164108 100644
--- a/lib/plugin/recma-jsx-rewrite.js
+++ b/lib/plugin/recma-jsx-rewrite.js
@@ -387,11 +387,13 @@ export function recmaJsxRewrite(options = {}) {
               })
             }
 
-            statements.push({
-              type: 'VariableDeclaration',
-              kind: 'const',
-              declarations
-            })
+            if (declarations.length > 0) {
+              statements.push({
+                type: 'VariableDeclaration',
+                kind: 'const',
+                declarations
+              })
+            }
           }
 
           /** @type {string} */

fbubbar avatar Sep 22 '22 00:09 fbubbar

FWIW I have a PR open that is close... I just started school so I'm super busy right now but I'm planning to get back to it soon

calebeby avatar Sep 22 '22 01:09 calebeby

minimal reproduction of this bug

I’m still looking for something smaller: something without Shiki. The smallest possible reproduction. See the related PR for more info :)

wooorm avatar Sep 22 '22 08:09 wooorm

Hello, this might be a good enough repro. It’s using Astro but hopefully that doesn’t really prevent you looking into it. The link to the repro demo is in the linked issue.

https://github.com/withastro/compiler/issues/504#issuecomment-1268959791

lloydjatkinson avatar Oct 06 '22 09:10 lloydjatkinson

I’m looking for a smaller reproduction. The smallest thing possible. So that we can know why it was occurring and that it won’t regress

wooorm avatar Oct 06 '22 10:10 wooorm

Released in 2.1.5!

wooorm avatar Oct 11 '22 17:10 wooorm