Remove generics from virtual dom and fix nested components #756

jstarry · 2019-11-28T01:45:35Z

⚡️ This change greatly improves the ergonomics of nested components ⚡️

The virtual dom is now cloneable

Nested children will render from a fresh clone rather than from a closure which was very unergonomic in practice because it forced moves into the closure. For example, you couldn't have this because self cannot be moved into the closure:

html! {
  <Component>
    { self.render_sub_view() }
  </Component>
}

Properties need to be cloneable again

An unfortunate side effect of making the virtual dom cloneable is that props also need to be cloneable again. I don't forsee this causing any major issues but we can revisit if there's any pushback.

`Html<Self>` -> `Html` No more generics!

Virtual dom nodes are no longer genericised and arbitrary HTML fragments are easier to create and the typing of children nodes is much simpler (thanks to @kellytk for the suggestion).

No more magic closures into callbacks

This comes as a side effect of removing generics. Now, you have to explicitly create a callback using a ComponentLink<Self> instead of relying on Yew to magically create one.

Old approach:

fn view(&self) -> Html<Self> {
  html! {
    <button onclick=|_| Msg::DoIt>{ "Click me!" }</button>
  }
}

New approach:

fn view(&self) -> Html {
  let onclick = self.link.callback(|_| Msg::DoIt);
  html! {
    <button onclick=onclick>{ "Click me!" }</button>
  }
}

Hoping this isn't too painful!!

Fixes: #743
Fixes: #738

jstarry · 2019-11-28T01:47:16Z

@hgzimmerman could you please try out this branch for yew-router?

mdtusz · 2019-11-28T01:52:17Z

I'll test this out tomorrow but this seems like a totally ok compromise to me!

hgzimmerman

Great work! I didn't think that it would be possible to move the framework in this direction, and am impressed that you have managed to accomplish that.

hgzimmerman · 2019-11-29T14:25:38Z

examples/nested_list/src/app.rs

+    fn view(&self) -> Html {
+        let on_hover = &self.link.send_back(Msg::Hover);
+        let onmouseenter = &self.link.send_back(|_| Msg::Hover(Hovered::None));
+        html! {


Casual observation:
This demonstrates P -> GP and C -> GP, but not C -> P. It would be nice to incorporate that into the example somehow if that is possible.

I augmented the example to show how this is possible with "weak" links

hgzimmerman · 2019-11-29T14:35:04Z

src/html/mod.rs

 pub struct ChildrenRenderer<T> {
-    len: usize,
-    boxed_render: Box<dyn Fn() -> Vec<T>>,
+    children: Vec<T>,


~~Caveat: I don't have a full understanding of the requirement of Clone at the moment. But maybe wrapping this in Rc might at the very least improve performance?~~

hgzimmerman · 2019-11-29T14:42:27Z

src/virtual_dom/vcomp.rs

-impl<C: Component> PartialEq for VComp<C> {
-    fn eq(&self, other: &VComp<C>) -> bool {
+/// Transform properties to the expected type.
+pub trait Transformer<FROM, TO> {


A transformer impl

src/virtual_dom/vtag.rs

hgzimmerman · 2019-11-29T14:53:58Z

examples/fragments/src/lib.rs

@@ -3,6 +3,7 @@
 use yew::{html, Component, ComponentLink, Html, ShouldRender};

 pub struct Model {
+    link: ComponentLink<Self>,


The only thing I don't like about this is the fact that the ComponentLink needs to be saved as part of the state for practically every Component now.

I would much rather access a link via a method on Component that is able to reach inside the component's state machine and get a link from that. I haven't looked at that structure to see if it is possible though.

An alternative that is definitely possible, is to provide a link on every existing lifecycle hook: create (already present), mounted, update, view, etc...

Although I don't know if that is really any better than just making users add a link to their component state.

Yeah the boilerplate isn't great, I think we can follow up with better ergonomics later

hgzimmerman · 2019-11-29T15:02:24Z

examples/file_upload/src/lib.rs

@@ -80,7 +80,7 @@ impl Component for Model {
                </div>
                <div>
                    <label>{ "By chunks" }</label>
-                    <input type="checkbox" checked=flag onclick=|_| Msg::ToggleByChunks />
+                    <input type="checkbox" checked=flag onclick=self.link.send_back(|_| Msg::ToggleByChunks) />


While I think that send_back is fine, It might be worth considering just renaming it to callback or make_callback to make it painfully obvious what is happening now that this function is being used in many more places.

I think onclick=self.link.callback(|_| Msg::ToggleByChunks) is more readable and understandable to new users then the present naming scheme, which seems to be named more according to its implementation details rather than its purpose to users.

I propose the following renames - which might be better addressed in a separate PR, if at all:

send_back -> callback

send_back_batch -> batch_callback

send_self -> process | immediate | queue_message

send_future -> future (but this is being moved elsewhere anyways)

If it's possible to access the link via the Component trait, it would be great to have something like self.callback or self.message provided by the trait as well (accessing the ComponentLink behind the scenes).

Yeah, the ideal ergonomic solution would be:

onclick=self.callback(|_| Msg::ToggleByChunks)

And people would be free to create traits to add methods that make it even more terse if they so desired:

onclick=self.cb(|_| Msg::ToggleByChunks)

Edit: or even a real dumb macro:

onclick=cb!(|_| Msg::ToggleByChunks)

that expands to the aforementioned:

onclick=self.callback(|_| Msg::ToggleByChunks)

(although it wouldn't work in functions not associated to a component)

In the event that Component links can't be gotten at arbitrary points, in order to add self.callback() as a short option , a Linked trait could be added to Yewtil, where users could define or derive which one of their fields is a ComponentLink, and then provide callback/batch_callback methods for the component instance.

Like:

#[derive(Linked)] pub struct Model { #[link] link: ComponentLink<Self> } //...later let cb = self.callback(|_| Msg::Something);

Leveraged some macro magic in this draft PR: https://github.com/jstarry/yew/pull/3/files

Thinking it might be too magic though 🤔 let me know what y'all think!

I'm impressed, but I don't have an affinity for using function-positioned macros to define structs, in that they don't end up immediately looking like struct declarations, and they tend to confuse IDE's syntax highlighting.

If I recall correctly, Scopes have a default instance created within a html! block, which is effectively attached to the "real" shared state later when the Html is integrated into the existing vdom state.
Could the same be done for ComponentLinks?
Being able to create links from thin air would be a nice shortcut around this problem.

I think either that, providing links on lifecycle hooks (or maybe just view?), or attribute-based or automatic deduction of the link field within the above proposed Linked derive trait, might be acceptable alternatives.

Edit with examples:

fn view(&mut self) -> Html { let link: ComponentLink<Self> = ComponentLink::default(); // Not sure if this could work html! { <button onclick=link.callback(|_| ()) /> } }

// maybe update, mounted, change would have to rely on the link provided in // create if they want to support creating callbacks, etc fn view(&mut self, link: ComponentLink<Self>) -> Html { html! { <button onclick=link.callback(|_| ()) /> } }

I'm sort of fine with the idea of having the boilerplate of having to put a link in every component state. You can accomplish the above terseness by just making a local variable to the link:

fn view(&mut self) -> Html { let link: ComponentLink<Self> = self.link; html! { <button onclick=link.callback(|_| ()) /> } }

or even better:

fn view(&mut self) -> Html { let callback = self.link.callback; html! { <button onclick=callback(|_| ()) /> } }

I think let callback = self.link.callback; or let cb = self.link.callback; becoming an idiom solves the problem of verbosity within html!{} blocks. I don't know if something short of your proposal will make removing boilerplate from creation possible though.

Maybe extending Linked could allow implementing a LinkedComponent trait that doesn't have a create method. Properties would have to be annotated, link could be automatically be deduced based on its type. This would all live in yewtil.

eg:

#[derive(Linked)] pub struct Model { link: ComponentLink<Self>, #[properties] props: Props, some_value: usize } impl LinkedComponent for Model { type Message = Msg; fn update(&mut self, msg: Self::Message) -> ShouldRender {} // etc... } // --- // The auto-derived create would look like: fn create(props: Props, link: ComponentLink<Self>) -> Self { Self { link: link, props: props, some_value: Default::default() } }

//---
// and creating it could be done like:

html!{ // Link implements Component and makes use of Model's LinkedComponent <Link<Model> with props /> }

After working with this a bit more, the boilerplate bits aren't actually too too bad. It feels repetitive when you're creating a new component, but the frequency of doing that is low, and it's easily circumvented with a bit of text-editor config for autocompletes and the like.

The one thing that I could see being useful (although I'm unsure how feasible it is) could be if the html! macro accepted a link (callback?) to bind event callbacks as was currently done. E.g.:

fn view(&self) -> Html { html!({ <button onclick=|_| Msg::Clicked>{"Click me!"}</button> }, self.link) }

This would have to be an optional thing however (for component structs that don't have an explicitly stored ComponentLink member) and it wouldn't work if there was a mix of ComponentLinks that should be used in the view such as if making a table component where the table row can be selected (handled by the Table component), but the contents of the table row could contain a button that when clicked, needs to message the top level component (I can't remember whether we're calling that the base or parent... the most outer one though).

Might be best to just allow for some yew-idioms to develop and see what patterns emerge before making more drastic changes though.

I love the idea of providing a link to the html! macro - as long as falling back to onclick=self.link.callback(etc) is allowed if it isn't provided. The only minor issue I have with it is that it isn't possible to propagate that through inner control structures (I think), or html blocks defined elsewhere:
eg:

html! ({ <> <div> {"text"} </div> { if cond { html!({...}, self.link) } else { html!({...}, self.link) } } }, self.link) // doesn't propagate to inner html!s

It would undoubtedly save a bunch of people's code from breaking because it would allow them to keep the same old html syntax by just adding a self.link to the end of their html, but at the same time, for branchy code with sparse callbacks, it might get in the way more than it would help, as well as introducing multiple ways to accomplish the same thing - leading to confusion. There is also the question about whether adding the additional complexity to the parser is worth it.

I am in agreement that link being required everywhere isn't as much of a problem as I initially made it out to be.

How about we follow up on the ergonomics in another issue?

hgzimmerman · 2019-11-29T15:38:26Z

Another thought for a follow up PR:

Take the new syntax you proposed:

fn view(&self) -> Html {
  let onclick = self.link.send_back(|_| Msg::DoIt);
  html! {
    <button onclick=onclick>{ "Click me!" }</button>
  }
}

and allow eliding the name of the callback if it matches a callback in scope. like:

fn view(&self) -> Html {
  let onclick = self.link.send_back(|_| Msg::DoIt);
  html! {
    <button onclick>{ "Click me!" }</button>
  }
}

This might introduce some confusion, and may be incompatible with the bool / option coercion mentioned in #745

jstarry · 2019-12-01T21:09:17Z

@hgzimmerman I updated the PR with the ComponentLink renames but opted for send_message

mdtusz · 2019-12-02T18:15:25Z

After making the changes to my codebase, big fan of the changes from send_back -> callback and send_self -> send_message. Makes it much easier to grok when scannng quickly!

hgzimmerman · 2019-12-05T19:29:33Z

@hgzimmerman could you please try out this branch for yew-router?

Starting work here: yewstack/yew_router#188

Currently encountering the following error:

error[E0277]: the trait bound `M: std::clone::Clone` is not satisfied
   --> src/router.rs:186:16
    |
186 | impl<T, SW, M> Component for Router<T, SW, M>
    |                ^^^^^^^^^ the trait `std::clone::Clone` is not implemented for `M`
    |
    = help: consider adding a `where M: std::clone::Clone` bound
    = note: required because of the requirements on the impl of `std::clone::Clone` for `router::Props<T, SW, M>`

error[E0277]: the trait bound `M: std::clone::Clone` is not satisfied
   --> src/router.rs:167:10
    |
167 | #[derive(Properties, Clone)]
    |          ^^^^^^^^^^ the trait `std::clone::Clone` is not implemented for `M`
    |
    = help: consider adding a `where M: std::clone::Clone` bound
    = note: required because of the requirements on the impl of `std::clone::Clone` for `router::Props<T, SW, M>`

I'm running into problems where the Properties derive macro wants all of my bounds to implement the bounds present on the Props, even though the use of an Rc means that if my M (message) type parameter should not have to be Clone.

I could add a Clone bound to M, but I shouldn't have to.

I've noticed this problem before, but its kind of hard to nail down into a concrete Issue.

I wouldn't let this stop you from merging this PR, but at the very least an Issue should be opened with a minimum example that reproduces this compiler error.

Edit:
I should mention that this isn't a regression specifically introduced by this build, but is presenting itself now via the new requirement that Props implement Clone.

hgzimmerman · 2019-12-06T15:37:24Z

Update on Yew Router's migration:

Well, it turns out that the router doesn't need to be generic over the sort of messages inner callbacks could produce, because callbacks are now explicitly associated to a given link, so the M type parameter could be removed - completely sidestepping the problem of strange bounds requirements.

At this point, the router crate is completely able to accommodate the changes in this PR.

jstarry · 2019-12-06T16:04:01Z

@hgzimmerman great to hear. Do you think the Clone issue you were seeing was a result of using the derive macro for Clone?

hgzimmerman · 2019-12-06T21:55:42Z

It might be, but I can't say. I do want to dive into this in the near future and I'll try to hold myself to opening an issue with a reproduction of this error this weekend or early next week.

jstarry · 2019-12-08T03:03:13Z

Closing in favour of smaller PRs, thanks for the discussion!

jstarry · 2019-12-08T03:03:41Z

I will keep the degenerify branch up until master reaches parity

hgzimmerman · 2019-12-08T04:30:49Z

I'll switch yew_router and yewtil over to rely on yew:master tomorrow.

jstarry added the breaking change label Nov 28, 2019

jstarry mentioned this pull request Nov 28, 2019

Support non-literal-closures for VTag event handlers #733

Closed

jstarry mentioned this pull request Nov 28, 2019

Auto-convert Msg #691

Closed

jstarry requested a review from therustmonk November 28, 2019 04:55

This was referenced Nov 28, 2019

New event macros cause extremely confusing error message #251

Closed

Features expected for 1.0 version #136

Closed

jstarry requested a review from hgzimmerman November 29, 2019 03:48

hgzimmerman reviewed Nov 29, 2019

View reviewed changes

jstarry force-pushed the degenerify branch from 9983c85 to 1216fd8 Compare December 1, 2019 00:02

This was referenced Dec 3, 2019

yew-macro: Handling of boolean properties #745

Closed

yew-macro: Allow omitting redundant property names #769

Closed

jstarry added 6 commits December 3, 2019 10:16

Remove generics from virtual dom and fix nested components

a8ddc6f

Fix clippy

376aad1

cargo fmt

6c5bf71

Rename send_back to callback

42a5c76

Rename send_self to send_message

236bf37

Add example with subcomponent calling wrapper component

f1459a8

jstarry force-pushed the degenerify branch from 9de9f5d to 141828c Compare December 3, 2019 16:42

Filter empty string classes

9ed69e4

jstarry force-pushed the degenerify branch from 141828c to 946016b Compare December 3, 2019 16:47

Reuse Transformer

d2afad2

jstarry force-pushed the degenerify branch from 946016b to d2afad2 Compare December 3, 2019 17:03

Merge branch 'master' into degenerify

d37911d

hgzimmerman mentioned this pull request Dec 7, 2019

Support passing callbacks to elements #777

Merged

jstarry closed this Dec 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove generics from virtual dom and fix nested components #756

Remove generics from virtual dom and fix nested components #756

jstarry commented Nov 28, 2019 •

edited

Loading

jstarry commented Nov 28, 2019

mdtusz commented Nov 28, 2019

hgzimmerman left a comment

hgzimmerman Nov 29, 2019

jstarry Dec 3, 2019

hgzimmerman Nov 29, 2019 •

edited

Loading

hgzimmerman Nov 29, 2019

hgzimmerman Nov 29, 2019

hgzimmerman Nov 29, 2019

jstarry Dec 3, 2019

hgzimmerman Nov 29, 2019

mdtusz Nov 29, 2019

hgzimmerman Nov 29, 2019 •

edited

Loading

hgzimmerman Nov 29, 2019

jstarry Dec 1, 2019

hgzimmerman Dec 2, 2019 •

edited

Loading

hgzimmerman Dec 2, 2019 •

edited

Loading

mdtusz Dec 2, 2019

hgzimmerman Dec 2, 2019

jstarry Dec 3, 2019

hgzimmerman commented Nov 29, 2019

jstarry commented Dec 1, 2019

mdtusz commented Dec 2, 2019

hgzimmerman commented Dec 5, 2019 •

edited

Loading

hgzimmerman commented Dec 6, 2019

jstarry commented Dec 6, 2019

hgzimmerman commented Dec 6, 2019

jstarry commented Dec 8, 2019

jstarry commented Dec 8, 2019

hgzimmerman commented Dec 8, 2019

Remove generics from virtual dom and fix nested components #756

Remove generics from virtual dom and fix nested components #756

Conversation

jstarry commented Nov 28, 2019 • edited Loading

The virtual dom is now cloneable

Properties need to be cloneable again

Html<Self> -> Html No more generics!

No more magic closures into callbacks

jstarry commented Nov 28, 2019

mdtusz commented Nov 28, 2019

hgzimmerman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hgzimmerman Nov 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hgzimmerman Nov 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hgzimmerman Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

hgzimmerman Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hgzimmerman commented Nov 29, 2019

jstarry commented Dec 1, 2019

mdtusz commented Dec 2, 2019

hgzimmerman commented Dec 5, 2019 • edited Loading

hgzimmerman commented Dec 6, 2019

jstarry commented Dec 6, 2019

hgzimmerman commented Dec 6, 2019

jstarry commented Dec 8, 2019

jstarry commented Dec 8, 2019

hgzimmerman commented Dec 8, 2019

jstarry commented Nov 28, 2019 •

edited

Loading

`Html<Self>` -> `Html` No more generics!

hgzimmerman Nov 29, 2019 •

edited

Loading

hgzimmerman Nov 29, 2019 •

edited

Loading

hgzimmerman Dec 2, 2019 •

edited

Loading

hgzimmerman Dec 2, 2019 •

edited

Loading

hgzimmerman commented Dec 5, 2019 •

edited

Loading