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(client): add default header middleware #1181

Closed

Conversation

joelwurtz
Copy link
Contributor

This PR add a new middleware to set default headers for client, avoiding repeating the same calls when we are sure that some headers should be always there

@joelwurtz joelwurtz marked this pull request as ready for review January 16, 2025 11:15
@joelwurtz joelwurtz force-pushed the feat/client-default-header-middleware branch from 06f4f0c to f9a7fd5 Compare January 16, 2025 11:18
@fakeshadow
Copy link
Collaborator

fakeshadow commented Jan 17, 2025

Thanks for the PR.

In general we want to avoid adding too much default middleware unless it has complicated internal logic that end user may have difficulty implementing themselves. The default header one seems not belonging into that category.

I do get applying headers to the request API is not a good experience and I guess my opinion is we should consider making writing it easier without adding a dedicated middleware.

What do you think if we add middleware_fn API where an async closure is accepted as middleware like how xitca-web hehave. And adding default headers will become something like

Client::builder()
    .middleware_fn(async |req, srv| {
        req.headers_mut().insert();
        srv.call(req).await
    })

This would offer the same functionality as the middleware proposed by this PR and on top of it enables closure capture for stateful headers constructing on fly.

@joelwurtz
Copy link
Contributor Author

I think that's a good idea, I also want to add a better api on the request builder to add header with a fluent interface (returning Self)

@joelwurtz
Copy link
Contributor Author

Should the client reuse the xitca service crate ? or should we duplicate this logic ? i think reusing all the service / middleware logic from xitca-service would help a lot here

@fakeshadow
Copy link
Collaborator

fakeshadow commented Jan 17, 2025

I explained it in previous discussion #1164 (comment)

Due to current Rust's limitation there is no way to unify the Service trait between thread safe and non thread safe use case. Rust features like RTN and/or TAIT are required to reuse a single abstraction. For now duplicating the trait is the only solution that work on stable Rust.

@joelwurtz
Copy link
Contributor Author

Closed in favor of #1182

@joelwurtz joelwurtz closed this Jan 17, 2025
@joelwurtz joelwurtz deleted the feat/client-default-header-middleware branch January 17, 2025 14:04
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