using a transformer to add a wrapping element results in extra duplicative nodes

olets commented 1 week ago

Validations

[X] Follow our Code of Conduct
[X] Read the Contributing Guide.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.

Describe the bug

I'm working on a transformer which adds a root wrapper. Minimal goal is

<div data-my-attr="val">
  <pre class="shiki <theme>" style="…" tabindex="0">
    <code class="language-js">
    …
    </code>
  </pre>
</div>

I'm using @shikijs/markdown-it.

root hook

My first thought was to use the root hook:

        root(hast) {
          return [
            {
              type: 'element',
              tagName: 'div',
              properties: {
                'data-my-attr': 'val',
              },
              children: hast.children,
            },
          ];
        },

Given

```js


that transformer results in this HTML (I added the comments)

```html
<pre> <!-- unexpected wrapper 1 -->
  <code class="language-js"> <!-- unexpected wrapper 2 -->
    <div data-my-attr="val">
      <pre class="shiki <theme>" style="…" tabindex="0">
        <code class="language-js">
          <span class="line"></span>
        </code>
      </pre>
    </div>
  </code>
</pre>

postprocess hook

Using a postprocess hook is a better fit for my need, because I'd like to use options.meta.

That doesn't work either:

        postprocess(html) {
          return `<div data-my-attr="val">${html}</div>`;
        },

Given the same markdown as in the root hook example above, I get the same problematic result as above.

Note that if we console log html in this hook, it does already have the pre > code wrappers.

Reproduction

https://stackblitz.com/edit/shikijs-shiki-issue-811

Contributes

[ ] I am willing to submit a PR to fix this issue
[ ] I am willing to submit a PR with failing tests

fuma-nama commented 2 days ago

Make sure to return a Root element:

function transformerTab(): ShikiTransformer {
  return {
    name: 'rehype-code:tab',
    root(root) {
      return {
        type: 'root',
        children: [
          {
            // anything you want ;)
            children: root.children,
          },
        ],
      };
    },
  };
}

olets commented 2 days ago

Thanks for the correction. I see now that that's made clear in the types. Still looks like there isn't a way to get this to work with a root transformer https://stackblitz.com/edit/shikijs-shiki-issue-811-bwecfk?file=main.js. Feels like a bug but maybe I'm misunderstanding "root".

And again a postprocess hook is really what I need, to have access to the rest of the fence. If anyone has a solution for that please share 🙏

(And if anyone finds this issue because you're trying to migrate from shikijs/twoslash/remark-shiki-twoslash or shikijs/twoslash/eleventy-plugin-shiki-twoslash with a custom transform to @shikijs/markdown-it to get Shiki v1: for now I'm using my forks https://github.com/olets/shiki, which has v1's languages and themes)

fuma-nama commented 2 days ago

oh you're right, I overlooked that. Markdown-it wraps the result from highlight with <pre> and <code> if the result does not.

See https://github.com/markdown-it/markdown-it/issues/269, this can be disabled with some configurations.

Or the jsdoc of markdown-it:

    /**
     * Highlighter function for fenced code blocks.
     * Highlighter `function (str, lang, attrs)` should return escaped HTML. It can
     * also return empty string if the source was not changed and should be escaped
     * externally. If result starts with <pre... internal wrapper is skipped.
     * @default null
     */
    highlight?: ((str: string, lang: string, attrs: string) => string) | null | undefined;
}

olets commented 1 day ago

Great find!

Okay now I have working implementation of @shikijs/markdown-it with a postprocess transformer where the outermost element isn't a <pre>. As mentioned in the issue you linked, it requires copypasting markdown-it's source. https://stackblitz.com/edit/shikijs-shiki-issue-811-solution-for-markdown-it-14-1-0?file=main.js

I've proposed a change to markdown-it https://github.com/markdown-it/markdown-it/issues/269#issuecomment-2445155660, but the maintainer has said no to related ideas before.

Maybe as a bandaid we could add my stackblitz's fence rule to the @shikijs/markdown-it exports? Then users using a root transform or postprocess transformer to add a wrapper around the <pre> could

import shikiMarkdownItPlugin, { wrapperlessMarkdownItFenceRule } from '@shikijs/markdown-it';
import MarkdownIt from 'markdown-it';

const md = MarkdownIt();

md.renderer.rules.fence = wrapperlessMarkdownItFenceRule;

md.use(await shikiMarkdownItPlugin({
  // …
  transformers: // …,
}));

shikijs / shiki