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

Inline comment directly after jsx block fails to parse. #784

Closed
mjhenkes opened this issue Sep 16, 2019 · 5 comments · Fixed by #1039
Closed

Inline comment directly after jsx block fails to parse. #784

mjhenkes opened this issue Sep 16, 2019 · 5 comments · Fixed by #1039
Labels
💪 phase/solved Post is done 👶 semver/patch This is a backwards-compatible fix 🐛 type/bug This is a problem 🦋 type/enhancement This is great to have

Comments

@mjhenkes
Copy link

Subject of the issue

Adding an inline md comment directly after an inline html/jsx block in a markdown file causes mdx to fail to parse.

Similar to this issue: #243

Your environment

  • OS: macos
  • Packages: "@mdx-js/react": "^1.4.5",

Steps to reproduce

ON the sample site, add an inline comment to the in html example.

# Below is a JSX block

<div style={{ padding: '10px 30px', backgroundColor: 'tomato' }}>
  <h2>Try making this heading have the color green</h2>
</div>
<!-- AUTO-GENERATED-CONTENT:END -->

🎉 BONUS POINTS for creating a minimal reproduction and uploading it to GitHub. This will get you the fastest support. 🎉

Expected behaviour

The comment is ignored.

Actual behaviour

Parsing fails.

@mjhenkes mjhenkes added 🐛 type/bug This is a problem 🙉 open/needs-info This needs some more info labels Sep 16, 2019
@johno johno added 🙆 yes/confirmed This is confirmed and ready to be worked on 🐛 type/bug This is a problem and removed 🐛 type/bug This is a problem 🙉 open/needs-info This needs some more info labels Sep 16, 2019
@danon
Copy link

danon commented Apr 7, 2020

Successful parse:

<!-- Comment -->
<Component/>
<Component/>

<!-- Comment -->

Failure:

<Component/>
<!-- Comment -->
<!-- Comment --><Component/>
<Component/><!-- Comment --> 

@wooorm
Copy link
Member

wooorm commented Apr 18, 2020

Heya, I’m working on a replacement for the current JSX parsing to solve many of the outstanding issues, like this one.

I have one Q about this: why should <!--comments like this--> be supported in MDX? We replace HTML with JSX, so wouldn’t it make more sense to only support <>{/* comments like this */}<>?

@danon
Copy link

danon commented Apr 18, 2020

@wooorm Good question. I reckon MD doesn't have a syntax for comments, so we were left with HTML comment. Nor does JSX have comments, really.

I'm not sure about <>{/* comments like this */}<> since:

  • that's 10 characters of comment syntax (pragmatic? not sure)
  • That's not a JSX comment, actually. That's actually JS comment, inside {} syntax inside JSX inside MDX.

But I reckon that, if we, in fact, stop treating HTML as a valid MDX, in favour of JSX as a valid MDX, then yes, indeed JS comment in JSX syntax is more "proper" than HTML comment, yes.

(just not sure whether we do xd)

@wooorm
Copy link
Member

wooorm commented Dec 11, 2020

Just circling back to report that the next branch supports my earlier idea, and you don’t need fragments, this works:

{/* comments like this */}

Which is one character less than HTML comments!

@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 👶 semver/patch This is a backwards-compatible fix 🦋 type/enhancement This is great to have 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
💪 phase/solved Post is done 👶 semver/patch This is a backwards-compatible fix 🐛 type/bug This is a problem 🦋 type/enhancement This is great to have
Development

Successfully merging a pull request may close this issue.

4 participants