-
-
Couldn't load subscription status.
- Fork 22
Add unist-util-ancestor to list of utilities
#53
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
Conversation
Nice!
For performance reasons you might want to not use unist-util-find (in this way). And perhaps also getParents like this.
Right now, for every node, the whole tree is walked many many times.
If possible, it would be muuuch faster to walk the tree once/twice.
Here’s some pseudocode:
const stacks = new Map() visitParents(tree, (node, parents) => { if (nodesToFind.includes(node)) { stacks.set(node, parents) } }) nodesToFind.forEach(node => { if (!stacks.has(node)) { throw new Error('unist-util-ancestor requires all nodes be contained in the tree') } }) // Somethign like this? let index = -1 let ancestor = tree // This is broken, maybe you get the gist? while (++index) { const nextAncestor = stacks.get(nodesToFind[0])[index] const shared = nodesToFind.every(node => stacks.get(node)[index] === nextAncestor) // To do: exit if there are no nodes left on stacks. if (shared) { ancestor = nextAncestor } else { break } }
This comment has been minimized.
This comment has been minimized.
- Remove `unist-util-find` - Based on: syntax-tree/unist#53 (comment)
Thank you so much for taking the time to review and provide this great feedback!
I have implemented your suggestions and removed the find util completely. Way cleaner :)
That looks great! Nice find as well on the things that I didn't get to. Glad it worked!
Initial checklist
Description of changes
Adding a small utility to
readmethat helps with finding common ancestors of one or more unist nodes. It has come in handy a few times while working with ast trees so I thought I'd package it up. Here's hoping it might be helpful to someone else too!P.S. I am open to renaming the package if desired.