TrueLayer / reqwest-middleware

Wrapper around reqwest to allow for client middleware chains.
Apache License 2.0
257 stars 78 forks source link

chore: revert change switching from `Arc` to `Box` in public APIs #142

Closed eopb closed 6 months ago

eopb commented 6 months ago

fixes #139 related https://github.com/TrueLayer/reqwest-middleware/pull/135

There are other potential long term solutions listed here

I didn't put much thought into it. Internally there used to be a Box<[Arc<dyn Middleware>]> which is cloned about quite often.

Because of all the cloning, I decided to flip it to be Arc<Box> instead of Box<Arc>.

I see three solutions here.

  1. The one I'd lean towards is Arc<Arc> even if it's a bit silly, but prevents extra unnecessary allocations on each request.
  2. An alternative is to implement Middleware for Arc and then you can use the regular with(...) api. This would mean there's an extra box around your middleware
  3. Revert this particular change

In the short term, I think it's best to go with option 3. This will unblock the next release. We can consider the other options for future releases.