Awesome! This has really good synergy with my own rehype-mermaid package. I have some tips:

1. You dual publish the package, but you use ESM dependencies. This means the CJS version of your package is broken. I suggest to remove the CJS version completely.
2. You dual publish JavaScript, but you only only publish TypeScript type definitions for the ESM version. In package.json you tell TypeScript you’re only publishing ESM. If not for (1), you would have to publish types for both CJS and ESM. See https://arethetypeswrong.github.io/?p=rehype-svgo@1.1.0 for details.
3. I see you use a custom build tool. I strongly recommend using the tsc command to build libraries.
4. Instead of using rehype-parse, you could use the slightly lower level hast-util-from-html-isomorphic package. This also strips positional information, which is appropriate, since you processed the SVG as a whole new document.
5. You might not need unist-util-find. optimizedAst always represents an SVG tree, so you could use const svgElement = optimizedAst.children[0].
6. Jest is pretty bad at dealing with ESM. I strongly suggest using another test framework. At unified and for my own project we’ve had good experiences using node:test. If you like using snapshots, you might like snapshot-fixtures.

Awesome! This has really good synergy with my own rehype-mermaid package.

Coincidentally, that's actually exactly why I made it haha. I use rehype-mermaid on my website (example on this page) and wanted a way to optimize the output a bit :)

I have some tips:

1. You dual publish the package, but you use ESM dependencies. This means the CJS version of your package is broken. I suggest to remove the CJS version completely.
2. You dual publish JavaScript, but you only only publish TypeScript type definitions for the ESM version. In package.json you tell TypeScript you’re only publishing ESM. If not for (1), you would have to publish types for both CJS and ESM. See https://arethetypeswrong.github.io/?p=rehype-svgo@1.1.0 for details.

Good catch. Removed the CJS entry point! Force of habit...

3. I see you use a custom build tool. I strongly recommend using the tsc command to build libraries.

Just personally like using this one to combine a bunch of my dev deps into one thing. No worries though; it uses tsc under the hood!

4. Instead of using rehype-parse, you could use the slightly lower level hast-util-from-html-isomorphic package. This also strips positional information, which is appropriate, since you processed the SVG as a whole new document.

Ayyy nice. Didn't notice this one. Updated!

5. You might not need unist-util-find. optimizedAst always represents an SVG tree, so you could use const svgElement = optimizedAst.children[0].

I tried this, but the tests fail. For whatever reason, after doing AST -> SVG string -> AST I end up with an html, head, and body tags... I might look into why that happens at some point, but for the moment just finding the SVG element within has worked for me

6. Jest is pretty bad at dealing with ESM. I strongly suggest using another test framework. At unified and for my own project we’ve had good experiences using node:test. If you like using snapshots, you might like snapshot-fixtures.

I've actually found it to be okay after you figure out the configuration (thankfully I only had to do this one and now I use the same config everywhere)

svgo likely generates XML, not HTML.
fromHtml might be able to parse that fine, because browsers are lenient.
If you do that, do see it’s options, you want fragment: https://github.com/syntax-tree/hast-util-from-html-isomorphic#example-fragment-versus-document.

Perhaps a slightly more correct way would involve https://github.com/syntax-tree/xast-util-from-xml. It would then still need to map the xast nodes back to hast nodes manually. That step might be a bit complex.

TypeScript works best if you are very explicit. TS cannot narrow visit(tree, { tagName: svg }, ... well, but it can narrow if (node.tagName === 'svg') very well.

So, I’d recommend something along the lines of:

    visit(tree, (node, index, parent) => {
      if (
        parent &&
        typeof index === 'number' &&
        node.tagName === 'svg'
      ) {
        const replacement = fromHtml(
          optimize(toHtml(node, { space: `svg` }), svgoConfig).data,
          { fragment: true }
        )
        parent.children.splice(index, 1, ...replacement.children)
      }
    })

It also looks like svgo uses xast internally already: https://github.com/svg/svgo/blob/main/lib/xast.js.

You likely shave of a lot of time by skipping the serializing/parsing that now happens twice by mapping objects and using the svgo internals: https://github.com/svg/svgo/blob/8d6385bd9ab49d1d300a10268930238baa5eb269/lib/svgo.js#L65-L84.

I would really recommend looking into this, as it’s why unified exists! So then not all these tools have to parse/stringify between everything!

Thanks!

Good feedback by @wooorm! Some additional context: remcohaszing/mermaid-isomorphic#3, https://twitter.com/remcohaszing/status/1777704432039264467

Maybe SVGO is open to addding a public API that accepts and returns a xast tree directly.

Just an idea: It would be cool if you add support for SVG in data URLs in <img> / <source> tags.

svgo likely generates XML, not HTML. fromHtml might be able to parse that fine, because browsers are lenient. If you do that, do see it’s options, you want fragment: https://github.com/syntax-tree/hast-util-from-html-isomorphic#example-fragment-versus-document.

Good catch with the fragment option. That does indeed make it so that I don't end up with the extraneous html, head, and body tags I mentioned!

Perhaps a slightly more correct way would involve https://github.com/syntax-tree/xast-util-from-xml. It would then still need to map the xast nodes back to hast nodes manually. That step might be a bit complex.

TypeScript works best if you are very explicit. TS cannot narrow visit(tree, { tagName: svg }, ... well, but it can narrow if (node.tagName === 'svg') very well.

So, I’d recommend something along the lines of:

    visit(tree, (node, index, parent) => {
      if (
        parent &&
        typeof index === 'number' &&
        node.tagName === 'svg'
      ) {
        const replacement = fromHtml(
          optimize(toHtml(node, { space: `svg` }), svgoConfig).data,
          { fragment: true }
        )
        parent.children.splice(index, 1, ...replacement.children)
      }
    })

TypeScript actually already infers node as Element for visit(tree, { tagName: svg }, (node, index, parent) => { (same thing as your if-statement). So I guess y'all did a good job with the types :)

It also looks like svgo uses xast internally already: https://github.com/svg/svgo/blob/main/lib/xast.js.

You likely shave of a lot of time by skipping the serializing/parsing that now happens twice by mapping objects and using the svgo internals: https://github.com/svg/svgo/blob/8d6385bd9ab49d1d300a10268930238baa5eb269/lib/svgo.js#L65-L84.

I would really recommend looking into this, as it’s why unified exists! So then not all these tools have to parse/stringify between everything!

Neat. I think I agree this would be ideal. I can open up an issue with svgo

Good feedback by @wooorm! Some additional context: remcohaszing/mermaid-isomorphic#3, https://twitter.com/remcohaszing/status/1777704432039264467

Maybe SVGO is open to addding a public API that accepts and returns a xast tree directly.

Just an idea: It would be cool if you add support for SVG in data URLs in <img> / <source> tags.

Optimizing inline SVG in data URLs would also be cool