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

Refactor and simplify Callback #2269

Closed
hamza1311 opened this issue Dec 13, 2021 · 8 comments · Fixed by #2301
Closed

Refactor and simplify Callback #2269

hamza1311 opened this issue Dec 13, 2021 · 8 comments · Fixed by #2301
Assignees
Labels
A-yew Area: The main yew crate breaking change
Milestone

Comments

@hamza1311
Copy link
Member

hamza1311 commented Dec 13, 2021

yew::Callback should be refactored to make the following changes:

Remove CallbackOnce:

  • There are only a handful of usages it has in the yew repo.
  • I can't think of a situation where it would be needed.
  • Using it has some overhead:
Rc<RefCell<Option<Box<dyn FnOnce(IN)>>>>

is a pretty complex type with a many indirections. We should remove it altogether

Move the passive option somewhere else

passive is event listener specific, not Callback specific. It should be a set as part of listener API, not the Callback one. passive in Callback increases it's complexity

Replace Rc<_> with Box<_>/Remove Clone implementation

Rc<dyn Fn(IN)> will become Box<dyn Fn(IN)>. With #1961, there is no need for props to be Clone so callback doesn't need to be Clone either. We could save the overhead of reference counting by using Box here instead.

Update: it seems this is a bad idea

Get rid of reform

It's a helper which isn't used much (4 usages in the entire yew repo). #2196 has discussion about it that ended with it's existence not really mattering

Callbacks should also return values

By default, the return value can just be () but there should be a way to specify it.

Potential future changes

Use static dispatch instead of dynamic dispatch for Fn:
type_alias_impl_trait feature (rust-lang/rust#63063) would allow it.

#![feature(type_alias_impl_trait)]

type Function = impl Fn();
struct Callback(Function)
// use Callback
// ...
@hamza1311 hamza1311 added breaking change A-yew Area: The main yew crate labels Dec 13, 2021
@hamza1311 hamza1311 self-assigned this Dec 13, 2021
@hamza1311
Copy link
Member Author

If there's no objections, I will PR this change (hopefully) soon.

@hamza1311 hamza1311 added this to the v0.20 milestone Dec 13, 2021
@intendednull
Copy link
Contributor

A callback that can only be executed once might be useful for situations where you'd like to limit the user from repeating an action (i.e. payment submission). Though panicking is not terribly helpful here.

That being said I don't see a use case where it is the only solution.

@hamza1311
Copy link
Member Author

A callback that can only be executed once might be useful for situations where you'd like to limit the user from repeating an action (i.e. payment submission). Though panicking is not terribly helpful here.

Right. Any such situation can be dealt by the user on a per-case bases. In current implementation, I feel like it also adds too much of a runtime to justify using

@rjmac
Copy link
Contributor

rjmac commented Dec 14, 2021

Making Callback non-Clone would remove the ability to pass a callback that a component is given in its props directly down into a sub-component without creating an intermediate callback. This is a pattern that occurs reasonably frequently in my codebase. I could work around it, but it'd be a little annoying.

@hamza1311
Copy link
Member Author

@rjmac, you're right. That would be incontinence and could read to Rc<Box<dyn Fn()>> type being passed around. Keeping it Clone seems like a better option. Can you explain the workaround you're thinking of? It might be possible to implement Clone with that without reference counting.

@rjmac
Copy link
Contributor

rjmac commented Dec 14, 2021

Don't even need the Box then, Rc can contain unsized values, so Rc<dyn Fn()> is a perfectly good type and has one fewer layer of indirection.

The workaround I was thinking of is a bit ugly, instead of passing down the callback you got in your props you pass down a fresh callback that sends you a message to emit the callback from your props.

@voidpumpkin
Copy link
Member

A callback that can only be executed once might be useful for situations where you'd like to limit the user from repeating an action (i.e. payment submission). Though panicking is not terribly helpful here.

That being said I don't see a use case where it is the only solution.

@intendednull This is usually called debouncing. And using FnOnce is not the way to implement it as FnOnce panics if you call it a second time.

@voidpumpkin
Copy link
Member

@hamza1311 I think Callback could also return values
I already have 10+ cases in my work codebase where I use a very primitive copy of Callback just for that.

Main case is when im passing half-filled route enums inside a callback so that when on event i have the missing data i just call the callback.

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 breaking change
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants