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

Simplify Command Matching #1157

Closed
parker-codes opened this issue Jan 27, 2021 · 33 comments
Closed

Simplify Command Matching #1157

parker-codes opened this issue Jan 27, 2021 · 33 comments

Comments

@parker-codes
Copy link

parker-codes commented Jan 27, 2021

The Problem

The main.rs file gets pretty bulky. Yes, things can be split apart into simpler chunks, and that's what I'm proposing - I just think Tauri can assist with some "sugar".
This probably isn't a priority. I just think of how much simpler it could be, especially for beginners to Rust.

Specifically I'd like to point out:

  • deserializing the command from a string
  • matching commands inline
  • move ||
  • destructuring and passing along Tauri-specific values like the callback and error fields

image
Link to code

Preferred Solution

Describe the solution you'd like
A clear and concise description of what you want to happen.

I see a few options:

  1. Create a macro that converts all variant matches into methods of the same name, like GetAllTodos into get_all_todos(). The problem then becomes passing the value into the method. If there are multiple fields in the variant, would they be passed in order? That doesn't seem right. It would be cleanest to pass the command itself, but enum variants aren't real types, so the value can't be destructured and passed in. Here's a playground example of how the command can be passed, but it still needs to be matched on the other end, which detracts from the desired cleanliness.
  2. Deserialize the command into a tuple variant. I think the syntax is quite ugly and confusing, but all of the RFCs have been closed or slow moving. See Types for enum variants rust-lang/rfcs#1450 and Enum variant types rust-lang/rfcs#2593. Also see this Stack Overflow post for clarification on why this is ugly. Serde has some great features that may be able to deserialize this, like flatten. This way would probably be pretty simple.

Both of these together could look like:

#[derive(Deserialize)]
struct GetAllTodos {
    callback: String,
    error: String,
}

#[derive(Deserialize)]
struct CreateTodo {
    title: String,
    callback: String,
    error: String,
}

// some type of flatten?
#[derive(Deserialize)]
#[serde(tag = "cmd", rename_all = "camelCase", flatten)]
pub enum Cmd {
    GetAllTodos(GetAllTodos),

    CreateTodo(CreateTodo),

    // .. more
}

And then potentially an attribute or macro to generate this, if desired:

match command {
    Cmd::GetAllTodos(cmd) => get_all_todos(cmd),
    Cmd::CreateTodo(cmd) => create_todo(cmd),
}

And the usage could look like:

fn get_all_todos(cmd: GetAllTodos) {
    // stuff here
}

Another thing to think about here would be handling responses and return types like via tauri::execute_promise(). Is there any way for these methods and params to be abstracted or hidden as well (like success and error callbacks)? Perhaps the commands or functions could have an attribute like #[tauri(type = "execute_promise")] which acts like a decorator?

Conclusion

This is all to hopefully help simplify how people use it - not necessarily code cleanliness. If there is a "cleaner" structure that can be done without any API changes, perhaps it's worth showcasing that in the examples instead of the very straightforward examples that are there currently.

@parker-codes
Copy link
Author

Here's a potential implementation moving the match into the Cmd enum: Playground Link

@nothingismagick
Copy link
Member

This is a great idea, and would love to see this mainline before beta release. @lucasfernog - what do you think?

@parker-codes
Copy link
Author

Question: What are the use cases for accessing the webview? Perhaps that could be passed into the command execution function - maybe even as dependency injection?

Because arg could be abstracted and potentially webview, that makes me think that people could utilize this new API by using something similar to invoke_handler like invoke_enum or bind_commands that takes the enum as a parameter.

@parker-codes
Copy link
Author

I had another idea. I'll keep spitballing them here until something sounds good. 😄

The machine crate has a great API that I think could be copied here for some great benefit.

image

image

So essentially the API could be expressed how it currently is, but with a macro scoping it. Each of the structs generated would need to implement some Command trait (just a function) which is where the user would put what they normally put in the big match statement.

The generated match statement could potentially wrap the execute_promise/etc callbacks so the user only has to specify them via an attribute in the enum definition or something.

@lucasfernog
Copy link
Member

That sounds perfect! Nice finding @parker-codes you have no idea how excited I was to read that.

@lucasfernog
Copy link
Member

What if we do the reverse operation of the machine crate? The user writes the structs and the macro generates the enum with the pattern matching to call the structs Command impl

@parker-codes
Copy link
Author

That would certainly give the users more control, and it probably wouldn't be as confusing/magical.

It would also allow for the API to be extended in the future to more than just a Command impl. Not sure what that could be, but it would be more straightforward.

@parker-codes
Copy link
Author

parker-codes commented Feb 4, 2021

