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

mdx v2: New line inside DOM element (jsx) creates unnecessary paragraph #1798

Closed
ksinas opened this issue Nov 5, 2021 · 26 comments
Closed
Labels
🙅 no/wontfix This is not (enough of) an issue for this project

Comments

@ksinas
Copy link

ksinas commented Nov 5, 2021

New line inside DOM element (jsx) creates unnecessary paragraph

Steps to reproduce

in playground editor, input the following

<p>
  Hello world
</p>
<div>
  <span>
    one
  </span>
  <span>
    two
  </span>
</div>

Expected compiled result

<p>
  Hello world
</p>
<div>
  <span>
    one
  </span>
  <span>
    two
  </span>
</div>

Actual compiled result

<p>
  <p>
    Hello world
  </p>
</p>
<div>
  <span>
    <p>
      one
    </p>
  </span>
  <span>
    <p>
      two
    </p>
  </span>
</div>

this ends up being invalid HTML

@wooorm
Copy link
Member

wooorm commented Nov 5, 2021

This is expected behavior. It’s different from before, but I believe the new rules are in the end more sensical than the previous rules.
See Interleaving and Migrating

@ksinas
Copy link
Author

ksinas commented Nov 8, 2021

I understand the intention, but I would like to be able to disable md parsing inside jsx. Furthermore, I noticed that if you have an expression in a new line, it will not create p element, which in my opinion is inconsistent behaviour.

<div>                        <div>
  foo               =>         <p>foo</p>
</div>                       </div>
export const foo = 'bar';

<div>                        <div>
  {foo}             =>         bar
</div>                       </div>

@wooorm
Copy link
Member

wooorm commented Nov 8, 2021

but I would like to be able to disable md parsing inside jsx

I noticed that if you have an expression in a new line, it will not create p element

The first is solved by the latter! You can use an expression if you don’t want markdown.

A lot of people do want markdown inside JSX, hence the change:

<MyNote>
  ```js
  console.log(1)
  ```
</MyNote>

@wooorm
Copy link
Member

wooorm commented Nov 10, 2021

Did that answer your question? Can this be closed?

@ksinas
Copy link
Author

ksinas commented Nov 10, 2021

I understand what you mean by "expected behavior". However it breaks our use case. We use mdx to create complex content with custom components. People who are using it, they have very basic understanding about HTML. It is not easy to control whether a user adds text on the new line or not. And it is not possible to teach them about expressions.

So while I'm happy to see the capabilities of the new version and awesome new features, It's painful not to be able to switch off the md parsing within jsx context by providing an option.

@wooorm
Copy link
Member

wooorm commented Nov 10, 2021

I don’t really understand. Authors are extremely experienced at JSX making complex content, but don’t know how HTML or JS work? That seems... counter intuitive? Who are these people that can’t be taught?

If you don‘t want markdown, I mean… Why not use JSX directly?

@ksinas
Copy link
Author

ksinas commented Nov 10, 2021

Authors are not experienced in programming. But they are savvy enough to understand the markup (HTML). By complex I mean, they have a list of custom markup components, which they can use. Some of these components are simply styled components, but some adds certain functionality to the content. But if they would do somethng like this

<CustomHeader>
  header
</CustomHeader>

The HTML will become invalid.

We use mdx because of how it compiles a component in runtime, injecting predefined components from the library.

@wooorm
Copy link
Member

wooorm commented Nov 10, 2021

So there are no expressions or esm. Nothing about JSX other than that components are passed and used?

@ksinas
Copy link
Author

ksinas commented Nov 10, 2021

We have a platform (used internally by one company) which allows to bootstrap small websites for live conferences. The platform has standard features like account management, live streaming, interactivity. But some content has to be tailor made. We decided to use mdx for that reason, where it is compiled on runtime.

Sometimes we use expressions like <Typography>Hi {props.name}</Typography >.
Which usually are provided as standard templates for the editors.

We just don't use markdown. It's probably very non standard use case, but it worked great for us so far. Though I must say, some templates I've seen were very messy... :D And I'm just afraid that with the extra md parsing within jsx context will make it even messier. Therefore it would be great if this would be optional.

If you would like to know more about how do we use this library, we could take it off from this thread.

@wooorm
Copy link
Member

wooorm commented Nov 10, 2021

where it is compiled on runtime.

Do you mean compiled on the server, and that it’s static generated?

