syntax-tree / hast-util-to-dom

utility to transform hast to a DOM tree
https://unifiedjs.com
ISC License
19 stars 8 forks source link

Make hast-to-dom composable with other DOM tools - compose DOM with a… #8

Closed gregid closed 4 years ago

gregid commented 4 years ago

…lready generated dom fragments

This is minor change (1 line) but allows

I needed this change for my code to work and if it doesn't conflicts with any parts of hast ecosystem please feel free to merge.

wooorm commented 4 years ago

I don’t think it’s a good idea to allow the DOM inside hast.

to compose hast with other DOM tools (apart from hastscript)

Wouldn’t it break all hast utilities and rehype plugins?

to build custom DOM elements on top of hast that can then be mixed into another hast element

Can you expand on why you want to do this?

gregid commented 4 years ago

For sake of brevity let's call here hast with DOM Nodes as children allowed: hast+

toDom accepting DOM Nodes as children won't break any plugins since its output is a DOM Node. Using hast+ with other plugins may break them since they wouldn't know how to process them.

My main use case is as follows - my hast+ tree is automatically generated from spec + data and my children is a function that may return hast or node if one exists already. So here hast+ serves both as transform and append specification. Alternative was having separate transform spec with placeholders and separate append spec that traverse the DOM and appends where desired.

As you noticed other plugins won't work well with hast+ so I have my own element generators (similar to hastscript) like T.div() and T.wrapperDiv() with some predefined attributes. These generators create DOM Nodes as opposed to hast Nodes (as in hastscript)

Another case is I have 2 containers that a view can be dragged into, once dragged its wrapper changes but the view itself shouldn't be rerendered.

Another use case is progressively migrating from other rendering techniques, templates, web components, etc. With this change you can mix in existing nodes.

To sum up adding this change won't break functionality but hast+ shouldn't be used with other plugins.  I propose that this stays as undocumented feature eventually an option could be added { allowDOMNodes: false } (false as default) that would have to be intentionally switched on. 

if (options.allowDOMNodes && node.nodeType) return node;

I understand you must take into consideration entire ecosystem so If you decide against merging it, I will just continue to use my fork.

wooorm commented 4 years ago

Thanks for the detailed write up! Your observations are correct, those are the reasons why I’d rather not support this.

I think a proper solution would be to indeed create “virtual” hast nodes, and at the last stage (here) choose whether to pick an existing node, or create a new tree, such as through “handler” (like here: https://github.com/syntax-tree/mdast-util-to-hast/tree/48d420d10cd142a6266d4dee3eb76d804b5fe623#optionshandlers), which then would look something like this:

var original = require('.../place-to-default-element-handler')

function newOrExistingElement(node) {
  var id = node.properties.id
  var existing = id ? document.getElementById(id) : null
  return existing || original.apply(this, arguments)
}

Using your own fork seems fine too, not sure if the above idea works for you or for others

wooorm commented 4 years ago

I am under the impression I can close this issue. Please comment below if that is incorrect.