Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: traverse backwards with $dfsIterator #7094

Closed
wants to merge 0 commits into from

Conversation

nigelgutzmann
Copy link
Contributor

Description

The $dfsIterator allows you to pass in a startNode and an endNode, and then will progress from the start to the end.

However, if you pass in an endNode that exists in the tree BEFORE the startNode, the iterator will progress forward from the startNode until it traverses the entire remainder of the tree and never hit the endNode.

This change allows the iterator to detect this condition and traverse the tree backwards.

The use-case for this is to traverse backwards from a starting point, towards the beginning of the editor, until you meet some condition. For example:

let previousTextNode = null;
for (const node of $dfsIterator(startNode, $getRootNode())) {
    if ($isTextNode(node)) {
        previousTextNode = node;
        break
    }
}

The alternative is to call $dfs().reverse(), but that is slow for editors with many nodes (lots of overhead).

Copy link

vercel bot commented Jan 25, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 25, 2025 0:22am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 25, 2025 0:22am

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 25, 2025
Copy link

github-actions bot commented Jan 25, 2025

size-limit report 📦

Path Size
lexical - cjs 29.07 KB (0%)
lexical - esm 28.86 KB (0%)
@lexical/rich-text - cjs 38.1 KB (+0.15% 🔺)
@lexical/rich-text - esm 30.96 KB (+0.16% 🔺)
@lexical/plain-text - cjs 36.61 KB (+0.17% 🔺)
@lexical/plain-text - esm 28.22 KB (0%)
@lexical/react - cjs 39.89 KB (+0.11% 🔺)
@lexical/react - esm 32.33 KB (+0.18% 🔺)

@etrepum
Copy link
Collaborator

etrepum commented Jan 25, 2025

I’m not sure it makes sense for the direction to be determined by the two nodes alone here, could be a bit confusing. I have been working on a much more robust traversal API that’s not quite ready that does offer both directions in #7046

@nigelgutzmann
Copy link
Contributor Author

@etrepum thanks for taking a look at my PR, I appreciate it!

I took a look at #7046, but I don't fully understand it. It is a new concept, so it might take me some time to get the idea fully.

Naively, I don't understand how that PR will change the $dfsIterator API. You mentioned that it could be confusing for the direction to be determined by the two nodes alone, but the API of $dfsIterator is the same on your branch - it still just accepts the two nodes.

Or is there going to be some other API entirely for a backwards depth-first search, using the NodeCaret from your PR?

@nigelgutzmann
Copy link
Contributor Author

Perhaps I should have marked this PR as a bug fix. It seems wrong to me that you can supply an endNode, but the iterator will travel further and further from the endNode, never hit it, and then stop at the end of the tree.

@etrepum
Copy link
Collaborator

etrepum commented Jan 25, 2025

That’s right, the PR doesn’t change the existing API at all, it provides new more powerful abstractions. $dfsIterator is always strictly depth first left to right, there aren’t any existing full traversal APIs that go right to left ($getNextRightPreorderNode is as close as it gets IIRC).

@nigelgutzmann
Copy link
Contributor Author

nigelgutzmann commented Jan 28, 2025

I am happy that there is going to be a more structured/consistent way to traverse the tree. I need one traversal algorithm that works the same way forwards and backwards. Using two different algorithms (dfs forward and pre-order backwards) is very complicated from an application developer perspective. Managing how nodes are returned in different orders due to different traversal algorithms depending on if you are going forward or backward is unwieldy.

It sounds like that is going to be accomplished by the NodeCaret API, so I am looking forward to when that arrives.

In the meantime, I will just copy this diff into my project and use the dfs traversal forwards and backwards.

If you don't wish to merge this in, you may close the PR.

@etrepum
Copy link
Collaborator

etrepum commented Jan 28, 2025

I think if this sort of change were to be added as-is, it should probably be a separate function for a $dfsReverseIterator or something like that. Having the existing function reverse direction based on certain conditions with very few tests is risky, and it adds some extra overhead for when the nodes are already in the correct left to right order. In your application code or a wrapper function you could choose which iterator to create based on the relative position of the nodes.

@nigelgutzmann
Copy link
Contributor Author

@etrepum I would happily submit a PR for $dfsReverseIterator, if you would like?

@etrepum
Copy link
Collaborator

etrepum commented Jan 28, 2025

That’s up to you, it would be an incremental improvement outside of the core that would be low risk to add so if it looked right and had tests it would likely be approved.

Personally I think the design of the dfs API is not great, but it is what people are currently used to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants