beam-community / stripity-stripe

An Elixir Library for Stripe
Other
965 stars 344 forks source link

Make hackney an optional dependency #835

Closed sbaildon closed 3 months ago

sbaildon commented 4 months ago

Requiring :hackney brings in a ton of additional dependencies that can be avoided.

Unsure about how to add useful tests, some that set :http_module already exist.

yordis commented 4 months ago

What would happen if hackney was not installed with this change?

sbaildon commented 4 months ago

App starts, but errors when hackney is not available

iex(2)> Stripe.Customer.list()
** (UndefinedFunctionError) function :hackney.request/5 is undefined (module :hackney is not available)
    :hackney.request(:get, "https://api.stripe.com/v1/customers", [{"Accept", "application/json; charset=utf8"}, {"Accept-Encoding", "gzip"}, {"Authorization", "Bearer sk_test_abc"}, {"Connection", "keep-alive"}, {"Content-Type", "application/x-www-form-urlencoded"}, {"Stripe-Version", "2022-11-15"}, {"User-Agent", "Stripe/v1 stripe-elixir/2022-11-15"}], "", [{:pool, Stripe.API}, :with_body])
    (stripity_stripe 3.1.1) lib/stripe/api.ex:387: anonymous fn/5 in Stripe.API.do_perform_request_and_retry/6
    (telemetry 1.2.1) /home/user/src/sans_hackney/deps/telemetry/src/telemetry.erl:321: :telemetry.span/3
    (stripity_stripe 3.1.1) lib/stripe/api.ex:386: Stripe.API.do_perform_request_and_retry/6
    (stripity_stripe 3.1.1) lib/stripe/request.ex:203: Stripe.Request.make_request/1
    iex:2: (file)

Works (so far) when :http_module is set to something that satisfies request/5

config :stripity_stripe,
  http_module: SansHackney.HTTPClient,
defmodule SansHackney.HTTPClient do
  def request(method, url, headers \\ [], body \\ nil, opts \\ []) do
    request = Finch.build(method, url, headers, body, opts)

    do_request(request)
  end

  defp do_request(request) do
    request
    |> Finch.request(Finch)
    |> case do
      {:ok, response} ->
        {:ok, response.status, response.headers, response.body}

      {:error, reason} ->
        {:error, reason}
    end
  end
end

I'm still integrating stripity-stripe, we could consider marking this PR as a draft whilst the edge cases get caught

yordis commented 4 months ago

App starts, but errors when hackney is not available

Yeah... we can not break existing applications. I do agree with you that hackney is less than suboptimal, but we can not break others.

I'm still integrating stripity-stripe, we could consider marking this PR as a draft whilst the edge cases get caught

Done ✅

yordis commented 3 months ago

Feel free to reopen it once you are happy with a solution. We shouldn't break change for now