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

Generates empty const statement #2112

Closed
4 tasks done
calebeby opened this issue Aug 24, 2022 · 8 comments
Closed
4 tasks done

Generates empty const statement #2112

calebeby opened this issue Aug 24, 2022 · 8 comments
Labels
💪 phase/solved Post is done

Comments

@calebeby
Copy link
Contributor

calebeby commented Aug 24, 2022

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

@wooorm
Copy link
Member

wooorm commented Aug 25, 2022

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

@wooorm
Copy link
Member

wooorm commented Aug 25, 2022

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

@fbubbar
Copy link

fbubbar commented Sep 22, 2022

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} */

@calebeby
Copy link
Contributor Author

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

@wooorm
Copy link
Member

wooorm commented Sep 22, 2022

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 :)

hasparus added a commit to hasparus/zaduma that referenced this issue Oct 5, 2022
used with remark-shiki-twoslash.

@calebeby, who provided the workaround, is the king.

- mdx-js/mdx#2123
- mdx-js/mdx#2112
@lloydjatkinson
Copy link

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.

withastro/compiler#504 (comment)

@wooorm
Copy link
Member

wooorm commented Oct 6, 2022

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 wooorm closed this as completed in 90fa493 Oct 11, 2022
@wooorm wooorm added the 💪 phase/solved Post is done label Oct 11, 2022
@wooorm
Copy link
Member

wooorm commented Oct 11, 2022

Released in 2.1.5!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done
Development

Successfully merging a pull request may close this issue.

4 participants