leebyron / remark-comment

Remark plugin to support comments
MIT License
7 stars 4 forks source link

Does not support multi-line HTML comments? #2

Open slorber opened 1 year ago

slorber commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Sent a draft PR with a failing test case: https://github.com/leebyron/remark-comment/pull/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?