Sometimes we use expressions like <Typography>Hi {props.name}</Typography>.

Is that what y’all at this company, or what authors of MDX, do?

We just don't use markdown

How do you mean? You’re not using emphasis, paragraphs, headings?

And I'm just afraid that with the extra md parsing within jsx context will make it even messier.

It’s a change. But it’s something that tons of people have requested because the previous way to do it didn’t make sense.
The way you explain this all seems to ignore the fact that it was possible before, but harder.
Folks already had to know about the difference (blank lines switched to markdown).

Therefore it would be great if this would be optional.

If you don’t want JS(X), you can choose to go with just markdown. Use .md as an extension. No JSX, ESM, or expressions. You can still pass replacements for h1 and the like

@ksinas
Copy link
Author

ksinas commented Nov 11, 2021

Do you mean compiled on the server, and that it’s static generated?

No, it is compiled on frontend. As I mentioned, we use this package not in a traditional way. We just store a string of mdx, and pass it to a compiler component something like this.

import { compileSync } from '@mdx-js/mdx';
import { jsx, Fragment, jsxs } from 'react/jsx-runtime';
import { useMemo } from 'react';

const Compiler = ({ source, ...props }) => {
  const Component = useMemo(() => {
    const code = compileSync(source, { useDynamicImport: true, outputFormat: 'function-body' });
    const fn = new Function(code.value);
    return fn({ jsx, jsxs, Fragment }).default;
  }, [source]);

  return (
    <Component components={components} {...props} />
  );
};

Is that what y’all at this company, or what authors of MDX, do?

Authors, who are the part of the company, but not developers.

How do you mean? You’re not using emphasis, paragraphs, headings?

Our use case allows, for example, adding a button which would open certain dialog, or a link, navigating to another route without reloading the entire page (single page app). All compiled on front end. So for us the important part is JSX, and markdown syntax is something good to have, but I would like to have it optional within JSX context.

If you don’t want JS(X), you can choose to go with just markdown. Use .md as an extension. No JSX, ESM, or expressions. You can still pass replacements for h1 and the like

I'm saying quite the opposite. We want JSX, I just don't want invalid HTML, which is out of my control.

@wooorm
Copy link
Member

wooorm commented Nov 11, 2021

We just store a string of mdx, and pass it to a compiler component something like this.

I’d recommend at least using evaluate on the client, it does most of what you’re doing for you.
It might be better to compile on the server and run on the client. See: #1792 (comment). Note that run is not yet exposed from the main package, but you can get it from @mdx-js/mdx/lib/run.js.

Authors, who are the part of the company, but not developers.
So for us the important part is JSX

In-house authors who write mostly JSX, sometimes expressions, and rarely markdown. I’d personally call those programmers.
Given that it’s possible to create invalid HTML with JSX itself, which was also possible before in MDX (with blank lines), I don’t get the argument that now having to learn about interleaving is too much to ask.

I would like to have it optional within JSX context.

You make this sound trivial, I don’t think it is. What even is a JSX context? MDX@1 used:

<Box>

# heading

</Box>

Folks didn’t get that. There have been so many issues and discussions about it: to name a few, #1312, #1170. This new behavior is different from MDX@1 yes. It was highly requested and I agree.

Now take this:

<Box>
  # heading
</Box>

^-- this looks like a heading in a box to me.

What about:

<Box>
  *emphasis*?
</Box>

<Box>*emphasis*?</Box>

^-- both look like emphasis to me. Should one be a paragraph? That’s up for debate.

You need some rule because “JSX context” is vague and the goal of MDX is combining JSX and markdown. The previous rule didn’t make sense. I think this is probably the best the rule can be. Because the headings are headings and the emphasis is emphasis. And the paragraph is there in the first case which matches how the heading works.
The rule is explained in Interleaving: text and tags on the same line contain “inline” markdown, on separate lines they contain “block”s.

I just don't want invalid HTML, which is out of my control

There are many ways to create invalid HTML with MDX, JSX, or even HTML itself. There are also ways to use MDX and JSX that have nothing to do with HTML (native development, rendering to terminals, etc).
There are also several good tools that can check whether HTML is valid. https://validator.w3.org, React itself, etc.
And you have quite technical authors that you can ask to read a manual.

I personally don’t see why these technical authors can’t be asked to read a manual. Or why there is no way to check whether they’re writing valid HTML.

