Skip to content

Add visitor function #14

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

Closed
4 tasks done
danistefanovic opened this issue Jan 24, 2022 · 6 comments · Fixed by #15
Closed
4 tasks done

Add visitor function #14

danistefanovic opened this issue Jan 24, 2022 · 6 comments · Fixed by #15
Labels
💪 phase/solved Post is done

Comments

@danistefanovic
Copy link
Contributor

Initial checklist

Problem

Really like the unified ecosystem! I'm currently working on a MutationObserver-based project where those DOM mutations need to be serialized and deserialized. To reconstruct a DOM mutation (e.g., changing the value of a text node), I need to keep track of all DOM nodes that have been deserialized by hast-util-to-dom and connect those to the appropriate hast node and vice-versa. Overview for context:

Record (not relevant for this feature request)

  1. Take a snapshot of the whole DOM and serialize it to a virtual DOM (hast)
  2. MutationObserver detects a text change: listener contains the target DOM node, and with the help of a custom node store, everything gets tracked and serialized

Replay

  1. Deserialize the entire DOM snapshot with hast-util-to-dom
  2. Identify the target DOM node and apply the text change: Unfortunately, this step is not possible with the current hast-util-to-dom functionality because there's no reliable way to map a generated DOM node to a hast node.

Solution

By adding a visitor function, it's possible to inject custom logic to the tree traversal to accomplish the DOM<>VDOM mapping needed in the above example.

options.onNode: (hastNode: HastNode, domNode: Node) => void

Usage example:

function deserialize(
  // A custom store to track all DOM<>VDOM associations
  store: NodeStore,
  // The virtual DOM to deserialize
  vdom: HastNode,
): Node {
  return toDom(vdom, {
    // The new visitor function that receives both nodes as arguments
    onNode: (hastNode: HastNode, domNode: Node) => {
      // Custom code here...
      store.addNode(domNode, hastNode);
    },
  });
}

Although not required for my use case, such a visitor function could also be used in the future similarly to unist-util-visit-parents#visitor to transform nodes while the tree is being traversed instead of looping multiple times through the entire tree.

Alternatives

There's currently no viable alternative. What I've considered for my specific use case:

  • Add a data-node-id attribute to the DOM node, which gets included in HastElement.properties. However, this is only possible for (Hast)Element nodes. Text and many other DOM nodes don't have attributes.
  • A selector-based approach to identify the nodes (e.g., with XPath) is not an option because DOM nodes could have been added or removed, breaking the selectors.

Happy to provide a PR with the addition!

@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 Jan 24, 2022
@wooorm
Copy link
Member

wooorm commented Jan 24, 2022

Hey!

I understand the use case, and agree that the alternatives are insufficient, though I am not completely sold on the proposed solution yet. Some random thoughts:

  • This seems like more generic “events” than visitors/tree walking to me
  • Your solution runs after the visit/transform/handle (i like the term handle I believe), should that after be used in the prop/method name? handle, afterNode, onTransformNode? Node is maybe vague because everything’s a node 🤔
  • Does a before counterpart make sense (probably not?)
  • Can the handler return a different DOM node?

@danistefanovic
Copy link
Contributor Author

This seems like more generic "events" than visitors/tree walking to me

For my use case, definitely yes. However, it depends on whether such a callback should allow tree mutations like unist-util-visit or not.

Does a before counterpart make sense (probably not?)

A "before" callback can make sense to eliminate multiple tree traversals if someone needs to modify the hast before it gets transformed into a DOM node.

import {toDom} from 'hast-util-to-dom';
import {visit} from 'unist-util-visit'

const tree = /* hast with 5000 nodes */

const visitor = (node) => {
  if (node.type === 'element' && node.tagName === 'a') {
    node.properties.href = '#';
  }
}

visit(tree, visitor);
const dom1 = toDom(tree);
// Total of 10'000 interations

const dom2 = toDom(tree, { before: visitor });
// Total of 5000 iterations

Can the handler return a different DOM node?

Good question. If a "before" is introduced, all node mutations could be performed there, and modifying DOM nodes directly should theoretically be unnecessary.

I would go with the minimum: a callback after the hast->DOM transformation without supporting any additional node mutations, but choosing a more generic naming that could be used for all the above cases in the future if needed. Some additional naming ideas:

  • before / after
  • beforeTransfrom / afterTransform
  • visitor / onTransform

@wooorm wooorm added the 👍 phase/yes Post is accepted and can be worked on label Jan 25, 2022
@wooorm
Copy link
Member

wooorm commented Jan 25, 2022

I would go with the minimum […] but choosing a more generic naming that could be used for all the above cases in the future if needed

Agreed 👍 I like afterTransform the best. 👍
You were interested in contributing the code, right?

@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Jan 25, 2022
@github-actions

This comment has been minimized.

@danistefanovic
Copy link
Contributor Author

Will submit a PR within the next day or two!

wooorm pushed a commit that referenced this issue Jan 26, 2022
Reviewed-by: Titus Wormer <[email protected]>

Closes GH-14.
Closes GH-15.
@github-actions

This comment has been minimized.

@wooorm wooorm added the 💪 phase/solved Post is done label Jan 26, 2022
@github-actions github-actions bot removed the 👍 phase/yes Post is accepted and can be worked on label Jan 26, 2022
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 a pull request may close this issue.

2 participants