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

Suspend should accept IntoFuture instead of Future #3509

Closed
iradicek opened this issue Jan 21, 2025 · 4 comments · Fixed by #3553 or #3555
Closed

Suspend should accept IntoFuture instead of Future #3509

iradicek opened this issue Jan 21, 2025 · 4 comments · Fixed by #3553 or #3555

Comments

@iradicek
Copy link
Contributor

Consider the following:

let res = Resource::new(..., ...);
// let s = Suspend:new(res); // Does not work because `res` is not `Future`
let s = Suspend::new(async move { res.await }); // Works because for await `IntoFuture` is sufficient

Is there any reason for Suspend to not accept impl IntoFuture?

@iradicek iradicek changed the title Suspend should accept IntoFuture instead of Future Suspend should accept IntoFuture instead of Future Jan 21, 2025
@gbj
Copy link
Collaborator

gbj commented Jan 21, 2025

You're probably right. I think it probably wouldn't even be a breaking change, technically, as there is a blanket implementation of IntoFuture for every F: Future.

n.b. Suspend::new(res.into_future()) works here too for now, of course, and is shorter.

@iradicek
Copy link
Contributor Author

You're right, Suspend::new(res.into_future()) does work.

Yes, anything that takes Future, should work with IntoFuture.

@gbj
Copy link
Collaborator

gbj commented Jan 22, 2025

Oh I should add, for the specifics of the example -- Resource itself also implements the rendering traits by doing move || Suspend::new(...), so just as you can use a signal directly in the view, you can do the same with a resource, without explicitly wrapping it in Suspend at all. (For most resources, you want to do some kind of transformation on the data which is why the async move { let res = res.await; /* do something with res */ } syntax is useful)

But point taken, you are correct that the trait bound should probably be IntoFuture.

@gbj
Copy link
Collaborator

gbj commented Feb 1, 2025

Done in #3532 as part of 0.8, as it does end up with enough of a change in generic bounds that it's technically breaking.

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