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

Prepared States dependency should be Reference Counted #2769

Merged
merged 9 commits into from Jul 19, 2022

Conversation

futursolo
Copy link
Member

Description

When using prepared states, Future requires to be 'static so they can be spawned via spawn_local.
Currently, prepared states accept a FnOnce<&D> -> impl Future<T> + 'static type as its closure type.

If the closure is not rewritten, the lifetime of &D can be extended with cloning.

|deps| {
    let deps = deps.clone();
    async move {
        // ...
    }
}

However, as we rewrite prepared states from async closure to closure + async block automatically, users do not have the ability to add any statements between the closure and async block.

This pull request adjusts the signature so that it passes Rc<D> instead of &D so the async block can be guaranteed to live 'static.

Checklist

  • I have reviewed my own code
  • I have added tests

@futursolo futursolo added A-yew Area: The main yew crate A-yew-macro Area: The yew-macro crate labels Jul 2, 2022
github-actions[bot]
github-actions bot previously approved these changes Jul 2, 2022
@github-actions
Copy link

github-actions bot commented Jul 2, 2022

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

https://yew-rs-api--pr2769-prepared-rc-z5rk6w8p.web.app

(expires Tue, 26 Jul 2022 12:59:14 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Jul 2, 2022

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
boids 173.451 173.458 +0.007 +0.004%
contexts 110.151 110.146 -0.006 -0.005%
counter 87.037 87.037 0 0.000%
counter_functional 87.704 87.704 0 0.000%
dyn_create_destroy_apps 90.146 90.146 -0.001 -0.001%
file_upload 103.503 103.503 0 0.000%
function_memory_game 168.675 168.673 -0.002 -0.001%
function_router 354.133 354.107 -0.025 -0.007%
function_todomvc 162.423 162.414 -0.009 -0.005%
futures 226.061 226.059 -0.002 -0.001%
game_of_life 107.724 107.726 +0.002 +0.002%
immutable 210.046 210.053 +0.007 +0.003%
inner_html 84.057 84.057 0 0.000%
js_callback 113.377 113.375 -0.002 -0.002%
keyed_list 195.877 195.875 -0.002 -0.001%
mount_point 86.680 86.683 +0.003 +0.003%
nested_list 116.264 116.263 -0.001 -0.001%
node_refs 93.981 93.982 +0.001 +0.001%
password_strength 1546.875 1546.875 0 0.000%
portals 97.866 97.871 +0.005 +0.005%
router 322.038 322.036 -0.002 -0.001%
simple_ssr 154.939 154.939 0 0.000%
ssr_router 400.279 400.156 -0.123 -0.031%
suspense 111.041 111.042 +0.001 +0.001%
timer 89.761 89.762 +0.001 +0.001%
todomvc 143.397 143.397 0 0.000%
two_apps 87.692 87.692 0 0.000%
web_worker_fib 154.684 154.684 0 0.000%
webgl 87.950 87.947 -0.003 -0.003%

✅ None of the examples has changed their size significantly.

@futursolo futursolo changed the title Prepared States dependency should be Referenced Counted Prepared States dependency should be Reference Counted Jul 2, 2022
@hamza1311
Copy link
Member

hamza1311 commented Jul 2, 2022

I'd argue that we should add a Clone bound on deps so they don't end up being double Rcd. See the following example:

#[derive(Properties, PartialEq)]
struct CloneProps {
    state: Vec<u8>,
}

#[function_component)]
fn Cloned(props: &CloneProps) -> Html {
    let state = use_preapred_state!(async move |state: Rc<Vec<u8>>| {  }}, props.state.clone()) // notice how this clone is required

    html! {}
}

#[derive(Properties, PartialEq)]
struct RcProps {
    state: Rc<Vec<u8>>,
}

#[function_component]
fn RcComp(props: &RcProps) -> Html {
    let state = use_preapred_state!(async move |state: Rc<Rc<Vec<u8>>>| {  /* now there's double Rc*/ }}, Rc::clone(&props.state)) // No clone of data is done

    html! {}
}

#[function_component]
fn Parent() -> Html {
    let state = use_state(Vec::<u8>::new)

    html! { 
        <div>
            <RcComp state={Rc::clone(&*state)} /> // this won't compile, but that is a another issue that we should deal with
            <Cloned state={(*state).clone()} />
        </div>
    }
}

Notice how we can avoid the clone by modifying use_state to return an Rc. I'm not entirely sure if that's possible. If it's not, then we have no choice but to deal with double Rc. Do you think it's better to keep that exposed that to the user or to make it hidden (like this PR does)

This problem isn't limited to this hook either. It spans to other hooks as well, such as use_future_with_deps.

@futursolo
Copy link
Member Author

futursolo commented Jul 3, 2022

serde does not implement Serialize and Deserialize for Rc<T> by default.

See: https://serde.rs/feature-flags.html#-features-rc

I think we have 2 ways to solve this:

  1. Accept deps as-is and Provide Rc<T> to the closure.
  2. Create a sealed trait ReferenceCounted to ensure only Rc<T> and Arc<T> is passed as deps and use associated types on trait (<D as ReferenceCounted>::Stored) for extraction of T to serialise.

@futursolo futursolo mentioned this pull request Jul 3, 2022
2 tasks
github-actions[bot]
github-actions bot previously approved these changes Jul 18, 2022
@hamza1311
Copy link
Member

There's another option of making the parameter PartialEq + Clone and forcing users to Rc::new it. That would cover the cases where Rc is not needed and where the value is already an Rc

github-actions[bot]
github-actions bot previously approved these changes Jul 19, 2022
github-actions[bot]
github-actions bot previously approved these changes Jul 19, 2022
@futursolo
Copy link
Member Author

There's another option of making the parameter PartialEq + Clone and forcing users to Rc::new it. That would cover the cases where Rc is not needed and where the value is already an Rc

We cannot do this with Clone + PartialEq because Rc<T> is not Serialize + Deserialize even if T is.

To achieve this, we need a way to extract T: Serialize + Deserialize + PartialEq + 'static from the following types:

  • Rc<T> where T: Serialize + Deserialize + PartialEq + 'static
  • T: Serialize + Deserialize + Clone + PartialEq + 'static

Which needs specialisation.

Copy link
Member

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which needs specialisation.

That sucks. This looks good to me in the current state. Seems like exposing Rc<D> is the best we can do

@futursolo futursolo merged commit 9d94f24 into yewstack:master Jul 19, 2022
@futursolo futursolo deleted the prepared-rc branch December 15, 2022 10:18
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 A-yew-macro Area: The yew-macro crate breaking change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants