-
Notifications
You must be signed in to change notification settings - Fork 257
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
Extensibility of AppContext #522
Comments
I'm not sure about the implication of extending |
I totally agree this framework refactor is massive and may introduce breaking among components. Can we explore alternative approaches that could allow us to achieve the same extensibility on |
Some way to make But I'm not sure how this can be done easily as the system has no way to know which transaction an API endpoint wants... In that respect, it would be very helpful to be able to create a temp Something like: let trans = ctx.begin().await?;
let new_ctx = ctx.clone_with_transaction(&trans); // Clone the context but replace the connection with a transaction
// ... use State(new_ctx) when calling other JSON API's
let item = add_one(State(new_ctx), params).await?;
trans.commit().await?; |
How about something like adding an pub trait Hooks: Send + Sync {
type ExtraAppContext: Send + Sync + Clone + 'static;
...rest of hooks trait
} then making pub struct AppContext<T>
where
T: Send + Sync,
T: Clone,
{
/// The environment in which the application is running.
pub environment: Environment,
#[cfg(feature = "with-db")]
/// A database connection used by the application.
pub db: DatabaseConnection,
/// An optional connection pool for Queue, for worker tasks
pub queue: Option<Pool<RedisConnectionManager>>,
/// Configuration settings for the application
pub config: Config,
/// An optional email sender component that can be used to send email.
pub mailer: Option<EmailSender>,
// An optional storage instance for the application
pub storage: Arc<Storage>,
// Cache instance for the application
pub cache: Arc<cache::Cache>,
// Extra user defined data
pub extra: Option<T>,
} I tried this with a local copy of the repo, and it ended up working well, the only thing is having to add type parameters everywhere app context is used. Here is the diff with my fork master...ElijahJohnson5:loco:master |
This works for most of the use cases. However, if we are implementing third-party traits/functions directly on One example is implementing Cookie // demo/src/app.rs
use loco::prelude::*;
use axum_extra::extract::cookie::Key;
// This is not going to work since both trait and struct are not in the same crate
impl FromRef<AppContext> for Key {
fn from_ref(state: &AppContext) -> Self {
state.0.key.clone() // You can also use `Key::generate()` to avoid the implementation
}
} Error Only traits defined in the current crate can be implemented for arbitrary types [E0117] |
Is that not a problem already with the current |
Yeah, it is an existing problem with the current |
Is the plan to move to something like an |
I want to thank everyone for the discussion and especially @mstallmo for providing something we can look at and play with. As was discussed the trait+generics solution is indeed breaking in many places. I'm wondering, as we're about to let users "digest" all these code changes they have to make, what are the use cases and motivations for extending or creating a custom AppContext. For my educational purposes, maybe I'm missing something.
So, if we are able to provide some "sugar" over these use cases maybe the issue is solved without creating the generic appcontext and breaking code. At this point I worry -- is there a use case I missed that forces extension of AppContext? |
I'll share what motivated me to implement this change and why I think it will be useful for others. Hopefully this can shed some light on why this change would be worth the disruption. My Specific Case I was looking into using Loco with an InertiaJS based frontend for an app that I'm working on and found the axum-inertia crate that implements the InertiaJS protocol for Axum. To use this crate with an Axum app the From their documentation this can either be done by using the General Case I suspect that there are other crates in the wider Axum ecosystem that have a similar requirement to the one that I ran into with axum-inertia. In general, libraries that are created to work with Axum wouldn't expect that a user would not be able to modify the struct that is present in There is an argument to be made that a solution would be to fork or create a new loco specific crate that does the same thing (something that I am also working on for inertia) that instead uses With that said I think it would be good for Loco to be as compatible with the wider Axum ecosystem as possible as it both makes transitioning from an Axum app to a Loco app easier for users and gives them access to crates that were created to work with Axum without having to fork or create a new crate on their own. |
Thanks. This is very interesting. I will spend some time learning what inertia is, but meanwhile I read the create source code and it seems that it very easily can use Extension. in fact I’m almost convinced it should have used extension because it is exactly fitting for what extension is. Talking over state means some thing that’s central. Loco took over state because it is central to the app as it is the core framework so it needs this freedom of operation. I would actually submit a change to the inertia crate to move to use Extension or at least wonder why they didn’t. |
You're 100% right on this. I have a fork of I ultimately decided to go down the road of making a loco specific version of the crate because I wanted nicer integration with the rendering facilities already present in loco and to make a smoother holistic experience for loco apps. For these kinds of scenarios I think my general question is this: How many existing crates in the ecosystem use |
I would say a lot. Just in Loco itself we use Extension for all the extensions we have that are optional. As long as the type is unique you can have as many as you want. PS Even if you did not use Loco and had your own state, the intertia crate would force a refactor on you. |
It would but it would be a minimal refactor, no? Just implementing |
Correct, but Extension makes this completely transparent |
Another point to consider in favor of allowing users to utilize custom With extensions you do not get a compile time error if an extension is missing that is needed by a route handler. Instead this shows up at runtime with a panic or server error response. Since the Allowing for custom state gives user the ability to lean on Rust's strong compile time guarantees with respect to state used in their route handlers. |
Feature Request
The current AppContext does not allow users to extend the struct which forces the user to create a lot of workaround when working with extensions/initializers.
Describe the solution you'd like
We can create a thin wrapper which exposes the inner structure to the user and allow the user to add/modify the wrapper as they want.
Discord Discussion
Discord link
The text was updated successfully, but these errors were encountered: