Skip to content
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

Integrate use_ref with NodeRef #1642

Closed
Tracked by #2052
ranile opened this issue Nov 9, 2020 · 10 comments · Fixed by #2093
Closed
Tracked by #2052

Integrate use_ref with NodeRef #1642

ranile opened this issue Nov 9, 2020 · 10 comments · Fixed by #2093
Labels
A-yew Area: The main yew crate

Comments

@ranile
Copy link
Member

ranile commented Nov 9, 2020

This was initially posted in #1129. This issue tracks its progress.

I'd love to have what @jstarry meant by this "integration" explained here because as it stands, use_ref works well with NodeRef. Example:

#[function_component(UseRef)]
fn ref_hook() -> Html {
    let (outer_html, set_outer_html) = use_state(|| "".to_string());

    let node_ref = use_ref(|| NodeRef::default());

    let on_click = {
        let node_ref = Rc::clone(&node_ref);

        Callback::from(move |e| {
            let to_set = (*node_ref.borrow().deref())
                .cast::<yew::web_sys::Element>()
                .unwrap()
                .outer_html();
            set_outer_html(to_set)
        })
    };
    html! {
        <>
            <div id="result" ref=(*node_ref.borrow_mut().deref_mut()).clone()>{ "Filler" }</div>
            {outer_html}
            <br />
            <button onclick=on_click>{"Refresh"}</button>
        </>
    }
}

Here's the output displayed after clicking on "Refresh" button:
image

@lukechu10
Copy link
Contributor

Can this issue be closed since it seems like it works already?

@siku2
Copy link
Member

siku2 commented Dec 12, 2020

It's still unclear what is actually desired here. use_ref currently brings nothing to the table that helps with NodeRef (it's actually much more verbose than using it with use_state) because NodeRef already comes with interior mutability.

The entire hook API is a bit all over the place so I believe this issue is justified to discuss a better approach to this.

@jstarry
Copy link
Member

jstarry commented Aug 8, 2021

I'd love to have what @jstarry meant by this "integration" explained here because as it stands, use_ref works well with NodeRef. Example:

@hamza1311 my intent was just to ensure that use_ref and NodeRef play well together and are easy to use since I imagine it to be a fairly common pattern that devs use. As indicated by your example, they already do work fine together so I don't think there's much to do here besides maybe some docs and an additional api method which is more ergonomic / discoverable?

@mc1098
Copy link
Contributor

mc1098 commented Aug 9, 2021

It works but it's not very ergonomic to use use_ref with NodeRef and the internal use_hook will store an equivalent to this:

Rc<RefCell<Rc<RefCell<Rc<RefCell<NodeRefInner>>>>>>
                      ^^^^^^^^^^^^^^^^^^^^^^^^^ NodeRef part

Which is lots of interior mutability 👀

Might be worth biting the bullet here and just making a specific hook for NodeRef which has a simple implementation:

pub fn use_node_ref() -> NodeRef {
    use_hook(NodeRef::default, |state, _| state.clone(), |_| {})
}

Then the example above would look like this:

#[function_component(UseRef)]
fn ref_hook() -> Html {
   let outer_html = use_state(|| "".to_string());
   let node_ref = use_node_ref();

   let onclick = {
       let node_ref = node_ref.clone();
       let outer_html = outer_html.clone();

       Callback::from(move |_| {
           let to_set = node_ref
               .cast::<yew::web_sys::Element>()
               .unwrap()
               .outer_html();
           outer_html.set(to_set)
       })
   };
   html! {
       <>
           <div id="result" ref={node_ref}>{ "Filler" }</div>
           {(*outer_html).clone()}
           <br />
           <button {onclick}>{"Refresh"}</button>
       </>
   }
}

syntax updated to work on master

Just thought I'd add this into the mix :)

@ranile
Copy link
Member Author

ranile commented Aug 9, 2021

I had a thought of having a trait do the trick. NodeRef won't have interior mutability itself but the mutability aspect will be provided by the trait. I'm not sure if it's even possible to do it or how it implement it... just a thought.

@mc1098
Copy link
Contributor

mc1098 commented Aug 9, 2021

If you could manage it then that would be even better than having a separate hook :)
As it stands I think use_ref with NodeRef is not a pleasant experience.

@mc1098
Copy link
Contributor

mc1098 commented Aug 31, 2021

I've been playing around with this bit more and I do think that without specialization integrating NodeRef with use_ref is always going to be bit painful, either ergonomically or just because for every NodeRef you are double wrapping it with Rc + RefCell and adding some trait or impl that hides the borrow & deref stuff. The requirement is different for normal refs and NodeRef as the latter only ever requires immutable borrows whereas the former is really for mutably borrowing and editing,

I think it will be quite common for users to want to have multiple NodeRefs, form components come to mind. This in mind I do have the following working:

// type can be inferred but added for clarity :)
let [a, b, c]: &[NodeRef; 3] = &*use_node_ref();

where use_node_ref fn signature looks like this:

pub fn use_node_ref<const N: usize>() -> Rc<[NodeRef; N]>;

The Rc here is why we need to deref then borrow i.e. &* and we need the Rc so we can clone and get to a copy of the array from the runner closure in use_hook.

The downside to this is that currently it requires a sprinkling of unsafe to achieve - there are actually quite a few ways to get this same effect but most of the safe ones are currently hiding in nightly 🙃..

One way but isn't quite as ergonomic is:
As NodeRef implements Default you can use this to create up to [NodeRef; 32], however, you can only get this working by calling the Default::default as the FnOnce() -> T when using use_node_ref (until const generic evaluation stabilises) - so it looks like this:

let [a, b, c] = &*use_node_ref(Default::default);

where use_node_ref fn signature looks like this:

pub fn use_node_ref<const N: usize>(init: impl FnOnce() -> [NodeRef; N]) -> Rc<[NodeRef; N]>;

Thought I'd add these points into consideration too :)

@ranile
Copy link
Member Author

ranile commented Aug 31, 2021

How is

pub fn use_node_ref<const N: usize>() -> Rc<[NodeRef; N]>;

better than

pub fn use_node_ref() -> NodeRef;

The hook can be called multiple times to create new node refs. It can be implemented without nightly or unsafe like you described in another comment above.

Also, when we add that hook, it would be better to rename use_ref to use_mutable_ref/use_mut_ref.

@mc1098
Copy link
Contributor

mc1098 commented Aug 31, 2021

How is

pub fn use_node_ref<const N: usize>() -> Rc<[NodeRef; N]>;

better than

pub fn use_node_ref() -> NodeRef;

It was just another idea really and wasn't sure how often people use multiple NodeRefs in the wild :) but unless users need few or more NodeRef then it's probably not really "better" as with the single use version we can avoid an extra Rc and for the short term avoid the unsafe / nightly or kind of awkward Default::default call.

Also, when we add that hook, it would be better to rename use_ref to use_mutable_ref/use_mut_ref.

Agreed, prefer use_mut_ref :D

@mc1098
Copy link
Contributor

mc1098 commented Sep 13, 2021

So if use_ref becomes use_mut_ref. We can have a use_ref that is just an Rc<T> and then I think that works with NodeRef, not quite as nice as use_node_ref but I think it would be enough and wouldn't warrant having a new use_ref and use_node_ref together imo. What do you think?

@mc1098 mc1098 added the A-yew Area: The main yew crate label Sep 20, 2021
@mc1098 mc1098 mentioned this issue Sep 30, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew Area: The main yew crate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants