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

When using commonmark, <var> inside of <code> results in a codeblock #10533

Open
7 tasks done
robert-j-webb opened this issue Sep 27, 2024 · 9 comments
Open
7 tasks done
Labels
bug An error in the Docusaurus core causing instability or issues with its execution domain: markdown Related to Markdown parsing or syntax

Comments

@robert-j-webb
Copy link

robert-j-webb commented Sep 27, 2024

Have you read the Contributing Guidelines on issues?

Prerequisites

  • I'm using the latest version of Docusaurus.
  • I have tried the npm run clear or yarn clear command.
  • I have tried rm -rf node_modules yarn.lock package-lock.json and re-installing packages.
  • I have tried creating a repro with https://new.docusaurus.io.
  • I have read the console error message carefully (if applicable).

Description

Heres the rerpo of the issue - you can see the demo
here

This is the problem code:

# intro.md:

This is <code>inline <var>var</var></code>

Results in

image

However, it should not result in a code block, instead it should still be inline.

Note that if you remove commonmark support by editing docusaurus.config.ts to not have:

    markdown: {
      format: 'detect',
    },

Then it works with inlining.

I know commonmark support is experimental, and I am not expecting a quick fix. I am merely hoping my issue report will help improve support for commonmark when it does work. Thanks a ton for all of your work on this project :)

Self-service

  • I'd be willing to fix this bug myself.
@robert-j-webb robert-j-webb added bug An error in the Docusaurus core causing instability or issues with its execution status: needs triage This issue has not been triaged by maintainers labels Sep 27, 2024
@krittika019
Copy link

is this issue open ?

@robert-j-webb
Copy link
Author

is this issue open ?

Yes

@azamshaikh1103
Copy link

Hey @robert-j-webb, are you working on this issue ???

@robert-j-webb
Copy link
Author

Hey @robert-j-webb, are you working on this issue ???

No, I'm not a maintainer though so I have no say in whether or not it should be worked on which we should wait for before we start doing anything.

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Oct 2, 2024

My hypothesis is that it's due to the logic here: https://github.com/facebook/docusaurus/blob/16500436f7b14a6801f9ae466ef405ab29a22b84/packages/docusaurus-theme-classic/src/theme/MDXComponents/Code.tsx

All <code> elements are rendered using this MDXCode, which determines whether it should be rendered as <code> or a <pre> by checking whether it has string contents or nested code. I don't remember the original intention for this but we have always treated <code> the same as <pre> to support things like:

<code>{`
function foo() {
  return 1;
}
`}</code>

I'm not familiar enough with our CommonMark infrastructure to tell why it only happens with CM, though. Investigation is welcome.

@Josh-Cena Josh-Cena added domain: markdown Related to Markdown parsing or syntax and removed status: needs triage This issue has not been triaged by maintainers labels Oct 2, 2024
@scottamain
Copy link

scottamain commented Oct 2, 2024

we have always treated <code> the same as \<pre\> to support things like:

<code>{`
function foo() {
  return 1;
}
`}</code>

What's the reasoning to do that?

The <code> element is designed for inline code and the <pre> block is designed for block code. Changing <code> to become <pre> is a violation of the author's intention—it's like saying, "I know better than you what you want." And changing it reinforces bad behavior. People should be forced to learn the correct HTML semantics.

EDIT: @Josh-Cena sorry, you did say, "I don't remember the original intention," so my question is not addressed at you directly. But it's also mostly rhetorical... IMO, this is clearly a bug.

@Josh-Cena
Copy link
Collaborator

I'm happy to always render <code> as inline regardless of what's inside it, given that it's compatible with what people expect and/or are already doing. We actually have testing for this: https://docusaurus.io/tests/pages/code-block-tests (which is actually broken atm)

And, blaming that test, I dug up some more context: #6767, #6177, #2857. It seems that this has been the case ever since #1555, and we don't know the intention anymore since the author is no longer there.

@slorber
Copy link
Collaborator

slorber commented Oct 3, 2024

😄 It's been a while.

We'll never know exactly but to me the goal is to be able to distinguish Markdown triple-backticks code blocks (that MDX automatically wraps in <pre>) from Markdown inline code blocks, that all use the CodeBlock component.

Note that things used to be different under MDX v1 and now MDX v2 also has a different behavior. I think we can get rid of some complexity here indeed.

For example these historical things:

 function MDXPre(props: Props): ReactNode | undefined {
  // With MDX 2, this element is only used for fenced code blocks
  // It always receives a MDXComponents/Code as children
  return <>{props.children}</>;
}
// Solution inspired by https://github.com/pomber/docusaurus-mdx-2/blob/main/packages/mdx-loader/src/remark/codeCompat/index.ts
// TODO after MDX 2 we probably don't need this - remove soon?
// Only fenced code blocks are swapped by pre/code MDX components
// Using <pre><code> in JSX shouldn't use our MDX components anymore

// To make theme-classic/src/theme/MDXComponents/Pre work
// we need to fill two properties that mdx v2 doesn't provide anymore
export default function codeCompatPlugin(this: Processor): Transformer {
  return async (root) => {
    const {visit} = await import('unist-util-visit');

    visit(root, 'code', (node: Code) => {
      node.data = node.data || {};

      node.data.hProperties = node.data.hProperties || {};
      node.data.hProperties.metastring = node.meta;

      // Retrocompatible support for live codeblock metastring
      // Not really the appropriate place to handle that :s
      node.data.hProperties.live = node.meta?.split(' ').includes('live');
    });
  };
}

Will need to study all that in depth to figure out what we should do, thanks for reporting.

@slorber
Copy link
Collaborator

slorber commented Oct 3, 2024

For future references, here's how MDX compiles:

CleanShot 2024-10-03 at 12 37 15

And here's how it compiles CommonMark:

CleanShot 2024-10-03 at 12 38 20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An error in the Docusaurus core causing instability or issues with its execution domain: markdown Related to Markdown parsing or syntax
Projects
None yet
Development

No branches or pull requests

6 participants