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

Add useful methods to UseStateHandle & UseReducerHandle #3422

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

its-the-shrimp
Copy link
Contributor

@its-the-shrimp its-the-shrimp commented Sep 28, 2023

Description

This PR adds:

  • UseStateHandle::get & UseReducerHandle::get that return the contained value of the handle in the form in which it's stored internally: in an Rc; These 2 methods will make it easier to work with non(-trivially)-clonable types in said containers;
  • UseStateHandle::into_inner & UseReducerHandle::into_inner that consume the handle and return its 2 main parts: the inner value and the setter/dispatcher associated with the handle. These methods will allow for avoiding cloning in certain contexts where one would have to use get() + setter()/dispatcher() otherwise.

Checklist

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

@github-actions
Copy link

github-actions bot commented Sep 28, 2023

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

https://yew-rs-api--pr3422-add-handle-methods-dx5xc4eu.web.app

(expires Thu, 05 Oct 2023 20:54:58 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Sep 28, 2023

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 360.013 360.211 360.098 0.074
Hello World 10 687.706 696.796 691.000 2.394
Function Router 10 2172.100 2202.425 2185.140 8.032
Concurrent Task 10 1006.855 1008.117 1007.461 0.391
Many Providers 10 1576.487 1608.791 1590.125 11.505

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 359.987 360.105 360.055 0.035
Hello World 10 691.831 697.689 694.551 1.934
Function Router 10 2210.087 2235.343 2227.472 7.332
Concurrent Task 10 1006.772 1007.782 1007.408 0.401
Many Providers 10 1593.305 1638.931 1606.330 13.944

@github-actions
Copy link

github-actions bot commented Sep 28, 2023

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 102.886 102.886 0 0.000%
boids 175.667 175.667 0 0.000%
communication_child_to_parent 95.304 95.304 0 0.000%
communication_grandchild_with_grandparent 109.031 109.031 0 0.000%
communication_grandparent_to_grandchild 105.724 105.724 0 0.000%
communication_parent_to_child 92.786 92.786 0 0.000%
contexts 113.452 113.452 0 0.000%
counter 89.195 89.195 0 0.000%
counter_functional 89.932 89.932 0 0.000%
dyn_create_destroy_apps 92.369 92.369 0 0.000%
file_upload 103.515 103.515 0 0.000%
function_memory_game 174.644 174.642 -0.002 -0.001%
function_router 352.274 352.280 +0.006 +0.002%
function_todomvc 163.509 163.509 0 0.000%
futures 227.443 227.443 0 0.000%
game_of_life 112.218 112.218 0 0.000%
immutable 188.789 188.789 0 0.000%
inner_html 85.979 85.979 0 0.000%
js_callback 113.463 113.467 +0.004 +0.003%
keyed_list 201.207 201.207 0 0.000%
mount_point 89.186 89.186 0 0.000%
nested_list 114.614 114.614 0 0.000%
node_refs 96.289 96.289 0 0.000%
password_strength 1721.318 1721.318 0 0.000%
portals 98.365 98.365 0 0.000%
router 318.267 318.271 +0.004 +0.001%
simple_ssr 144.242 144.240 -0.002 -0.001%
ssr_router 390.032 390.034 +0.002 +0.001%
suspense 119.012 119.012 0 0.000%
timer 91.795 91.795 0 0.000%
timer_functional 100.472 100.476 +0.004 +0.004%
todomvc 143.688 143.688 0 0.000%
two_apps 89.896 89.896 0 0.000%
web_worker_fib 138.877 138.875 -0.002 -0.001%
web_worker_prime 190.394 190.399 +0.006 +0.003%
webgl 88.551 88.551 0 0.000%

✅ None of the examples has changed their size significantly.

/// Returns the inner value of the handle.
pub fn get(&self) -> Rc<T> {
// Safety: `UseStateReducer<T>` is `repr(transparent)` and only contains `T`
unsafe { transmute(self.inner.get()) }
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid the transmute somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not really, this transmute converts between Rc<UseStateReducer<T>> and Rc<T>, there's not much you can do with a value inside an Rc

Copy link
Contributor

@kirillsemyonkin kirillsemyonkin Sep 29, 2023

Choose a reason for hiding this comment

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

You would need to convert Rc<UseStateReducer<T>> -> Rc<T> (assuming T is not guaranteed to be Clone) some other way then (maybe via Any?). I think using repr(transparent) was a brilliant idea for Rcs and now I hope Rust makes it like a safe trait / Rc function.

Copy link
Member

@futursolo futursolo left a comment

Choose a reason for hiding this comment

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

I am not in favour of this pull request.

We have discussed in some previous pull requests. How a state is stored should be an implementation detail. Not exposing this allows us to switch the underlying implementation at a later time.

e.g.:
To implement concurrent mode, we will need to have multiple schedulers and states need to be associated with a scheduler ID .
I am not exactly settled on how this would be associated.
This pull request rules out the following possibility.

struct UseStateReducer<T> {
    inner: T,
    scheduler_id: Id,
}

@its-the-shrimp
Copy link
Contributor Author

The PR does restrict the flexibility in implementation, but on the other hand, how likely is the implementation to change? The current approach works just fine, obtaining/setting the value doesn't require much indirection, and the new methods will allow for easier usage of non(-trivially)-clonable types in the aforementioned handle types.

@kirillsemyonkin
Copy link
Contributor

kirillsemyonkin commented Sep 29, 2023

@futursolo Provide other solutions of being able to encapsulate/not clone setter/dispatcher in Yew then.

  1. This PR is best case, since it being tied to implementation helps users to have an easy optimization path and easy access to the current value in a familiar Clone wrapper (which is being used for Reducible by the way).

  2. Medium case is introducing a custom wrapper type UseStateValue<T> instead of giving the Rc<T> to the user. This type may contain your wanted scheduler_id, should be able to be Clone as UseStateHandle is, while not forcing clones of setter/dispatcher upon the users, as well as allowing the developer to not give power of changing parent's state to children.

#[derive(Clone, ...)] // also ImplicitClone
struct UseStateValue<T> {
    inner: Rc<T>, // using same `repr(transparent)`/`transmute` trick
}

impl Deref for UseStateValue<T> {
    type Target = T;
    ...
}
  1. Worst case is adding custom wrapper type UseStateValue<T> that simply wraps UseStateHandle<T>. This type will not contain .set, but it will still be including setter/dispatcher via UseStateHandle<T>. It will also not be allowed in functions as an argument where Rc<T> parameter is required (functions trying to support this will need to have a impl Deref<Target = T> parameter instead) (medium case also suffers from this).

  2. Even worst-er case is not doing anything and making users implement the worst case themselves.

Also possible that use_state will change so much in future that it might not even allow to get a value out of it as easily as Deref (e.g. callback access only). In such case this PR is quite benevolent, since these changes will vanish at that point as well.

@ranile
Copy link
Member

ranile commented Sep 30, 2023

The medium case isn't good because oftentimes the value needs to be cloned after getting it out of the state, forcing T to be Clone when it can be avoided. It's either that or recreating the Rc that we just took the value out of.

Yew playground has a component that suffers from this exact same thing today when attaching the change listener to Monaco editor


Concurrent mode, when implemented, will most likely come with breaking changes of its own. I'd rather remove these methods then, providing a suitable alternative, than not add them in the first place

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants