Conn methods + feedback on conn macros

[mathiversen]

• The conn.path() method only return the relative path of the current connection. I was thinking that maybe the path should return the full path, and maybe have a separate method for the relative path?
• Should there also be helper methods for query & hash for the url?
• When using the conn macros it sort of feels strange to start with the return conn instead of the match expression, ex:
  
  I think the other way around is much easier to read.

[mathiversen]

Hope it doesn't come across as nit-picking, just wanted to give some general feedback after some usage. I'm not entirely sure about the future direction so that's why I've not created any issues :)

[jbr]

The conn.path() method only return the relative path of the current connection. I was thinking that maybe the path should return the full path, and maybe have a separate method for the relative path?

I assume you're using * routes? What is the case for using * but wanting the full path?

Should there also be helper methods for query & hash for the url?

Yes, that's an outstanding issue. I haven't added query or hash accessors yet.

When using the conn macros it sort of feels strange to start with the return conn instead of the match expression

This isn't how I imagined these macros being used — I was figuring the first argument would always be short, probably always conn. If you need to specify the response body, I recommend using a match expression instead of conn_unwrap or conn_try, which just expand into match expressions. If you're taking several lines with the conn_try or conn_unwrap macro, you might as well use match and the code will be clearer

let user = match db.users.get(uuid.as_bytes) {
    Ok(user) => user,
    Err(_) => {
        return handle_db_error(conn);
    }
}


// elsewhere
fn handle_db_error(conn: Conn) -> Conn {
    conn.with_body("failed to access db")
        .with_status(500)
        .halt()
}

[mathiversen]

I assume you're using * routes? What is the case for using * but wanting the full path?

Correct, I am experimenting a little with how to structure my router logic and what i did was to have one handler wrapping all my routes, then i created modules for each and every sub-route inside that router. So I've basically got .any("/api/user/*", http::user::handler()) and then inside the user module I've got another handler mapping all of the different endpoints inside that module. But by doing so i sort of lose the context of where the module is used, unless i pass it down, or hard code it. That's why i was a little surprised when the path on connection was just the current context.

This isn't how I imagined these macros being used...

I started out by just using match similar to the example you shared, but i switched to macros for most of the cases as it condensed the code a lot. I quite like it and i don't think id like to pass even this much info to the client in prod. It just felt a little backwards to start with the return type. :)

[jbr]

This is all really useful feedback! Thanks!

The way that mounting subrouters is working is what I intended but I see that in this use case it's not what you'd expect. The use case I was imagining was someone shipping a whole mini app as a crate and an application mounting it. The sub-app wouldn't know ahead of time where it was mounted. But in retrospect, that's probably a less common use case, even though I'd like to still support it. Maybe the sub-router would have a setting that says "I don't know where I'm being mounted, truncate the path please." I think I can do this directly in the router, so that's a quick change

The more I think about it, I like the idea of switching the conn_try and conn_unwrap arg order. Unfortunately, that's a clear breaking change to the public api, so I'll probably throw it on a 0.2 branch and it'll probably languish a bit while I accumulate enough other breaking changes to the trillium crate to be worth the release effort (requires releasing patch versions of 23 other crates 😬). Breaking changes to that specific crate is intentionally difficult in order to keep the core dependency small and stable.

Thanks again for the feedback!

[jbr]

.any("/api/user/*", http::user::handler())

Do you actually want any here, or would this be useful: .methods(&[Method::Get, Method::Post, Method::Delete], "/api/user/*", http::user::handler()) (.methods(&["get", "post", "delete"], "/api/user/*", http::user::handler()) would also work)

I might actually replace any with all and call methods in the above example any, but the interface would be the same: a slice of methods and the handler

[mathiversen]

requires releasing patch versions of 23 other crates grimacing

No worries, maintaining public repos require a lot of work so don't stress it! I'm just happy to help :)
I like your ideas for .any and .all