@ksinas
Copy link
Author

ksinas commented Nov 11, 2021

I hear what you're saying. Thanks for the discussion. I understand that the option I would be interested in will not be implemented, so you can close the issue.

Anyway, thanks for the great work!

@wooorm wooorm closed this as completed Nov 11, 2021
@wooorm wooorm added 🙅 no/wontfix This is not (enough of) an issue for this project and removed 🐛 type/bug This is a problem 🔍 status/open labels Nov 11, 2021
@andrewplummer
Copy link

I'm baffled how the new behavior can be considered anything but a bug. In the first place standard paragraphs in markdown require two new lines, not one. Changing to a single line is inconsistent with markdown and seems to be leading to a lot of confusion. Then on top of that there doesn't seem to be a straightforward way to achieve the expected behavior.

Something as simple as:

<p>
  line
</p>

becomes a nested validation error... why?

On top of this you have people using prettier which forces a code syntax (as it should) and suddenly these ghost tags start appearing.

The previous behavior should minimally be configurable, and probably the default.

@ChristianMurphy
Copy link
Member

I'm baffled how the new behavior can be considered anything but a bug.

This was discussed at length in #1170
The new style is more consistent and sensible than the previous rules (#1798 (comment))
The alternative you suggest is considered a bug by many (E.G. #1312)

In the first place standard paragraphs in markdown require two new lines, not one.

The area you are referring to isn't markdown though.
It is JSX, transitioning to a new block (either a new JSX block or a new Markdown block).

On top of this you have people using prettier which forces a code syntax (as it should) and suddenly these ghost tags start appearing

This is an issue with Prettier, which currently only supports MDX 1, this should be resolved when prettier 2 support is added prettier/prettier#12209
In the meantime, prettier has initial support for MDX 2 comments, which can be used to disable unwanted re-formatting https://prettier.io/blog/2021/11/25/2.5.0.html#mdx

The previous behavior should minimally be configurable.

It is configurable, though plugins, as noted here: #1170 (comment) and with a related example here: #1170 (comment)

@andrewplummer
Copy link

The alternative you suggest is considered a bug by many

Well in all fairness they are expecting to write markdown without following markdown's rules (two new lines per block). But whatever, I'm sure this discussion has been had a number of times and there are reasonable expectations on both sides. Sounds like a good candidate for configuration! I don't mean a plugin that works around the issue by "unwrapping" blocks but a core configuration as this is fundamental behavior.

The area you are referring to isn't markdown though. It is JSX, transitioning to a new block

I'm rather confused by this statement. Where specifically are you referring to? It clearly is trying to render markdown inside a component. I think I see what you're saying but I don't see how it isn't markdown from the last > to the first <

fwiw I would like to turn off all nested markdown rendering (this is how I ended up on this bug). I am simply trying to use flat markdown alongside JSX components. In other words if a JSX context is opened with any tag markdown should not be parsed inside. Is there a way to do this?

It seems like a lot of the issues that are arising here (the above prettier issue included) stem from the trickiness that comes from trying to mix a format that is inherently flat where all newlines have meaning (markdown) with one that inherently has a nested structure (jsx).

If I were approaching from a clean slate I would expect the behavior described above (no nested markdown) to be the default and a special syntax (or mdx provided component) would re-open a flat markdown context to allow nesting to be opted into as I'm reasonably confident in saying it's probably not the core use case (after all, the flat, free-form style of markdown is it's strength, getting tricky by nesting that is going against it to a degree). This would cleanly sidestep all of the issues people are having.

I realize this is easy to say and I'm sure there's been a billion discussions and for all I know it used to work this way. Still, I think it's a valid perspective.

Lest I sound too critical btw, MDX is a godsend and sorely needed. Thank you for all the work you've done here!

@wooorm
Copy link
Member

wooorm commented Feb 21, 2022

Wrap JSX in an expression if you don’t want markdown-in-jsx:

# markdown

{
  <div>
    Just JSX text, *not markdown*
  </div>
}

> markdown again

More information on how JSX and markdown interleave in MDX is explained in the docs: https://mdxjs.com/docs/what-is-mdx/#interleaving

@andrewplummer
Copy link

So let me be sure I have this right...

If I have a long document with large text inside a component or html element:

<p id="some-id">
  Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod
  tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim
  veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea
  commodo consequat. Duis aute irure dolor in reprehenderit in voluptate
  velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint
  occaecat cupidatat non proident, sunt in culpa qui officia deserunt
  mollit anim id est laborum.
</p>

My only options are either to:

  1. Put each paragraph's content on a single line or
  2. Wrap every instance of them in {}

Otherwise I will always have nested <p> tags and potential validation errors. Is that correct?

@wooorm
Copy link
Member

wooorm commented Feb 22, 2022

Not at all. The link explains different mechanics.
It also feels like you’re changing the goalposts: previously you asked about “never entering markdown again and just staying in JS(X)”.

Your phrasing doesn’t match your code. You paint a picture as if paragraphs can’t be made with MDX, it’s instead possible and you don’t need JSX:

Lorem ipsum dolor sit amet

I don’t understand the reason for your code: why would you put an ID on a paragraph? Practically I’d recommend adding slugs to headings instead (rehype-slug). And if you want to add IDs to terms, you can use a link-to-self:

Lorem <a id="ipsum" href="#ipsum">ipsum</a> dolor

If you mean that you have a theoretical problem, “can attributes by put on <p>, <h1>, etc”, then there are several ways to do that, as explained in this thread, on the website, and some of them listed by you already. Here’s one more:

<p id="some-id">Lorem ipsum dolor sit amet,
consectetur adipiscing elit,
sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.</p>

@andrewplummer
Copy link

The id was simply to ensure that it was clear that there are times when you want to use html over markdown syntax as this is perfectly allowed by markdown.

But let's not get hung up on the p tag or the id as they're not relevant. Imagine any component:

<Container>
   some long text.......
</Container>

@wooorm
Copy link
Member

wooorm commented Feb 22, 2022

Right. There are times you want markdown. There are also times you want JSX. And there are times when you want markdown inside JSX. So there are rules.

Taking your new example: there are cases where you want a p in there. There are cases where you don’t want a p in there. There are cases where you want a p around it. So there are rules.

Markdown’s rules for HTML didn’t make sense to many users, so we needed different rules.
It’s cool that you don’t need part of MDX, but some people do. I quit enjoy it personally. I find it very useful in cases like our website.
So there are rules. These are the rules: https://mdxjs.com/docs/what-is-mdx/#interleaving

@andrewplummer
Copy link

And which do you think is the more common use case? Genuine question.

@wooorm
Copy link
Member

wooorm commented Feb 22, 2022

Please understand that this is open source. I was under the impression that I was helping a confused user. It appears you are looking for an argument and "being right". I'm not interested in that.

@andrewplummer
Copy link

You seem to be reading a lot into my motives. I'm sure you probably deal with a lot of people who are as you describe above but I assure you I am genuinely interested in which is the more common use case, which is something that undoubtedly know better than me. It seems to me unlikely that markdown nested inside JSX would be the more common use case (for reasons described above) but perhaps I'm totally off the mark about that?

Also for the record I am still a bit confused, as my question above was whether those 2 solutions to my use case were the only options, to which you replied "not at all", but then proceeded to describe only why my use case was invalid, then link to the same article that described, as far as I can see, 2 solutions. I suppose I'm meant to infer that 2 solutions is plenty and I'm being arrogant for asking for something else? But I never meant that and I really was curious if perhaps I'm doing it wrong?

@ChristianMurphy
Copy link
Member

@andrewplummer

You seem to have missed or be unclear that the original ask, skipping some paragraphs, is supported/configurable today with plugins (noted in the first reply to you #1798 (comment), and would be conceptually similar to remark-unwrap-images)

You also seem to unclear what MDX is, it is JSX mixed with Markdown.
You seem to want something that isn't JSX mixed with Markdown, if so you may want another language.
Maybe plain markdown with component replacement (like https://github.com/remarkjs/react-markdown)?

@ChristianMurphy
Copy link
Member

More broadly this issue has gone off topic and is no longer constructive.

For support with syntax or using plugins the discussion forum is open https://github.com/mdx-js/mdx/discussions

A friendly reminder for folks on both issues and discussions.
"I have a specific problem, how can it be solved?" are conversations which can be beneficial.
"I have an opinion, and am in search of a problem to opine on" rarely goes anywhere.

@mdx-js mdx-js locked as off-topic and limited conversation to collaborators Feb 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🙅 no/wontfix This is not (enough of) an issue for this project
Development

No branches or pull requests

4 participants