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 generic to parent parameter in visitor callback #30

Closed
wants to merge 1 commit into from
Closed

Add generic to parent parameter in visitor callback #30

wants to merge 1 commit into from

Conversation

crossjs
Copy link

@crossjs crossjs commented Sep 12, 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

Parent should be superset of Child

@github-actions github-actions bot added the 👋 phase/new Post is being triaged automatically label Sep 12, 2021
@github-actions

This comment has been minimized.

@@ -20,7 +20,7 @@
* @callback Visitor
* @param {V} node Found node
* @param {number|null} index Position of `node` in `parent`
* @param {Parent|null} parent Parent of `node`
* @param {import('unist').Parent<V>|null} parent Parent of `node`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't the case though.

For example if you have an AST like https://astexplorer.net/#/gist/07590330c6b74c87dc6ca4c170f71bf0/c6107a73d37212d1ea8b1b6cd18cd1cef230527d

And have a visitor going to emphasis nodes

visit(ast, 'emphasis', (node, position, parent) => {/* do something */})

It would be incorrect to say Parent<Emphasis>, the Parent also contains Text, Strong, and InlineCode

@github-actions github-actions bot added 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Sep 12, 2021
@wooorm
Copy link
Member

wooorm commented Sep 12, 2021

First of all I think this library should stay in sync with its parent (ha!), unist-util-visit-parents, so this change should also be there.

Furthermore, this should be smarter! As we’re already inferring which children exist in tree, and filter against that with test, could we also figure out which nodes are potential ancestors of those children?
https://github.com/syntax-tree/unist-util-visit-parents/blob/be4b7a920fe10e9c23157a2a4aeb9693d3d0a11d/complex-types.d.ts#L6
I can imagine this being quite hard to figure out though, but it would be very useful!

@remcohaszing
Copy link
Member

Furthermore, this should be smarter! As we’re already inferring which children exist in tree, and filter against that with test, could we also figure out which nodes are potential ancestors of those children?

I think this is fairly doable, but it requires some type-fu.

I think the check should be the other way around:

The parent type can be similar to InclusiveDescendant, but instead of checking type, it checks children[number],type. Then the child of type can be based on the parent type.

First of all I think this library should stay in sync with its parent (ha!), unist-util-visit-parents, so this change should also be there.

Punny! 😄

Jokes aside, I doubt if this is possible for unist-util-visit-parents, whereas I’m pretty sure it is possible for unist-util-visit. This is because for unist-util-visit just the parent would need to be determined, but for unist-util-visit-parents the entire ancestry. I fear that if this would be possible, it would really impact performance. I would love to be proven wrong though.

As for the current implementation I agree with @ChristianMurphy.

wooorm added a commit to syntax-tree/unist-util-visit-parents that referenced this pull request Sep 18, 2021
Previously, a basic `Parent` from `@types/unist` was used, as an array,
for the second parameter of a visitor (`parents`).
This changes that to instead use an array of descendants in `tree`
which implement the abstract `Parent` interface.
This is not perfect, because several parents can’t be found in certain
nodes practically, but it will at least help folks narrow.

Related to syntax-tree/unist-util-visit#30.
wooorm added a commit to syntax-tree/unist-util-visit-parents that referenced this pull request Sep 21, 2021
Previously, a basic `Parent` from `@types/unist` was used, as an array, for the second parameter of a visitor (`parents`). This changes that to instead use an array of descendants in `tree` which implement the abstract `Parent` interface. This is not perfect, because several parents can’t be found in certain nodes practically, but it will at least help folks narrow.

Related to syntax-tree/unist-util-visit#30.
Closes GH-11.

Reviewed-by: Christian Murphy <christian.murphy.42@gmail.com>
wooorm added a commit that referenced this pull request Sep 21, 2021
Previously, a basic `Parent` from `@types/unist` was used for the third
parameter of a visitor (`parent`).
This changes that to instead use an array of descendants in `tree`
which implement the abstract `Parent` interface and can have `node`
as a child.

Closes GH-30.
Related-to:  syntax-tree/unist-util-visit-parents#11.
@wooorm wooorm mentioned this pull request Sep 21, 2021
5 tasks
@wooorm wooorm closed this in #31 Sep 23, 2021
wooorm added a commit that referenced this pull request Sep 23, 2021
Previously, a basic `Parent` from `@types/unist` was used for the third parameter of a visitor (`parent`). This changes that to instead use an array of descendants in `tree` which implement the abstract `Parent` interface and can have `node` as a child.

Closes GH-30.
Closes GH-31.
Related-to: syntax-tree/unist-util-visit-parents#11.
@wooorm wooorm added the 💪 phase/solved Post is done label Sep 23, 2021
@github-actions

This comment has been minimized.

@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants