thlorenz / doctoc

📜 Generates table of contents for markdown files inside local git repository. Links are compatible with anchors generated by github or other sites.
https://www.npmjs.com/package/doctoc
MIT License
4.23k stars 480 forks source link

feat: add mdx support #256

Open I-3B opened 8 months ago

I-3B commented 8 months ago

closes #211

This line may causes problem if some users where only using doctoc for .md files, I didn't check for it, should I?

I-3B commented 8 months ago

@AndrewSouthpaw would you please review this?

chris-dura commented 8 months ago

This line may causes problem if some users where only using doctoc for .md files, I didn't check for it, should I?

So, if I understand, this means that users would all of a sudden start getting TOCs in their .mdx files, which they weren't before... I could see how this would be considered a "breaking change" at worst, and annoying at best to users 😅.

I don't think it's worth forcing a MAJOR version bump, so to keep it backwards compatible, you may want to make the MDX support "opt-in", so the doctoc command does the same thing by default, and you have to explicitly enable mdx support?

I-3B commented 8 months ago

This line may causes problem if some users where only using doctoc for .md files, I didn't check for it, should I?

So, if I understand, this means that users would all of a sudden start getting TOCs in their .mdx files, which they weren't before... I could see how this would be considered a "breaking change" at worst, and annoying at best to users 😅.

I don't think it's worth forcing a MAJOR version bump, so to keep it backwards compatible, you may want to make the MDX support "opt-in", so the doctoc command does the same thing by default, and you have to explicitly enable mdx support?

I can think of two approaches:

chris-dura commented 8 months ago

I like what you're doing with a --syntax argument better... but I'm an idiot and must've forgot to open a PR from my fork where I implemented this for a local project, just for reference:

https://github.com/thlorenz/doctoc/pull/241

Anyway, not something I thought about in mine, but, I would allow --syntax to support just .md/.markdown (default), just .mdx, or "all syntaxes". Then, the default behavior is unchanged, but you can opt-in to doing .mdx files, or all of them?

AndrewSouthpaw commented 8 months ago

That seems reasonable to me. Not breaking the default behavior is important.

I-3B commented 5 months ago

@AndrewSouthpaw hi!

fixed the issue mentioned before here a6ee89ccf7c5f3e0f2bcba4df67bd473979a8b7f, so now mdx files won't get matched unless --syntax mdx is explicitly specified.

AndrewSouthpaw commented 4 months ago

Hi there! Sorry it took so long to get back to you. The code looks good, all it needs now is some test coverage and we can ship it! 🥳

I-3B commented 4 months ago

Hi~ @AndrewSouthpaw added some tests, should cover the new change, would you like me to add more?

I-3B commented 4 months ago

Hi @AndrewSouthpaw ! thanks for the thorough review, can you please go through the new commits to check if they cover the review?

AndrewSouthpaw commented 4 months ago

Perfect! One quick lint cleanup and this is good to go. Thanks @I-3B.