reqwest-middleware icon indicating copy to clipboard operation
reqwest-middleware copied to clipboard

implement `Middleware` for common wrapper types

Open ezracelli opened this issue 2 years ago • 3 comments

Motivations

I'm building a type-safe API client around an untyped REST API, and I'm trying to accept dynamic Middleware. Because this client's builder's stack is dynamic, the middleware needs to be represented as Box<dyn Middleware>, which Middleware is not implemented for.

Solution

Middleware should be implemented for common derivatives and wrapper types:

  • &T
  • &mut T
  • Arc<T>
  • Box<T>
  • Cell<T>
  • Cow<T>
  • MutexGuard<T>
  • Pin<T>
  • Rc<T>
  • Ref<T>
  • RefMut<T>

Alternatives

I'm currently wrapping the Box<dyn Middleware> with a newtype, like:

struct BoxedMiddleware(Box<dyn Middleware>);

and impl Middleware for BoxedMiddleware, but this will need to be replicated for each crate (I'll be building multiple wrapper clients), so it'd be much cleaner to have the impls in the source crate!

Additional context

Happy to submit a PR! 😁

ezracelli avatar Aug 29 '22 21:08 ezracelli

Great idea, to avoid unnecessary bloat, I'd rather just &T, Box<T> and Arc<T> for now since these are the most common.

One thing to remember when implementing is to ensure that the ?Sized bound is specified otherwise Box<dyn Middleware> won't work (I sometimes forget this...)

impl<T: ?Sized + Middleware> ...
//      ^^^^^^

conradludgate avatar Aug 30 '22 16:08 conradludgate

Looking back - I think implementing this on Arc<dyn Middleware> is a bit of an anti-pattern since we manually wrap them in arcs anyway. We should be promoting the use of with_arc instead where applicable

conradludgate avatar Aug 30 '22 20:08 conradludgate

Hmm, yeah that makes sense! I actually have no idea what's considered an anti-pattern here — is Arc<Arc<T>> a normal thing?

ezracelli avatar Aug 30 '22 23:08 ezracelli