refactor(protocol): respond with a function call instead of return value #1017
Conversation
There was a problem hiding this comment.
LGTM! but I am wondering if we can keep the same DX as before, e.g. allow either responding early or just return the response from the closure, for example:
.with_custom_protocol("wry".into(), move |request, api| {
// do work
api.respond(response);
// do morework
})and
.with_custom_protocol("wry".into(), move |request, api| {
// do work
Ok(response)
})
|
That would be more confusing.. you can only respond once, so if we wanna keep the return value, we'd need to make it an Option where None means you're using the api parameter (yikes). |
|
Right, that would be confusing, how about two separate APIs then? maybe |
|
Good idea! Let me know what you think. |
src/webview/mod.rs
Outdated
| @@ -135,7 +153,7 @@ pub struct WebViewAttributes { | |||
| /// [bug]: https://bugs.webkit.org/show_bug.cgi?id=229034 | |||
| pub custom_protocols: Vec<( | |||
| String, | |||
| Box<dyn Fn(&Request<Vec<u8>>) -> Result<Response<Cow<'static, [u8]>>>>, | |||
| Box<dyn Fn(Request<Vec<u8>>, Response) -> Result<()>>, | |||
There was a problem hiding this comment.
It doesn't make sense to return Result here if you want to be async. Response should be the one to determine the Result.
There was a problem hiding this comment.
I mean the implementation should also handle this.
If it's responder dealing with it, it should decide which finish method should be called.
There was a problem hiding this comment.
We don't necessarily need to add .respond_with_err() variant, since you can just create a response with status code between 400-599 and you should be fine. However, I do agree that this function shouldn't return a result to allow the responder to respond even if this function errored and the responder were sent somewhere else.
There was a problem hiding this comment.
yeah at least it's less confusing for the user where you can call respond() and return an error later and it would crash the app.
There was a problem hiding this comment.
about manually creating error responses: that's what I'm trying to convince @wusyong :D
src/webview/mod.rs
Outdated
| @@ -135,7 +153,7 @@ pub struct WebViewAttributes { | |||
| /// [bug]: https://bugs.webkit.org/show_bug.cgi?id=229034 | |||
| pub custom_protocols: Vec<( | |||
| String, | |||
| Box<dyn Fn(&Request<Vec<u8>>) -> Result<Response<Cow<'static, [u8]>>>>, | |||
| Box<dyn Fn(Request<Vec<u8>>, RequestAsyncResponder) -> Result<()>>, | |||
There was a problem hiding this comment.
| Box<dyn Fn(Request<Vec<u8>>, RequestAsyncResponder) -> Result<()>>, | |
| Box<dyn Fn(Request<Vec<u8>>, RequestAsyncResponder)>, |
Since the decision is removing Result, let's change it and remove the underlying function it calls.
There was a problem hiding this comment.
we need to add custom_protocols_async field here and keep the existing one as is to avoid a breaking change.
There was a problem hiding this comment.
let me know if it looks good now @wusyong
…lue (#1017) Co-authored-by: Amr Bashir <amr.bashir2015@gmail.com>
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
Checklist
Other information
This enhances the custom protocol handler to allow working asynchronously instead of freezing the main thread for longer tasks. I wanted to do this for a long time but now with Tauri using custom protocols for IPC, we actually need this (my biggest issue right now is using the camera on mobile, it was freezing the app).