Request: IterableNodes for Rc<VirtualNode> somehow

[bkase]

Great project!

Background

I'm attempting to use parts of Percy (namely virtual-dom-rs) in a user interface that is driven using FRP signals (similar, but a bit different from https://cycle.js.org/ ). Using this library and futures-signals, I can get something that seems like it could work well.

Essentially, I'm modeling components as functions of the form:

fn component(model: Model) -> impl Signal<Item = Rc<VirtualNode>>

Because of the way the FRP signals work, I only get a reference to a VirtualNode in certain places and so I need to wrap the result in an Rc.

Problem

Here is my issue: VirtualNodes own all their descendants and so there's no easy way for me to use the Rc<VirtualNode> as a descendant of some other VirtualNode tree.

Request

It would be great if VirtualNode supported an O(1) way to embed an Rc<VirtualNode> as a child (aka there's an instance From<Rc<VirtualNode>> for IterableNodes).

Potential Solution

For example: If the VirtualNode enum had a third variant such as

Indirect(Rc<VirtualNode>),

It would be very easy to implement an efficient From<Rc<VirtualNodes>>.

Conclusion

Please let me know if you need any more information, or if there is some way for me to get an O(1) injection with the library as is. I have a workaround that is slow, but will at least unblock my development for now (see below), but not having a cheap way to embed Rc<VirtualNode>s will be a blocker for me soon.

Thanks!

===

Hacky Workaround

For anyone who sees this and has my same problem before it's resolved, I've figured out a nasty hack that doesn't require forking Percy, but is O(n) per injection (it's basically an implementation of clone for VirtualNode):

pub struct InjectNode(pub Rc<VirtualNode>);

impl From<InjectNode> for IterableNodes {
    fn from(h: InjectNode) -> IterableNodes {
        fn clone(n: &VirtualNode) -> VirtualNode {
            match n {
                VirtualNode::Element(e) => VirtualNode::Element(VElement {
                    tag: e.tag.clone(),
                    attrs: e.attrs.clone(),
                    events: Events(e.events.0.clone()),
                    children: e.children.iter().map(|v| clone(v)).collect(),
                }),
                VirtualNode::Text(txt) => VirtualNode::Text(VText {
                    text: txt.to_string(),
                }),
            }
        }

        clone(&*h.0).into()
    }
}

[chinedufn]

Thanks so much for opening this issue!

A big goal of the tools in the Percy family is to be useful to people who want to just use one or two tools to go do something else.. so this is such a great issue...!

---

Background

Over time we want to make Percy's data structures more and more generic / trait based so that no matter if you're using Rc or some other container these things just work for you.

Instead of just imagining that outright .. we're waiting for real use cases to emerge so that we can design against them.. So thanks for this real use case!

Current data structures

So right now the children are stored as Vec<VirtualNode>.

https://github.com/chinedufn/percy/blob/792b4e412cec19d09bdca0c601a117cfe8dd86ad/crates/virtual-node/src/lib.rs#L74

Potential Solution

Off the top of my head I can't think of a reason that we couldn't instead have children be stored as Vec<AsRef<VirtualNode>>.

Then you'd be able to use anything that can be referenced as a VirtualNode .. be it Rc<VirtualNode> or whatever have you.

Side thoughts

We eventually want virtual_dom_rs to be a bit more trait based so that people can create nodes however they please (without even using VirtualNode) and as long as they implement a trait they'll work with the diffing/patching algorithm ... But we'd like to run into some real world use cases to design against before making that leap.

Conclusion

My thinking here is that we can

1. Make children AsRef<VirtualNode>

2. Make IterableNodes(Vec<AsRef<VirtualNode>>

https://github.com/chinedufn/percy/blob/792b4e412cec19d09bdca0c601a117cfe8dd86ad/crates/virtual-node/src/lib.rs#L377

---

If you're interested in taking a stab I'd be more than happy to type up some detailed notes for you on how to go about it, as well as answer any questions that you might have.

Should you not be interested or not have the time - I'd be more than happy to take a look at this next time I'm diving into that library!

---

Thanks so much for opening this issue! Let me know if you have a different implementation in mind than what I described. Open to other ideas!

[bkase]

@chinedufn Thanks for the wonderful response!

I'm newish to Rust, so I hadn't seen the AsRef before, that seems like a great solution!

I can't guarantee I'll have the time, but next week I can attempt to try this late at night after work one day. Would you mind writing up those notes? Is there more I should know besides just swapping in the AsRef and fixing compiler errors until it works?

Thanks!

[chinedufn]

Yeah it should boil down to just that - but if it doesn't just let me know!

And no worries if you don't end up having time - I have this on my radar!

Cheers - happy to answer any questions should you end up diving in!

[bkase]

@chinedufn I started taking a look at this, and I just wanted to make sure I'm on the right track before I spend too much time down the compiler yelling rabbit hole:

Vec<AsRef<VirtualNode>> is no good since AsRef is a trait and thus it's not sized. In order to appease the compiler I did: Vec<Box<AsRef<VirtualNode>>>. Is this the right thing to do? Or is there some more elegant way to handle this that avoids the box?

See here for some of that work: https://github.com/bkase/percy/commit/9641797a947b679c5578984fe81f9221052460a6

[chinedufn]

@bkase looks about right.

There's a performance "penalty" for the heap allocation - but I can't see this being a material issue in any reasonable situation when working in the DOM (knock on wood).

In the future we'll want performance profiling around our virtual-dom ... but for now this looks totally fine!

[chinedufn]

Also apologies for the delays in this thread .. I typically try to respond more quickly.. sorry and feel free to keep any questions coming! Looking smooth!

[bkase]

No worries, thanks for the tips!

[bkase]

@chinedufn I've run up against some borrow-checker issues:

First one

pub struct VElement {
    /// The HTML tag, such as "div"
    pub tag: String,
    /// HTML attributes such as id, class, style, etc
    pub attrs: HashMap<String, String>,
    /// Events that will get added to your real DOM element via `.addEventListener`
    pub events: Events,
    /// The children of this `VirtualNode`. So a <div> <em></em> </div> structure would
    /// have a parent div and one child, em.
    pub children: Vec<Box<AsRef<VirtualNode>>>,
}
impl PartialEq for VElement {
    fn eq(&self, other: &VElement) -> bool {
        self.tag == other.tag
            && self.attrs == other.attrs
            && self.events == other.events
            && self
                .children
                .into_iter()
                .zip(other.children.into_iter())
                .all(|(x, y)| x.as_ref().as_ref().eq(y.as_ref().as_ref()))
    }
}

Apparently I'm moving children and other.children, but I feel like you should be able to iterate here without ownership, so maybe I'm missing something

Second one

// needed for trait coherence reasons
struct AsRefVirtualNode(Box<AsRef<VirtualNode>>);

impl fmt::Debug for AsRefVirtualNode {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        self.0.as_ref().as_ref().fmt(f)
    }
}

impl fmt::Debug for VElement {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        let xs: Vec<AsRefVirtualNode> = self
            .children
            .into_iter()
            .map(|x| AsRefVirtualNode(x))
            .collect();
        write!(
            f,
            "Element(<{}>, attrs: {:?}, children: {:?})",
            self.tag, self.attrs, xs
        )
    }
}

I don't really like making that wrapper struct, but I couldn't think of another way to get the Vec<_> Debug instance without making the items Debug too. Anyway, the borrow checker doesn't like that I'm constructing the Vec here for some reason.

Any help would be appreciated, thanks!

Here's the branch I'm working on: https://github.com/bkase/percy/tree/as-ref-children

[chinedufn]

From a quick glance it looks like into_iter might be the problem since into_iter consumes self (and thus moves).

I would try using iter instead of into_iter in both of these places.

Let me know if that helps!

---

As for the wrapper struct - cool for now after things are working / PR'd I'd be happy to take a looksie at things like that and see what we can massage around!

---

Feel free to keep asking questions here! Looking good!

[bkase]

@chinedufn Thanks for the tip! I was able to finish the virtual-node crate switching to iter.

I've made more progress, the wrapper struct has come in handy; it's a nice way to get the PartialEq and Debug instances in other structs (like Patch).

I'm at the point where I need to change the html! macro to include children as a Vec<Box<AsRef<VirtualNode>>> instead of Vec<VirtualNode>. I looked through some of the html-macro code, and it's not clear to me what I have to change to fix this. Could you point me in the right direction?

Thanks!

https://github.com/bkase/percy/tree/as-ref-children

[chinedufn]

Woohoo! This is awesome and is amazing groundwork for making the virtual dom implementation compatible with any other types (not just the ones we define). Which is groundwork for a future where making a component in XYZ framework would still make it compatible with components made using Percy. So sweet stuff!

---

So, to run the html macro tests

https://github.com/chinedufn/percy/blob/e4d86f3b5f6596f38bbaef5ba942760be48b9ce4/crates/html-macro-test/src/lib.rs#L5

---

Here's where we insert children:

https://github.com/chinedufn/percy/blob/64626888ee4371ce8b52bacfb0770ad8b04ebbbd/crates/html-macro/src/parser/mod.rs#L127

So I might try children.into_iter().map(|child| Box::new(child))

Let me know if that helps!

[bkase]

Hey there, just wanted to let you know that I've become quite busy with other things and this PR ended up requiring a lot more changes than I was expecting, so I'd like to abandon this task.

Please feel free to work off of my branch if it is helpful, and thanks for all your help!

[chinedufn]

Awesome - when I get around to solving this problem I'll update this issue - thanks for all of your effort here - cheers!!