Add set_if_neq for UseStateHandle

[voidpumpkin]

Description

Added a set_neq method for UseStateHandle that will only cause a re render when value is different than already set

Fixes #2104

Checklist

• I have run cargo make pr-flow
• I have reviewed my own code
• I have added tests

[github-actions]

Visit the preview URL for this PR (updated for commit c63c5df):

https://yew-rs--pr2109-master-7ptlqx8t.web.app

_{(expires Fri, 22 Oct 2021 00:30:59 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

[voidpumpkin]

Docs look good on the preview link

[mc1098]

Just want to expand on the docs a little bit more.

Last call for bike shedding the name :) @yoroshikun, @WorldSEnder, @hamza1311 (thought I'd ping you too as you were part of the discussion on Discord)

It has a 👍 from me - thank you for driving this forward @voidpumpkin :)

[WorldSEnder]

Bikeshedding input:

• set_neq has the sound of setting a value named neq. set_if_neq maybe?
• one could call the conditional set method simply update. Updates usually do not get applied if the version is already installed. Would have to rephrase the documentation of fn set to "Replaces the value".

[voidpumpkin]

Bikeshedding input:

* `set_neq` has the sound of setting a value named `neq`. `set_if_neq` maybe?

* one could call the conditional set method simply `update`. Updates usually do not get applied if the version is already installed. Would have to rephrase the documentation of `fn set` to "Replaces the value".

@WorldSEnder Loved the suggestions and implemented them both

[futursolo]

I think this implementation might not work reliably if the handle is moved into a callback / event listener.

(This might not be a good example, but is one I have on hand.)

use web_sys::window;
use yew::prelude::*;
use yew_router::attach_route_listener;
use yew_router::prelude::*;

#[derive(Routable, Debug, Clone, PartialEq)]
pub enum Route {
    #[at("/")]
    Home,
    #[at("/a")]
    A,
    #[at("/b")]
    B,
}

#[function_component(Content)]
fn content() -> Html {
    let pathname = use_state(|| window().unwrap().location().pathname().unwrap());
    let route = use_state(Route::current_route);

    let pathname_clone = pathname.clone();
    let route_clone = route.clone();
    use_state(move || {
        attach_route_listener(Callback::from(move |r: Option<Route>| {
            route_clone.set_if_neq(r);
            pathname_clone.set_if_neq(window().unwrap().location().pathname().unwrap());
        }))
    });

    html! {
        <>
            <div>{"Listener: "}{format!("{:?}", *route)}</div>
            <div>{"Pathname: "}{pathname.to_string()}</div>
            <Link<Route> route={Route::Home}>{"Home"}</Link<Route>>
            <Link<Route> route={Route::A}>{"A"}</Link<Route>>
            <Link<Route> route={Route::B}>{"B"}</Link<Route>>
        </>
    }
}

#[function_component(App)]
fn app() -> Html {
    let render_route = Router::render(|_| {
        html! {<Content />}
    });

    html! {<Router<Route> render={render_route} />}
}

fn main() {
    yew::start_app::<App>();
}

In the example above, the component will not rerender if the new value is / even if current path is not /.

You can replicate this by clicking: Home -> A -> Home

[mc1098]

I think this implementation might not work reliably if the handle is moved into a callback / event listener.

It does not, this is to do with how use_state works internally - it keeps replacing the Rc so any UseStateHandler value becomes stale after a setter has been called. Normally this doesn't cause an issue because we often rebuild Callbacks on each render and thereby updating the UseStateHandle. The set_if_neq method probably highlights this more but the effect would be the same if you did the equality check manually too.

This is an issue that either needs resolving or documenting but I think it should become its own issue as this PR doesn't cause the issue and any resolution would equally apply to set_if_neq. I just went with this thought and added #2112 so we can discuss it more there.

[futursolo]

I think set_if_neq should replicate the behaviour of React's useState setter where it reliably sets the value if current value does not equal to new value and this does not always have something to do with the current value that the handle holds.

const [height, setHeight] = useState(0);


useEffect(() => {
    // height can become stale but setHeight still works.
    window.addEventListener("resize", () => setHeight(window.innerHeight));
}, []);

I think this behaviour can be achieved by implementing set_if_neq as updater.callback instead of doing checks before calling .set.

[mc1098]

The issue with doing the check in the callback is that you haven't solved the whole problem - if a user prefers to perform an equality check manually before using set then they will still run into the same issue. The answer might be _use set_if_neq but I think that's inconsistent behaviour, it might be the best solution and we'd have to document this fact but I think this still needs further discussion in #2112.

At present this PR acts in the same manner as the manual approach:

// Manual equality check
if *state != new_state {
    state.set(new_state);
}

// new
state.set_if_neq(new_state);

Is it perfect behaviour, no, but it is consistent with the current semantics of use_state - I think if you actually fix the stale data issue then both these approaches would be equal in behaviour. Also if we did do it in the callback then I think we'd need a separate setter closure stored in UseStateHandle.

Not letting this PR through because of this issue actually doesn't change the fact that the manual approach still has the behaviour you described.

I would like to see some resolution to #2112 before the next release though - I want it to work with router listener and this also effects bridge callbacks too.

[voidpumpkin]

I think this implementation might not work reliably if the handle is moved into a callback / event listener.

(This might not be a good example, but is one I have on hand.)

use web_sys::window;
use yew::prelude::*;
use yew_router::attach_route_listener;
use yew_router::prelude::*;

#[derive(Routable, Debug, Clone, PartialEq)]
pub enum Route {
    #[at("/")]
    Home,
    #[at("/a")]
    A,
    #[at("/b")]
    B,
}

#[function_component(Content)]
fn content() -> Html {
    let pathname = use_state(|| window().unwrap().location().pathname().unwrap());
    let route = use_state(Route::current_route);

    let pathname_clone = pathname.clone();
    let route_clone = route.clone();
    use_state(move || {
        attach_route_listener(Callback::from(move |r: Option<Route>| {
            route_clone.set_if_neq(r);
            pathname_clone.set_if_neq(window().unwrap().location().pathname().unwrap());
        }))
    });

    html! {
        <>
            <div>{"Listener: "}{format!("{:?}", *route)}</div>
            <div>{"Pathname: "}{pathname.to_string()}</div>
            <Link<Route> route={Route::Home}>{"Home"}</Link<Route>>
            <Link<Route> route={Route::A}>{"A"}</Link<Route>>
            <Link<Route> route={Route::B}>{"B"}</Link<Route>>
        </>
    }
}

#[function_component(App)]
fn app() -> Html {
    let render_route = Router::render(|_| {
        html! {<Content />}
    });

    html! {<Router<Route> render={render_route} />}
}

fn main() {
    yew::start_app::<App>();
}

In the example above, the component will not rerender if the new value is / even if current path is not /.

You can replicate this by clicking: Home -> A -> Home

I don't know if this is how you prefer to do it in yew, but i would expect to treat agents like event listeners.
Event listeners in react/yew are attached using useEffect with providing the cleanup function that deletes the old listener
This way every re render the setter is updated

Here is react example https://reactjs.org/docs/hooks-effect.html#example-using-hooks-1

[voidpumpkin]

I think this implementation might not work reliably if the handle is moved into a callback / event listener.

(This might not be a good example, but is one I have on hand.)

use web_sys::window;
use yew::prelude::*;
use yew_router::attach_route_listener;
use yew_router::prelude::*;

#[derive(Routable, Debug, Clone, PartialEq)]
pub enum Route {
    #[at("/")]
    Home,
    #[at("/a")]
    A,
    #[at("/b")]
    B,
}

#[function_component(Content)]
fn content() -> Html {
    let pathname = use_state(|| window().unwrap().location().pathname().unwrap());
    let route = use_state(Route::current_route);

    let pathname_clone = pathname.clone();
    let route_clone = route.clone();
    use_state(move || {
        attach_route_listener(Callback::from(move |r: Option<Route>| {
            route_clone.set_if_neq(r);
            pathname_clone.set_if_neq(window().unwrap().location().pathname().unwrap());
        }))
    });

    html! {
        <>
            <div>{"Listener: "}{format!("{:?}", *route)}</div>
            <div>{"Pathname: "}{pathname.to_string()}</div>
            <Link<Route> route={Route::Home}>{"Home"}</Link<Route>>
            <Link<Route> route={Route::A}>{"A"}</Link<Route>>
            <Link<Route> route={Route::B}>{"B"}</Link<Route>>
        </>
    }
}

#[function_component(App)]
fn app() -> Html {
    let render_route = Router::render(|_| {
        html! {<Content />}
    });

    html! {<Router<Route> render={render_route} />}
}

fn main() {
    yew::start_app::<App>();
}

In the example above, the component will not rerender if the new value is / even if current path is not /.

You can replicate this by clicking: Home -> A -> Home

@futursolo
I looked more into your example.
attach_route_listener creates a new event listener. It makes sense that it gets stale because you create it only in the first render.

Here is how I would solve your problem:

Essentially you need to keep route_listener in use_state and call its setter every time route state changes using Use_effect.
(No need to cleanup like in react, because event listeners get clean up on Drop)

Here is the solution to your specific problem: https://github.com/voidpumpkin/yew-playground/tree/stale-agents

@mc1098 I don't think #2112 is even a real issue since in react world it works exactly like that.
Closures concept is just hard sometimes to wrap your head around.

PS:
Im not sure why yew has no use_memo or use_callback hook

[futursolo]

but i would expect to treat agents like event listeners.

Agent dies when all bridges to it are dropped. If the current bridge is the only bridge to an agent, every time this is destroyed / created, it will cause the agent to be repeatedly created / destroyed at the same time.

In addition, for private and job agents, each time a new bridge is created, it creates a new agent, which means any previous state in the agent is lost when re-bridged (this also applies to public and context if current bridge is the only bridge).

Essentially you need to keep route_listener in use_state and call its setter every time route state changes using Use_effect.
(No need to cleanup like in react, because event listeners get clean up on Drop)

In React, if you need to register an event listener that only uses the setter of a state, all of the following ways work:

useEffect(cb, []);

// probably recommended approach, but setter of a state never changes anyways.
useEffect(cb, [setValue]);

// or
useEffect(cb);

[voidpumpkin]

but i would expect to treat agents like event listeners.

Agent dies when all bridges to it are dropped. If the current bridge is the only bridge to an agent, every time this is destroyed / created, it will cause the agent to be repeatedly created / destroyed at the same time.

In addition, for private and job agents, each time a new bridge is created, it creates a new agent, which means any previous state in the agent is lost when re-bridged (this also applies to public and context if current bridge is the only bridge).

Essentially you need to keep route_listener in use_state and call its setter every time route state changes using Use_effect.
(No need to cleanup like in react, because event listeners get clean up on Drop)

In React, if you need to register an event listener that only uses the setter of a state, all of the following ways work:

useEffect(cb, []);

// probably recommended approach, but setter of a state never changes anyways.
useEffect(cb, [setValue]);

// or
useEffect(cb);

I somehow missed that my fix did not fix anything :D

@futursolo
Yeah i guess you are right about the set function always working, Only value closures would end up old, but the setter would always work 🤔

[voidpumpkin]

Still i don't think this separate issue blocks this PR

Or you think a solution for #2112 would create a world where set_if_neq is not even possible to implement

[mc1098]

Still i don't think this separate issue blocks this PR

Agreed.

[mc1098] Not letting this PR through because of this issue actually doesn't change the fact that the manual approach still has the behaviour you described.

I see set_neq as a convenience method for doing the check manually before using the set method, therefore if the behaviour is consistent between the two approaches then this PR is fine.

I agree #2112 needs to be resolved somehow but shouldn't block this PR because that wouldn't stop the bug from existing or really change how and when you'd hit this bug (it just might make it more convenient to do so).

[voidpumpkin]

@futursolo Sorry for the spam.. But in the end I did solve the problem by applying two fixes: https://github.com/voidpumpkin/yew-playground/tree/stale-agents

First applied fix

attach_route_listener always provides not the new route but previous route.
You can notice this in the console logs

Second applied fix

stale setter gets countered by simply creating a new event listener on every route state change

Gif

Other comments

I noticed that new router does not use an agent, it just creates an event popstate listener on DOM window object.
So my fix I think can be considered correct, if attach_route_listener providing old route gets fixed.

I still think there are cases where stale setters create problems, would be good to get a small reproducible case in #2112

cc @mc1098 (Again sorry for spam)

[mc1098]

It is just a question whether a bool return would be useful :)

Apart from that I'm happy with this PR.