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

Expose common types for App<T, _> #2301

Closed
wants to merge 1 commit into from

Conversation

chinedufn
Copy link

@chinedufn chinedufn commented Jul 5, 2021

Before this commit, many of the common monomorphizations of App<T, B>
where impossible to return from a function.

For example:

/// Create the actix-web application that can be used to start an HttpServer.
pub fn create_app() -> App<
    impl ServiceFactory<
        ServiceRequest,
        Config = (),
        Response = ServiceResponse<StreamLog<Body>>,
        Error = actix_web::error::Error,
        InitError = (),
    >,
    StreamLog<Body>,
> {
    unimplemented!()
}

Would fail to compile since the ServiceFactory a a couple of other types were not publicly exported from actix-web.


In this commit, a few types that are commonly used when constructing an App are now exported.

This allows you to have a fn create_app () -> App<T, B> that can be re-used across your test suite.

Before this commit, many of the common monomorphizations of `App<T, B>`
where impossible to return from a function.

For example:

```
/// Create the actix-web application that can be used to start an HttpServer.
pub fn create_app(
    config: AppConfig,
) -> App<
    impl ServiceFactory<
        ServiceRequest,
        Config = (),
        Response = ServiceResponse<StreamLog<Body>>,
        Error = actix_web::error::Error,
        InitError = (),
    >,
    StreamLog<Body>,
> {
    unimplemented!()
}
```

Would fail to compile since the `ServiceFactory` was not publicly
exported from actix-web.

---

In this commit, a few types that are commonly used when constructing
an App are now exported.

This allows you to have a `fn create_app () -> App<T, B>` that can be
re-used across your test suite.
@robjtede
Copy link
Member

robjtede commented Jul 6, 2021

we can see about exposing ServiceFactory through the dev module but the others are intentionally private types

@robjtede
Copy link
Member

robjtede commented Jul 6, 2021

pub fn create_app() -> App<
    impl ServiceFactory<
        ServiceRequest,
        Response = ServiceResponse<impl MessageBody>,
        Config = (),
        InitError = (),
        Error = Error,
    >,
    impl MessageBody,
> {
    App::new()
        .wrap(Logger::default()) // <-- can be removed without affecting the return type
        .route("/", web::to(|| async { "hello" }))
}

@robjtede robjtede closed this Jul 6, 2021
@chinedufn
Copy link
Author

Thanks for the impl MessageBody bit.

I think your example still doesn't work currently since ServiceFactory isn't exposed.

Can we re-open this PR and I'll change it to re-export ServiceFactory from dev?

@chinedufn
Copy link
Author

chinedufn commented Jul 7, 2021

Ah, one other issue. Your signature works fine, but it doesn't pass the trait bounds for HttpServer::new()

error[E0277]: the trait bound `App<impl ServiceFactory<ServiceRequest>, impl MessageBody>: actix_service::IntoServiceFactory<_, actix_http::request::Request>` is not satisfied
  --> crates/api-server/src/main.rs:20:5
   |
20 |     HttpServer::new(move || {
   |     ^^^^^^^^^^^^^^^ the trait `actix_service::IntoServiceFactory<_, actix_http::request::Request>` is not implemented for `App<impl ServiceFactory<ServiceRequest>, impl MessageBody>`
   |
   = help: the following implementations were found:
             <App<T, B> as actix_service::IntoServiceFactory<actix_web::app_service::AppInit<T, B>, actix_http::request::Request>>
   = note: required by `HttpServer::<F, I, S, B>::new`
    HttpServer::new(|| {
        create_app()
    })

I'm reading through the different trait bounds now to get myself familiar with how to get this working, but if you can suggest any tips here that would be awesome.


Also, I can't re-open the PR so I believe you'll have to.


Thanks!


EDIT - I think the issue is that the two impl MessageBody aren't guaranteed to be the same type, which is why this doesn't pass the HttpServer::new() trait bounds.

@robjtede
Copy link
Member

robjtede commented Jul 7, 2021

there's a problem with reopening, you'll have to create a new pr

@robjtede
Copy link
Member

robjtede commented Jul 7, 2021

indeed it seems like there's no nice way to express the link between those two impl MessageBody types (yet) so the last middleware would need putting in Compat and use this:

pub fn create_app() -> App<
    impl ServiceFactory<
        ServiceRequest,
        Response = ServiceResponse<AnyBody>,
        Config = (),
        InitError = (),
        Error = Error,
    >,
    AnyBody,
> {
    App::new()
        .wrap(Logger::default())
        .wrap(Compat::new(Logger::default()))
        .route("/", web::to(|| async { "hello" }))
}

fn main() {
    let _ = HttpServer::new(|| create_app());
}

@robjtede
Copy link
Member

robjtede commented Jul 7, 2021

Alternatively, (and this is what I did in my own projects) using macro_rules! is simple and easy way to share a create_app fn with main and tests.

macro_rules! create_app {
    () => {
        App::new()
            .wrap(Logger::default())
            .route("/", web::to(|| async { "hello" }))
    };
}

fn main() {
    let _ = HttpServer::new(|| create_app!());
}

@chinedufn
Copy link
Author

Yeah the macro was going to be my last resort.

Awesome, thanks for teaching me about Compat. Worked like a charm.


I'll open a new PR thanks!

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

Successfully merging this pull request may close these issues.

2 participants