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

Let Mutable::set take Into<A> #74

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Billy-Sheppard
Copy link

Hi Pauan,

This PR allows the set method to take Into<A> rather than A. Would you be open to merging this? Is there a (possibly very good) reason not to? Very open to discussion/improvement.

@Pauan
Copy link
Owner

Pauan commented Oct 10, 2023

What's your use case? The other mutable containers (like Cell) do not accept Into<T>.

Accepting Into<T> would bloat up the file size, because monomorphization will create a new method for each type.

It's not a big deal to just use mutable.set(foo.into()), which doesn't have any monomorphization costs.

@Billy-Sheppard
Copy link
Author

Monomorphization is a good point and a very fair reason to reject this PR.

The use case for me is I find I am often using Mutable<Rc<T>> in my code, which often leads to many into calls.

@Pauan
Copy link
Owner

Pauan commented Oct 10, 2023

Could you explain more about how you're doing the wrapping?

If you're using dominator, then the standard style is for components to have a fn new() -> Arc<Self> function, so you don't need to call .into() on that.

So the only time you need to manually call .into() is with Mutable<Arc<str>>

@Billy-Sheppard
Copy link
Author

As an example I have a Model for a page which contains fields of Mutables. If the type is non-copy then I wrap the T in an Rc.

struct Model {
    copy_type: Mutable<T>,
    non_copy_type: Mutable<Rc<T>>,
    ..
}

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.

None yet

2 participants