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

Does not support multi-line HTML comments? #2

Open
slorber opened this issue Jan 6, 2023 · 3 comments
Open

Does not support multi-line HTML comments? #2

slorber opened this issue Jan 6, 2023 · 3 comments

Comments

@slorber
Copy link

slorber commented Jan 6, 2023

When using multi-line HTML comments, this plugin seems to lead to an error

<!-- 

My comment 

-->
{
  err: TypeError: chunks[startIndex].slice is not a function
      at sliceChunks (file:///Users/sebastienlorber/Desktop/projects/docusaurus/node_modules/micromark/lib/create-tokenizer.js:514:32)
      at sliceStream (file:///Users/sebastienlorber/Desktop/projects/docusaurus/node_modules/micromark/lib/create-tokenizer.js:154:12)
      at Object.sliceSerialize (file:///Users/sebastienlorber/Desktop/projects/docusaurus/node_modules/micromark/lib/create-tokenizer.js:149:28)
      at Object.onexitdata (file:///Users/sebastienlorber/Desktop/projects/docusaurus/node_modules/mdast-util-from-markdown/lib/index.js:783:24)
      at compile (file:///Users/sebastienlorber/Desktop/projects/docusaurus/node_modules/mdast-util-from-markdown/lib/index.js:302:40)
      at fromMarkdown (file:///Users/sebastienlorber/Desktop/projects/docusaurus/node_modules/mdast-util-from-markdown/lib/index.js:120:29)
      at parser (file:///Users/sebastienlorber/Desktop/projects/docusaurus/node_modules/@mdx-js/mdx/node_modules/remark-parse/lib/index.js:15:12)
      at Function.parse (file:///Users/sebastienlorber/Desktop/projects/docusaurus/node_modules/@mdx-js/mdx/node_modules/unified/lib/index.js:273:12)
      at executor (file:///Users/sebastienlorber/Desktop/projects/docusaurus/node_modules/@mdx-js/mdx/node_modules/unified/lib/index.js:393:31)
      at new Promise (<anonymous>)
}

Unit tests do not seem to cover multi-line comments.

@leebyron
Copy link
Owner

leebyron commented Jan 6, 2023

Thanks for the report! Have you been able to do any debugging by chance? I noticed that this package didn't show up in that stack trace so any extra information you have about this particular case would be quite helpful for a fast resolution.

Thanks for spotting the opportunity for tests for this case. That seems like the right thing to add once this is figured out.

@slorber
Copy link
Author

slorber commented Jun 22, 2023

Did some tests, and it indeed looks like a bug in this plugin.

const { createProcessor: createMdxProcessor } = await import("@mdx-js/mdx");
const { default: comment } = await import("remark-comment");

The first 2 works fine 👍

createMdxProcessor({
  rehypePlugins: [],
  remarkPlugins: [comment],
  format: "md"
})
  .process({
    value: `
<!-- test md
some html comment.
-->`
  })
  .then(res => console.log(res.toString()));

createMdxProcessor({
  rehypePlugins: [],
  remarkPlugins: [comment],
  format: "mdx"
})
  .process({
    value: `
<!-- test mdx
some html comment.
-->`
  })
  .then(res => console.log(res.toString()));

The last 2 fails with error message above.

createMdxProcessor({
  rehypePlugins: [],
  remarkPlugins: [comment],
  format: "md"
})
  .process({
    value: `
test md
<!--
some html comment.
-->`
  })
  .then(res => console.log(res.toString()));

createMdxProcessor({
  rehypePlugins: [],
  remarkPlugins: [comment],
  format: "mdx"
})
  .process({
    value: `
test mdx
<!--
some html comment.
-->`
  })
  .then(res => console.log(res.toString()));

More info about variables in the method that throws:

{
  chunks: [
    -4,
    'test md',
    -4,
    '<!--',
    -4,
    'some html comment.',
    -4,
    '-->',
    null
  ],
  startIndex: 4,
  endIndex: 4,
  startBufferIndex: -1,
  endBufferIndex: -1
}

{
  chunks: [
    -4,
    'test mdx',
    -4,
    '<!--',
    -4,
    'some html comment.',
    -4,
    '-->',
    null
  ],
  startIndex: 4,
  endIndex: 4,
  startBufferIndex: -1,
  endBufferIndex: -1
}

Note that using this comment with an extra useless space after the opening makes it work:

<!-- 
some html comment.
-->

This workaround makes all 4 test cases pass:

fileContent = fileContent.replaceAll('<!--\n', '<!-- \n');

@slorber
Copy link
Author

slorber commented Jun 22, 2023

Sent a draft PR with a failing test case: #3

Not super familiar with the code in this repo so not sure I'll be able to fix it easily 😅 any idea what could be wrong in the current implementation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants