Should the type Html be cheap to clone?

[cecton]

I've been working on porting Yewprint to Yew 0.20 lately and I noticed something: maybe the type Html should be cheap to clone.

API perspective

When making components you need to pass props that are cheap to clone (ideally). This is part of the reason why I introduced implicit-clone to Yew. But some things in Yew are still not cheap to clone and maybe they should:

• Classes

  I already made a PR and this is a digression of the subject. See Make Classes cheap to clone #3021 for details.

• Html

  In component based frameworks you sometimes pass html nodes to your component. This is usually done through "children" like this:

  <MyComponent>
      <InnerComponentPassedAsChild>
  </MyComponent>
  

  But frameworks do sometimes need to differentiate components that are passed as argument. See here an example of documentation from Blueprint where we differentiate "left element" and "right element":

  

  The equivalent in Yew would be to use the type Html in a property. This is what we have done in Yewprint:

  #[derive(Clone, PartialEq, Properties)]
  pub struct InputGroupProps {
      #[prop_or_default]
      pub disabled: bool,
      #[prop_or_default]
      pub fill: bool,
      #[prop_or_default]
      pub large: bool,
      #[prop_or_default]
      pub small: bool,
      #[prop_or_default]
      pub round: bool,
      #[prop_or_default]
      pub placeholder: String,
      #[prop_or_default]
      pub left_icon: Option<IconName>,
      #[prop_or_default]
      pub left_element: Option<yew::virtual_dom::VNode>,              // <----- here
      #[prop_or_default]
      pub right_element: Option<yew::virtual_dom::VNode>,             // <----- and here
      #[prop_or_default]
      pub input_type: TextInputType,
      #[prop_or_default]
      pub oninput: Callback<InputEvent>,
      #[prop_or_default]
      pub onkeyup: Callback<KeyboardEvent>,
      #[prop_or_default]
      pub onkeydown: Callback<KeyboardEvent>,
      #[prop_or_default]
      pub value: String,
      #[prop_or_default]
      pub class: Classes,
      #[prop_or_default]
      pub input_ref: NodeRef,
  }
  

(Html is currently a type alias for VNode)

Yew perspective

Unfortunately I'm not very knowledgeable about the type VNode (Html is a type alias for VNode) but I assume the type was not made cheap to clone (put into a Rc) because there was no need internally for this.

If we want to make Html cheap to clone, we need to figure out where to put the Rcs. I see multiple options here:

1. Inside every variant of VNode:

   /// Bind virtual element to a DOM reference.
   #[derive(Clone)]
   pub enum VNode {
       /// A bind between `VTag` and `Element`.
       VTag(Rc<Box<VTag>>),
       /// A bind between `VText` and `TextNode`.
       VText(Rc<VText>),
       /// A bind between `VComp` and `Element`.
       VComp(Rc<VComp>),
       /// A holder for a list of other nodes.
       VList(Rc<VList>),
       /// A portal to another part of the document
       VPortal(Rc<VPortal>),
       /// A holder for any `Node` (necessary for replacing node).
       VRef(Rc<Node>),
       /// A suspendible document fragment.
       VSuspense(Rc<VSuspense>),
       /// A raw HTML string, represented by [`AttrValue`](crate::AttrValue).
       ///
       /// Also see: [`VNode::from_html_unchecked`]
       VRaw(Rc<VRaw>),
   }
   

   (This is just a stub code to illustrate)

2. Transform type alias Html into an struct and make it cheap to clone:

   #[derive(Debug, Clone, Default, PartialEq)]
   pub struct Html {
       pub(crate) node: Rc<VNode>,
   }
   

   The idea here is that we put the Rc on top most type. It will be cheap to clone for the users of Yew. But internally it won't impact positively Yew.

3. Use Rc at the lowest level:

   /// A type for a virtual
   /// [`TextNode`](https://developer.mozilla.org/en-US/docs/Web/API/Document/createTextNode)
   /// representation.
   #[derive(Clone)]
   pub struct VText {
       /// Contains a text of the node.
       pub text: AttrValue,
   }
   

   VText is already cheap to clone. I did not modify this code. Lets look at another:

   /// This struct represents a fragment of the Virtual DOM tree.
   #[derive(Clone, Debug)]
   pub struct VList {
       /// The list of child [VNode]s
       pub(crate) children: Rc<Vec<VNode>>,    // we should probably use Rc::make_mut for mutations
   
       /// All [VNode]s in the VList have keys
       fully_keyed: FullyKeyedState,           // Rc too?
   
       pub key: Option<Key>,                   // probably already cheap to clone
   }
   

4. At some other level?

I personally think option 1 is bad while option 3 is probably the best. Option 2 can be an easier viable option.

Conclusion

It does seem to make sense from an API perspective. But internally, VNode is probably one of the complicated type of Yew. This means that any change in it could have big impact on: binary size and performance under some conditions. On top of that, because VNode is at the cross of all roads in Yew's framework, it will require the expertise of a lot of different people and a thorough review.

Since I have interest in this feature I'm totally okay doing a PR at some point for this. Though I don't think I'm the best suited person as I haven't work much on VNode itself nor lifecycle nor other things that are being touched here. What bothers me the most is that it will be a big impactful PR that will require a lot of effort to review and evaluate the risks. If some of the maintainers can tell me now if they are okay with reviewing this or not it would give me confidence that this will not turn into a rotting PR in the back of the queue. Of course if I'm the only one with an interest in this it might be useless.

It's also worth noting that even though I explained why it makes sense, it is possible there are counter arguments here that proves this change is unwanted.

[voidpumpkin]

I would be up for options 2 and 3!

[cecton]

I tried option 2 some time ago and I really struggled. I was thinking I would give another shot one day .... 😅 but you know how things go haha. If you do want to give it a try feel free. Otherwise I will definitely try again, probably this year.

[futursolo]

I actually partially implemented Option 3 because I need to modify the behaviour of Children for render props.
I am fine with option 2 too.

I will request both of you to review it.

[cecton]

In the end, if all the underlying types are cheap to clone this will definitely solve the issue because Html itself will become cheap to clone. Since you needed this for VList and you already made efforts into that direction, I think we should move forward with that.