Slate: add `node.getAllAncestorsAtRange` method

[ianstormtaylor]

Do you want to request a _feature_ or report a _bug_?

Feature.

What's the current behavior?

Right now you have to manually iterate the blocks and look up each of their ancestors, and then make sure you convert it to a set to remove duplicates.

What's the expected behavior?

It would be nice to have a getAllAncestorsAtRange method that returned all of the ancestors in the range exactly once, so you can more easily find wrapping parents of certain types.

[zhujinxuan]

Hi, may I know how this is different with

this.getAncestors(path).filter?

?

[ianstormtaylor]

@zhujinxuan good question, actually the name I wrote in the issue is confusing. I think it would be better to have:

const ancestors = document.getAllAncestorsAtRange(range)

Which would return an de-duped list of ancestors of all of the text nodes in a range. That way you could then do your own .filter on them.

const quotes = document.getAllAncestorsAtRange(range).filter(a => a.type === 'quote')

[Dundercover]

I might be able to do this unless anyone is already on it. A couple of questions though:

1. Why name it getAllAncestorsAtRange and not just getAncestorsAtRange? since we already have getAncestors and also getBlocks/getBlocksAtRange, getTexts/getTextsAtRange etc.
2. Would you prefer we create another method getNodesAtRange and then just filter away the text nodes to get the result for getAllAncestorsAtRange?

Side question: For my project at work I've needed to get all top ancestors for a range e.g. getFurthestAncestorsAtRange, is that anything you feel would be good to add to the node API?

[ianstormtaylor]

@Dundercover very good questions, I don't have solid answers. It probably means we should re-evaluate all of those types of methods at once to stay consistent. Some of them are poorly named now too.

Right now we have getBlocksAtRange which returns the leaf-most blocks, and causes confusion for people since this is not obvious. I think that's why I tended towards All*, but it's probably better for us to qualify the other confusing methods names instead. So for blocks we might end up with:

getBlocksAtRange() // returns *all* of the blocks
getLeafBlocksAtRange() // returns the leaf-most blocks in the range
getRootBlocksAtRange() // returns the root-most blocks in the range

I was thinking of Leaf and Root as qualifiers, but Closest and Furthest could work better, they're just a bit longer and more likely to misspell. Then again, Leaf is already a concept we have, so maybe we stick with Closest/Furthest. All of these would apply to inlines too.

getNodesAtRange()

getTextsAtRange()

getBlocksAtRange() // changed to return *all*
getClosestBlocksAtRange()
getFurthestBlocksAtRange()

getInlinesAtRange() // changed to return *all*
getClosestInlinesAtRange()
getFurthestInlinesAtRange()

I agree that it would probably be better as getNodesAtRange too instead of getAncestorsAtRange, seeing as the only difference is text nodes and they are easy to filter out. (Unless there's an optimization we can make when looking them up, in which case we can offer both.)

[Dundercover]

Thank you for clarifying @ianstormtaylor!

I think getClosestXAtRange and getFurthestXAtRange would be confusing as I would be thinking closest to/furthest from the node I'm calling from and not the leaf nodes of the range I'm specifying as argument (and that would be the opposite from the existing getFurthestBlock, getClosestBlock, etc). Get closest/furthest nodes of a range feels weird as I somehow view all nodes in a range "equal" if you understand what I mean. I think I prefer root/leaf even if I don't think they are ideal for the same reason you described.

For the getAllXAtRange naming convention it might be better to keep the All and instead put a deprecation warning on the existing methods, letting people know that they are being renamed.

(sry for dragging you down into these discussions, my participation won't change based on how much I like the naming convention we decide, go ahead and stop me whenever you want)

[ianstormtaylor]

@Dundercover that makes sense! That's very helpful, I agree with you about that confusion. I'm down for Root/Leaf even if it's not perfect. Really its consistent because its using the "leaf" descriptor for the same purpose. Also we already have isLeafBlock() too.

For the "all" do you like it because it feels more precise?

[Dundercover]

Sounds good @ianstormtaylor, lets go with Root/Leaf then.

For the All, I think it works either way (I think I'm leaning more towards without the All tbh) but just so we could deprecate the existing methods (getBlocksAtRange, etc). If we prefer without the All, we could remove it later with another deprecation period.

I could also skip any changes to the current API methods and just add the getNodesAtRange for now?

[ianstormtaylor]

@Dundercover let's do it in two steps I think. The first one adds:

getNodesAtRange()
getClosestBlocksAtRange()
getFurthestBlocksAtRange()
getClosestInlinesAtRange()
getFurthestInlineAtRange()

And in doing so, it deprecates the prefix-less getBlocksAtRange and getInlinesAtRange. And then we can punt the "all or not" decision to after that deprecation. Because in the meantime it's fairly easy to use getNodesAtRange and filter for the same effect?

Side note, since we're chatting 😄 I feel like in the future people are going to find use in also having:

getContainedBlocksAtRange()
getContainedInlinesAtRange()

For cases where they need to only get the blocks/inlines that are fully contained, instead of ones where the selection falls partially in them. But I'm not sure on that yet, just a hunch from the commands I've been writing.

[Dundercover]

Sounds good @ianstormtaylor, I'll jump on this next.

The getContainedXAtRange methods do definitely feel like they could be useful for a lot of use cases. I do think though that we should create a separate issue for those to not spread this issue too wide.