req icon indicating copy to clipboard operation
req copied to clipboard

Add `Req.add_request_steps/3` and friends

Open wojtekmach opened this issue 1 year ago • 5 comments

Currently we have have pairs of append and prepend functions for each type of steps, Req.Request.append_request_steps/2, Req.Request.prepend_request_steps/2, ...

My idea of Req and Req.Request separation is the former is what most users would use most of the time and the latter is for putting together very custom request pipelines but in practice most commonly for developing custom steps.

A crucial idea for Req is using custom steps so by that rationale we should have a function on Req, not Req.Request. See https://github.com/wojtekmach/req/issues/139#issuecomment-1314314060.

Eventually we also want to add add steps after specific steps. See https://github.com/wojtekmach/req/issues/148.

I think we can solve both concerns as well as get rid of append/prepend pairs with this API:

Req.new()
|> Req.add_request_steps([foo: &foo/1])         # by default we append
|> Req.add_request_steps([foo: &foo/1], at: -1) # same as above
|> Req.add_request_steps([foo: &foo/1], at: 0)  # prepend
|> Req.add_request_steps([foo: &bar/1], after: :foo)
|> Req.add_request_steps([foo: &bar/1], before: :foo)
|> Req.add_response_steps([foo: &bar/1])
|> Req.add_error_steps([foo: &bar/1])

I don't plan to support :after and :before just yet, still waiting for concrete use cases, but that's something I want to keep in mind.

One more note about naming. When introducing Req.update I had an option to call it Req.Request.update. (https://github.com/wojtekmach/req/issues/139#issuecomment-1313500753). In "regular" usage, I thought this was better:

  Req.new
- |> Req.Request.update(options)
+ |> Req.update(options)
  |> Req.get!()

but simultaneously this seemed slightly worse:

  def attach(%Req.Request{} = request, options \\ []) do
    request
    |> Req.Request.register_options([:print_headers])
-   |> Req.Request.update(options)
+   |> Req.update(options)
    |> Req.Request.append_request_steps(print_headers: &print_request_headers/1)
    |> Req.Request.prepend_response_steps(print_headers: &print_response_headers/1)
  end

so the same goes for this Req.add_request_steps vs Req.Request.add_request_steps. Maybe the Req vs Req.Request separation isn't such a good idea after all?

Just to be clear, I'd hate to make significant API changes but everything is on the table before v1.0. If we'd end up making significant API changes, we'll deprecate things and only remove them on v1.0 or v1.1.

wojtekmach avatar Feb 28 '23 09:02 wojtekmach