Format markdown in TOC by default

[coryschires]

Fixes #19

Problem

Currently, markdown is not rendered within the TOC. Given the following markdown text:

# My **bold** title

The TOC should render as:

<li>My <strong>bold</strong> title</li>

But instead is rendered showing the raw markdown syntax:

<li>My **bold** title</li>

For more details and discussion around this change, see #19.

Solution

As suggested in #19, I have updated the default behavior to always format markdown within the TOC.

Additional notes

• My text editor automatically removed the trailing whitespace. So the diff is a little noisy – sorry about that.
• I had to update several of the fixture, replacing <span>1</span> with <span>1</span>. But this change is actually an improvement, imo. By default, markdown-it escapes raw HTML unless you pass html: true. With my change markdown-it-table-of-contents now also follows this config option (i.e. if you set html: true then HTML will reader, otherwise it will be escaped).

Conflict with forceFullToc?

Unfortunately, this change seems to conflict with the forceFullToc option. The test suite overflows in an endless loop (see inline code comment). This should be fixable? Honestly, I was having a little trouble understanding the code in this area.

While I am not using forceFullToc, it definitely sucks to regress on this feature. I'd appreciate any advice / suggestion to keep forceFullToc working as before.

Thanks again for creating this library!

[martinlissmyr]

Sounds good @coryschires !

@GiridharanNarayanan do you think you could help out here as it seems @coryschires has run into some issues with some aspect of forceFullToc that you've contributed.

[coryschires]

Hey, wanted to follow up with another possible solution. Not as good, imo. But worth mentioning.

Rather than parse markdown by default, we could at least expose the md object as the second argument to the format function. This way, users could at least process the markdown using the internal renderer. That would solve my problem and only require that I add one extra line of configuration to my code (i.e overwriting the format function).

This solution would probably un-block this PR because the default behavior would no longer conflict with forceFullToc – so that's good. But, at least imo, it would be a poor tradeoff. I think the TOC should support markdown by default and having that feature is more beneficial than supporting an unconventionally nested header hierarchy (which, afaict, is the goal of forceFullToc).

Anyway, if you'd like to move forward with this idea, let me know and I will update the PR accordingly.

[GiridharanNarayanan]

@coryschires , Apologies for late response. I am a bit held up with deliverables at work. You could go ahead with your new solution if @martinlissmyr is fine with it. By next week I should have time to have a look at this and I can try to fix the issue and make the MD rendering a default.

[martinlissmyr]

I think that the original intention here, to parse markdown per default, is better. If you @coryschires think it's fine to wait for @GiridharanNarayanan to fix any conflicting issues I would prefer that as I believe it makes things a bit less brittle.

[coryschires]

Sure. I don't mind waiting. No rush as I can always point to my branch in the meantime.

[coryschires]

Hi @martinlissmyr,

As mentioned above, I decided to point to my branch locally, as that fixes the bug on my end.

Unfortunately, along the way, I encountered another issue: My JS tests were failing, complaining about const. This error helped me notice that you updated to ES6 syntax as part of the 0.4.0 release.

ES6 is, of course, a huge improvement. However, in my experience, libraries normally transpile ES6 and thus expose an ES5 version for use by clients applications. For example, markdown-it-footnote uses browserify to transpile an ES5 version to dist/.

To workaround this issue, I have updated by PR and removed all ES6 syntax (i.e. =>, const, let, and backticks). So I'm good to go.

However, I think this library could be improved by doing either:

1. Remove all ES6 syntax so that transpilation is not necessary (i.e. what I have done in my PR).
2. Transpile markdown-it-table-of-contents using Babel or Browserify or whatever. Should only be a few additional lines of code.

Are you open to either of these solutions? Changes could be in this pull request or in a separate PR.

[martinlissmyr]

@coryschires My intention was for this to be used in Node, so for me ES6 was fine. Didn't think further than that :)

