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

[feat] Make AsyncCommandMustReturnResult trait public so it be implemented on custom "result" types. #9797

Closed
dam5h opened this issue May 16, 2024 · 5 comments

Comments

@dam5h
Copy link

dam5h commented May 16, 2024

Describe the problem

We use a custom Result type for my endpoints, so that we have full control of the handling behavior and type information of the Err variant on the typescript side.

We have not been using the tauri Manager functionality to manage state and I wanted to change that and give it a try (instead we have been using OnceCells). As soon as I injected the tauri::State<'_, MyState> annotation in a command signature, I started getting compiler errors that I must return a Result from async commands. This makes sense, but I don't want to return vanilla Result, but rather my own CommandResult due to the FE handling of Result types.

Here is where the private trait is implemented.

trait AsyncCommandMustReturnResult {}

Describe the solution you'd like

I would like to be able to impl AsyncCommandMustReturnResult trait myself for my custom result type. At least by having this trait restriction you know that folks will be opting in for good reason and can be trusted to understand why this is the case.

Alternatives considered

I discussed returning result to the FE folks and it seems there is some type erasure happening in that case and we have less control over the error handling on the TS side. It would also be a lot of work in this case as the FE is quite large.

Additional context

No response

@dam5h dam5h changed the title [feat] Make AsyncCommandMustResultResult trait public so it be implemented on custom "result" types. [feat] Make AsyncCommandMustReturnResult trait public so it be implemented on custom "result" types. May 16, 2024
@amrbashir
Copy link
Member

amrbashir commented May 16, 2024

AsyncCommandMustReturnResult only exists to ensure that the async command returns an std::result::Result if it has argument with references. Generally you don't need to implement this trait at all.

What does your custom Result type look like? why do you need a custom Result type in the first place? it is more like you need a custom Error type which you can do.

@dam5h
Copy link
Author

dam5h commented May 16, 2024

Thanks for the reply. My custom Result type is this:

use serde::{Deserialize, Serialize};

// A custom enum for returning results to the frontend. We do not use Result because Err values
// result in an error being thrown in Javascript, which is not what we want.
#[derive(Serialize, Deserialize)] // GUI
#[derive(Debug)]
#[derive(specta::Type)]
pub enum CommandResult<T, E> {
    Ok(T),
    Err(E),
}

impl<T, E> CommandResult<T, E> {
    pub fn is_ok(&self) -> bool {
        match self {
            CommandResult::Ok(_) => true,
            CommandResult::Err(_) => false,
        }
    }

    pub fn is_err(&self) -> bool {
        !self.is_ok()
    }
}

impl<T, E> From<Result<T, E>> for CommandResult<T, E> {
    fn from(value: Result<T, E>) -> Self {
        match value {
            Result::Ok(x) => CommandResult::Ok(x),
            Result::Err(x) => CommandResult::Err(x),
        }
    }
}

I am far more of a rust person than a JS / TS person so I may ask another team member to comment on the need of CustomResult. My understanding was that we did not want to have exceptions thrown in TS, but rather handle the errors ourselves. Also it sounded like the exception throwing was going to do some kind of type erasure on the error type and we would lose some type information in the conversion to exception process. I will gather some more insight though, appreciate the response.

@dam5h
Copy link
Author

dam5h commented May 17, 2024

As a follow up, here is an article referencing some of the issues the team wanted to avoid.

https://kentcdodds.com/blog/get-a-catch-block-error-message-with-typescript

I am going to experiment some today with using good ole Result in place of CommandResult to see if anything unsound happens or if we were worried about a non-existent problem.

@dam5h
Copy link
Author

dam5h commented May 18, 2024

Hi @amrbashir, to illustrate the issue, see below:

When using tauri-specta to generate our TS types, Result<(), E> gets translated to null on the

export function myEndpoint(...) {
    return invoke()<null>("myEndpoint", {...})
}

vs

export function myEndpoint(...) {
    return invoke()<CommandResult<null, E>>("myEndpoint", {...})
}

So it seems specta is throwing away the E part of Result. Are we doing something wrong?

@amrbashir
Copy link
Member

amrbashir commented May 20, 2024

I understand that the core issue is you're trying to avoid throwing errors on JS which happens when invoke either hits an internal error in tauri or your command returns an Err when using Result<T, Err> on Rust, so instead of creating your own custom result type, you can just wrap your Result inside another Result

use serde::Serialize;

#[derive(Debug, Serialize)]
enum CustomError {}
impl std::fmt::Display for CustomError {}
impl std::error::Error for CustomError {}

async fn greet_inner(name: &str) -> Result<String, CustomError> {
	// here you can propagate errors normally using ? 
    Ok(format!("Hello, {}! You've been greeted from Rust!", name))
}

#[tauri::command]
async fn greet(name: &str) -> Result<Result<String, CustomError>, ()> {
    // wrap the returned `Result` into an `Ok`, making the invoke
    // call to only throw on internal tauri errors and not our errors
    // so we can handle errors gracefully in JS without try/catch
    Ok(greet_inner(name).await)
}

I know this is a bit ugly but it is way better than exposing AsyncCommandMustReturnResult which is actually generated by tauri::command macro on the fly.

If you want to make this a bit better you could try writing a macro that generates these wrapper/inner functions.

@amrbashir amrbashir closed this as not planned Won't fix, can't repro, duplicate, stale May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants