Skip to content

Conversation

@rklaehn
Copy link
Collaborator

@rklaehn rklaehn commented Apr 17, 2025

This requires adding a few more bounds, but it is all bounds you will need anyway in a different place, so I don't think it is bad.

I have a few places in the new blobs where I need these futures to be static, and the .request() fn already returns a static future, so it has no runtime cost.

Q: I was thinking about having a LocalRpcMessage requirements trait so we don't have to repeat Debug + Send + Sync + Unpin + 'static everywhere and can take away some of the bounds in a single place based on a feature flag. WDYT?

@rklaehn rklaehn requested a review from Frando April 17, 2025 12:45
) -> impl Future<Output = Result<Res, Error>> + Send + 'static
where
S: Service,
M: From<WithChannels<Req, S>> + Send + Sync + Unpin + 'static,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

R could have the bound RpcMessage, and M could have the bound LocalRpcMessage to DRY the bounds. Not sure if this is worth it. I guess it depends a bit on if we need to take away some of the bounds for WASM. @matheus23 WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I have a really hard time reading this type signature.

In general, if you're wondering if it's okay to have a T: Send bound in Wasm, then you simply need to answer this question: "Will T contain/refer to a JsValue?"
Such a value could be a JsPromise if it just made a call to sth async (this happens a lot when you have async APIs and is what makes rust futures in async non-Send in practice a lot), or it could be because you're storing a js Error value in there, which is the other common thing.

I'd very hesitantly say you're good in this case to just have a Send bound. But again, I have no idea what's going on in this type signature, it's quite complex.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will T contain/refer to a JsValue?

So if there is a possibility that T contains a JsValue, it must not be Send? I think it is very unlikely that there will be a JsValue in these things.

@rklaehn rklaehn requested a review from matheus23 April 17, 2025 12:55
@rklaehn rklaehn force-pushed the static-sugar-futures branch from d234d3e to 1168865 Compare April 18, 2025 14:18
@rklaehn rklaehn merged commit 9db7372 into main Apr 18, 2025
15 checks passed
@rklaehn rklaehn deleted the static-sugar-futures branch April 18, 2025 14:26
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.

3 participants