So the changes I'm seeing:

  • a trait that defines a command set (an enum with the method that handles matching each variant)
  • a trait that defines a required method on a command struct, like execute(&self)
  • a declarative macro to wrap the struct definitions, which produces an enum with variants for each as well as a method that matches each command (plus an error/NoMatch variant?). They will also need a way to specify the name of the new command group/enum for clarity and perhaps because they could have multiple sets?
    - [ ] a proc macro (as an attribute) to optionally decorate a particular command's execute() method to hide the various API around execute_promise, etc
  • a new API method that takes an enum that impls the set trait and calls the method to match when a new event is passed through. This could also house the logic for deserializing so that's hidden as well

A POC could be made without any API changes because this could all technically be done in userland. The macro expansion could also be done by hand until the final API is decided on.

@lucasfernog
Copy link
Member

@parker-codes I think you're on the right track with these changes. Do you think you can work on it? This would be amazing for Tauri. I can also see stuff like these extended to the events API, which would simplify stuff like autocomplete event names on JS side.

@parker-codes
Copy link
Author

@lucasfernog Yes, of course!

@parker-codes
Copy link
Author

I'll summarize our Discord discussion here.

We discussed making all communication async. The callback and error flags could be handled by the core layer, simplifying another thing for the end user!

We also talked about having the macro add another property to commands to house the webview so the execute function only needs to have access to &self. To avoid conflicting with user variable names, we could make this property tauri_webview and then access it with a self.webview() method.

@parker-codes
Copy link
Author

@lucasfernog @chippers

If I'm understanding the code properly, context could be a second param, merging the two ideas we've been discussing:

  1. &self would be only the command values, with no overlap/conflict with user-defined properties
  2. the next param, context, could house the webview and any other app state they've set up.

This reminds me of the state pattern that Vuex (with VueJS) uses, as well as some Rust web and game frameworks.

If I misunderstood, I apologize. 😅 This is what I got out of it, which would be pretty cool since people will start adding some "global" stores.

@lucasfernog
Copy link
Member

Yeah you're right. I think we can also make the command interface take self instead of &self.

@parker-codes
Copy link
Author

Where would context come from? Wouldn't Tauri need a way to attach this global state? Perhaps it already does?

@lucasfernog
Copy link
Member

I think it could be similar to what boscop/web-view does: https://github.com/Boscop/web-view/blob/abb0721829d9764d07d8df08c249de9b27cd7447/src/lib.rs#L236

@parker-codes
Copy link
Author

I finally had some time to get a rough outline of how I envision it, but the code is broken because I'm not that great with type hinting.

https://github.com/parker-codes/tauri-commands-api-poc

  1. I was picturing a way to add multiple command sets, like modules, but I guess just one would work fine?
  2. Since serde_json uses the type as a hint of what to deserialize to, I was trying to find a way to cleanly allow the user to specify the type without instantiating it (via turbofish). This may not be idiomatic though, and would also not allow for adding instances to a vec for multiple command sets.
  3. I tried to think of a way to show proper impl errors and ended up just creating a wrapper function to force it, but there's probably a better way to do this as well.

Hopefully I can get back into fixing this, but it may not be at the pace we're all hoping for.

@lucasfernog
Copy link
Member

@parker-codes
Copy link
Author

@lucasfernog Thanks! It works great! I added a little more to test utilizing context. Now I can get started on the macro.

I know it's super basic and isn't async, but that wasn't the focus. Neither was naming. I'd like them to be a little more cohesive.

Do you think multiple command sets / modules would be beneficial? That could also be overkill.. If we don't need to worry about multiple, we probably don't even need to specify the type in each command, I'd think. We'd need to somehow pull that type into the generated CommandSet impl.

@lucasfernog
Copy link
Member

I think it'll complicate things too much. The user can probably figure something out if they want something like that.

@parker-codes
Copy link
Author

In that case, the generated enum could have a specific name and we probably wouldn't need to pass along types as much..

I can actually remove cmd_set_handler and PhantomData entirely, but this API should be optional, so I'll have to play around with removing stuff.

@parker-codes
Copy link
Author

I'm still trying to think of a way to opt into the CommandSet API. With this POC so far it's required. Same with user data.

This isn't working, but is more user-friendly. I wouldn't want people to have to unwrap, but maybe it's simpler to just keep the Option?

image

It would be great if we could have this trait have a default so it is optional.

@lucasfernog
Copy link
Member

tbh I think it's fine if we enforce this pattern.

@chippers
Copy link
Member

chippers commented Feb 20, 2021

It should be possible to strongly type the "Send" and "Receive" types for the user exposed API. For example in my program I had an enum Action to handle TypeScript => Rust and an enum Message for Rust => TypeScript. On the Rust side this allowed me to strongly type both incoming and outgoing events. On the frontend I had a dependency generating TypeScript definitions of the bindings crate. I had the entire Tauri Event pipeline strongly typed.

// bindings/src/lib.rs

