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

Add unist-util-ancestor to list of utilities #53

Merged
merged 2 commits into from
Sep 23, 2021
Merged

Add unist-util-ancestor to list of utilities #53

merged 2 commits into from
Sep 23, 2021

Conversation

gorango
Copy link
Contributor

@gorango gorango commented Sep 22, 2021

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Adding a small utility to readme that 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.

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Sep 22, 2021
readme.md Outdated Show resolved Hide resolved
@wooorm
Copy link
Member

wooorm commented Sep 23, 2021

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
  }
}

@wooorm wooorm added the 💪 phase/solved Post is done label Sep 23, 2021
@wooorm wooorm merged commit 3d0d9bb into syntax-tree:main Sep 23, 2021
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Sep 23, 2021
@github-actions

This comment has been minimized.

@wooorm wooorm added 📚 area/docs This affects documentation 🦋 type/enhancement This is great to have labels Sep 23, 2021
gorango added a commit to gorango/unist-util-ancestor that referenced this pull request Sep 23, 2021
- Remove `unist-util-find`
- Based on: syntax-tree/unist#53 (comment)
@gorango
Copy link
Contributor Author

gorango commented Sep 23, 2021

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 :)

@wooorm
Copy link
Member

wooorm commented Sep 23, 2021

That looks great! Nice find as well on the things that I didn't get to. Glad it worked!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 area/docs This affects documentation 💪 phase/solved Post is done 🦋 type/enhancement This is great to have
Development

Successfully merging this pull request may close these issues.

2 participants