However, as using ES6 doesn't really "do" anything I'm open to just revert that as you've done... Could you do it in a separate PR? If so I'm glad to merge it right away.

[mirisuzanne]

Is this still being considered? I'm using ToC in a code-heavy repo, and don't want to lose code formatting in my headings. Would love to see this merged.

[martinlissmyr]

Well, this PR became rather messy and confusing. Not sure where it stands right now. If you are willing to clean it up, make sure tests pass, rebase it from master and so on I can merge it.

[coryschires]

Hey. I can take a fresh look at this PR. It's been a while but the changes are still relevant / needed, imo. I'll follow up once I have things cleaned up.

[coryschires]

I have updated this PR and it is now synced with the latest master. Because there's been quite a bit of discussion on this thread, I will recap the significant changes in the hope of getting this merged quickly.

1. Update code to use ES5 syntax

As you can see in the above discussion, I think this project would be improved by migrating to plain ol' ES5 syntax. This ensures interoperability between node and browser environments. While I'm sure everyone agrees modern ES syntax is a huge improvement, I don't think it's a good trade off for this project given its small size.

Alternatively, you could transpile ES6 and expose an ES5 version for use by clients applications. This is the pattern used by other markdown-it extensions such as markdown-it-footnote. This would give the best of both worlds – i.e. interoperability and modern syntax – but would also require additional tooling which, imo, felt overkill given the smallness of this project.

Unfortunately, these changes do make the PR a bit noisy – but not too bad imo. If someone wants to make a separate PR dedicated to this change, that may be a good idea. That said, I think the PR is still legible (again, just my opinion) and I'm not interested in dividing this work into two separate PRs.

2. Updates the existing format option to render markdown by default

This is the most significant and important change. In my opinion, the current behavior – not rendering markdown in TOC headers – is plainly a bug. Similar markdown-it extensions render markdown by default. For example, when using markdown-it-footnote, if the footnote text includes markdown formatting, then the text will be automatically formatted as expected.

Similarly, I would expect formatting rules to inherit form my markdown-it configuration. For example, if I instruct markdown-it to escape HTML, then HTML should also be escaped when markdown-it-table-of-contents renders headings. Currently, that is not the case which is surprising and wrong (at least based on the behavior of similar extensions).

By updating the default behavior to use the markdown-it internal renderer, we fix all these issues with a single, easy change. Furthermore, I've added the renderer itself as a second argument of the format function. This way, users can override the existing behavior while still running the text thru the official rendered if they need. (FWIW, I am using this feature in my own use case.)

3. Remove support for forceFullToc option

Finally, as I mentioned above, the tests for forceFullToc conflict with markdown rendering. While the forceFullToc code is a bit obscure, I think I was able to determine the root cause. Essentially, forceFullToc works by counting tokens to control a while loop. If you enable markdown formatting, new tokens are added to the markdown-it AST (which makes sense). As a result, the while loop never exits, causing the test suite to hang indefinitely.

My "solution" is to remove the forceFullToc option. In my opinion, this option is a little odd (i.e. should this extension support invalidly ordered headings?). Additionally, if we must choose between forceFullToc and markdown formatting, then I think markdown formatting is a much more important feature.

That said, I understand this suggestion / change may be controversial. And that's okay with me. I'm not saying I'm right – it's just my opinion. In any event, I am happily using a fork. Of course, I would like to merge these changes and drop my fork, but if you don't want to merge, I can live with that.

Thanks for all your work on this extension! We've been using it for years and has worked very well for us.

[martinlissmyr]

Great work @coryschires! Thanks!

Update code to use ES5 syntax

I buy your arguments! I agree.

Updates the existing format option to render markdown by default

👍

Remove support for forceFullToc option

I agree it's a weird one... I'm fine with dropping support for this option. I'm just going to up the version number to reflect it's a breaking change.

If you have nothing more to add I'm going to merge and release this soon!

[coryschires]

That's great news!

Thanks for accepting my changes!