#[cfg_attr(feature = "typescript", derive(typescript_definitions::TypeScriptify))]
#[derive(Debug, Clone, Deserialize)]
#[serde(rename_all = "camelCase", tag = "tag", content = "action")]
pub enum Action {
    Status(StatusAction),
    Settings(SettingsAction),
}

#[cfg_attr(feature = "typescript", derive(typescript_definitions::TypeScriptify))]
#[derive(Debug, Clone, Serialize)]
#[serde(rename_all = "camelCase", tag = "tag", content = "message")]
pub enum Message<'a> {
    Status(StatusMessage<'a>),
    Settings(SettingsMessage<'a>),
}
// bindings/src/status.rs

#[cfg_attr(feature = "typescript", derive(typescript_definitions::TypeScriptify))]
#[derive(Debug, Clone, Deserialize)]
#[serde(rename_all = "camelCase")]
pub enum StatusAction {
    FetchStore,
}

#[cfg_attr(feature = "typescript", derive(typescript_definitions::TypeScriptify))]
#[derive(Debug, Clone, Serialize)]
#[serde(rename_all = "camelCase", tag = "type", content = "data")]
pub enum StatusMessage<'a> {
    Store(&'a StatusStore),
}

#[cfg_attr(feature = "typescript", derive(typescript_definitions::TypeScriptify))]
#[derive(Default, Debug, Clone, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct StatusStore {
    pub program: Option<Result<bool, String>>,
    pub existing: Option<Result<bool, String>>,
    pub structure: Option<Result<bool, String>>,
}
// src-tauri/src/events/status.rs

pub fn handle(ctx: Context, action: StatusAction) -> Result<(), Error> {
    use StatusAction::*;

    match action {
        FetchStore => fetch_store(ctx),
    }
}

fn fetch_store(ctx: Context<'_>) -> Result<(), Error> {
    let Context { state, webview, .. } = ctx;
    let state = state.read().map_err(|_| Error::State)?;
    Ok(emit(
        webview,
        Message::Status(StatusMessage::Store(&state.status)),
    )?)
}
// src-tauri/src/events.rs

#[tokio::main]
pub(crate) async fn handler(mut w: WebviewMut, receiver: Receiver<Action>) {
    let state = State::default();
    check(w.clone(), state.clone());

    // `crate::setup` which sets up a thread with this function and loop can be
    // called multiple times. This can result in the sender that was captured in the `FnMut` to be
    // dropped, which will close the channel. The last created channel + thread will still be alive.
    // That is why this `recv()` loop should **ALWAYS** handle closing the channel and (probably)
    // close the thread.
    loop {
        let ctx = Context::new(&mut w, state.clone());
        let res = match receiver.recv() {
            Ok(Action::Status(action)) => status::handle(ctx, action),
            Ok(Action::Settings(action)) => settings::handle(ctx, action),
            Err(_) => break,
        };

        if let Err(e) = res {
            eprintln!("error received in event loop: {:#?}", e);
        }
    }
}

This could probably be handled with two associated types on the CommandSet trait, but I haven't looked into it

edit: it's hard to see Message being used in that example, but the code handling events eventually calls

pub(crate) fn emit(webview: &mut WebviewMut, payload: Message<'_>) -> Result<(), EventError> {
    tauri::event::emit(webview, "", Some(payload)).map_err(|_| EventError::Emit)
}

@parker-codes
Copy link
Author

So we'd just have to have the template(s) define some basic/blank state and commands for people who don't want to touch Rust at all. So that means the POC works so far, I think.

How do you feel about using a different naming convention then boscop/webview? I want to play around with the names like user_data.

@lucasfernog
Copy link
Member

Absolutely :) go ahead and do whatever you want with this

@parker-codes
Copy link
Author

@chippers I haven't looked at your code yet, but I will soon!

@parker-codes
Copy link
Author

It's hard to find time for this and I don't want to slow the team down. 😞 Does anyone else want to pick this up?

@nklayman
Copy link
Member

We'll see... I'm trying to focus on scholarship applications but coding is just too darn fun 😂. Chances are I'll sneak in some time to work on this over the next week or so as I've been wanting to build up my macro experience. Thanks for the work you've done so far!

@parker-codes
Copy link
Author

I'll still help out where I can, so l won't just be disappearing, haha. Hopefully the chain above is detailed enough.

@lucasfernog
Copy link
Member

Maybe @chippers could also help Noah.. but I guess he's busy right now.

@chippers
Copy link
Member

Maybe @chippers could also help Noah.. but I guess he's busy right now.

yeah i have limited free time right now, so ill be focusing on the build stuff first, afterwords i can look at this

@lucasfernog
Copy link
Member

Thanks @chippers that's perfect :)

I can help Noah for now if he picks this task, this will be fun.

@nklayman
Copy link
Member

nklayman commented Feb 26, 2021

#1301 👀

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

No branches or pull requests

5 participants