add: option to allow headings under certain node type

[laysent]

Hi, I would like to propose this extra option to allow more flexibility of toc generation behavior.

My use case is, when I use remark plugin to wrap each section of article into a section node, headings are no longer on top level, but I would like to still have these headings inside toc.

This change won't break any existing usage.

[wooorm]

Nice work! 💪 I added some thoughts inline, but there’s also a bigger, more structural one:

mdast-util-toc/lib/search.js

Line 46 in 37560ad

if (parent !== root && parentTypeWhiteList.indexOf(parent.type) < 0) {

Maybe we can do this simpler. Default parents to 'root', that way we can drop the parent !== root part of the condition, which makes usage and code cleaner.

[laysent]

Hi, @wooorm, thanks for the feedback, I have made some changes accordingly. Please check details above.
For default value of parents, I think it might be better to use [] and still keeps parent !== root. In this way, users won't accidentally passed in options to overwrite default conditions and cause top-level headings not included. To me, top-level headings should always be included, with or without parents configured.

[wooorm]

@laysent Thanks for the changes, this looks really good! I only have a few tiny suggestions.

To me, top-level headings should always be included, with or without parents configured.

I’m not too sure, I think it’s probably often the case, but maybe someone would only want the ones in section or whatever 🤷‍♂️ It’s a tiny use case, but maybe it’ll show up somewhere the coming years.
And the code for it means less code, so that’s also 👍
But I don’t feel very strongly about it, so let’s see what @BarryThePenguin thinks!

[BarryThePenguin]

I agree with @wooorm. I'm more inclined to keep the api open to give people the option. Does that mean settings.parents would default to 'root'? And if people wanted they could change it to ['root', 'blockquote'] etc..?

As long as that's mentioned in the documentation I think that's fine 👍

[laysent]

Hi, I have made the changes according to the feedback.
I think one thing worth mentioning here: Previously, code checks root !== parent, now it checks !is("root", parent) by default. There will be different result when users passing in a tree where top node is not "root". Probably it's not a case that need to be worried here. Not very sure about that.
If we need to make things work exactly like before, we could set default value of parents to be root element (the first parameter passed in by user) instead of "root".

[wooorm]

@laysent You’re right! It would be a breaking change in some cases. I don’t think people ever pass in a non-root as root though, but it could indeed break. So how about settings.parents || root?
To have exactly the same behaviour, with added blockquotes, someone could pass in [root, 'blockquote'] themselves, where root is a reference to the tree passed in. But I’m fine having the docs be simple and mention 'root'

[laysent]

@wooorm Sure. Also, still wondering if we should keep typeof settings.parents === 'undefined' ? root : 'root' or simply settings.parents || root. There could be difference when user sets "parents": null. If we keep null as value for parents instead of replacing with "root", it should allow all headings to be included.

[wooorm]

I like having null and undefined meaning the same thing, so I think settings.parents || root makes sense. If one were to allow any parent, () => true works and is explicit enough in my opinion!

[laysent]

Agreed! I have made the changes. :-)

[wooorm]

@laysent Wonderful!

@BarryThePenguin What do you think?

[BarryThePenguin]

I've got a wip to add tests specifically for the scenarios we discussed above. LGTM 👍 and I'll add another PR