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

Support instantiating different API clients, and advanced HTTP configuration #14

Merged
merged 7 commits into from
Nov 29, 2022

Conversation

foca
Copy link
Contributor

@foca foca commented Jul 5, 2022

This PR allows you to use Tarpon to target multiple different RevenueCat Projects from within the same codebase.

Our use case is probably uncommon, but we have a backend that supports multiple different mobile apps, each with its own RevenueCat configuration, as a different Project in RC. This means different API keys, and at that point Tarpon is not able to work, since the configuration happens in a singleton object (the Tarpon::Client class).

This changes the library so that it now instantiates Tarpon::Client and calls methods on the instances, rather than calling singleton methods on the class:

config.project_1.revenue_cat = Tarpon::Client.new do |c|
  c.public_api_key = ENV['PROJECT_1_REVENUE_CAT_PUBLIC_KEY']
  c.secret_api_key = ENV['PROJECT_1_REVENUE_CAT_SECRET_KEY']
end

config.project_2.revenue_cat = Tarpon::Client.new do |c|
  # ...
end

In order to preserve backwards compatibility—and to support the common use case of just needing to target a single RevenueCat project, this also introduces a Tarpon::Client.default instance to which all methods are delegated.

This allows users to continue using the same API, but provides the flexibility to target different projects if needed.

This also provides a new API to configure clients, by passing properties to Client.new. This makes it easy for use in e.g. Rails, where you might want to use something like config_for to load different configurations from a YAML file.

Finally, this extends the configuration to allow a Proc to be passed to configure the HTTP.rb requests made under the hood, to enable any and all features of that library, such as instrumentation, proxy usage, or granular timeouts:

Tarpon::Client.new do |c|
  c.http = ->(http) do
    http
      .use(instrumentation: { instrumenter: ActiveSupport::Notifications.instrumenter })
      .via(ENV['PROXY_URL'], ENV['PROXY_PORT'])
      .timeout(read: 5, write: 5, connect: 1)
  end
end

At first I considered adding specific instrumentation options (which is our more immediate need), but I though providing access to the HTTP requests might be a bit more flexible, since it keeps the API surface small (only one extra setting) while giving users a lot of power to tweak the requests.

However, this does make the dependency on HTTP.rb part of the public API of the library, so if, in the future, Tarpon desires to move away from HTTP.rb, that would require a major version change as it would be a breaking change. WDYT?

foca added 4 commits July 5, 2022 18:46
This allows you to use Tarpon to target multiple different RevenueCat
projects from within the same codebase.

This changes the library so that it now instantiates `Tarpon::Client`
and calls methods on the instances, rather than calling singleton
methods on the class.

In order to preserve backwards compatibility—and to support the common
use case of just needing to target a single RevenueCat project, this
also introduces a `Tarpon::Client.default` instance to which all methods
are delegated.

This allows users to continue using the same API, but provides the
flexibility to target different projects if needed.
This makes it easy to pass values from an existing configuration Hash
defined elsewhere.
By letting users access the HTTP.rb request mid-flight, they can enable
advanced features like request instrumentation, HTTP proxy usage, or
more granular timeouts, if desired.
@foca foca changed the title Support instantiating different API clients Support instantiating different API clients, and advanced HTTP configuration Jul 6, 2022
Nested requests objects need the client in order to access the correct
configuration.
@foca foca force-pushed the support-multiple-clients branch from 14ebee7 to c79c9b9 Compare July 6, 2022 14:31
@foca
Copy link
Contributor Author

foca commented Sep 6, 2022

bump? It's been a couple months :)

@blasut
Copy link
Contributor

blasut commented Sep 13, 2022

@foca hey, thanks for the PR I'll review this later today

@blasut blasut self-requested a review September 13, 2022 09:17
Copy link
Contributor

@blasut blasut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for contributing to Tarpon!

Very well written PR. We just have a couple of comments around naming.

lib/tarpon/request/base.rb Outdated Show resolved Hide resolved
lib/tarpon/configuration.rb Outdated Show resolved Hide resolved
lib/tarpon/configuration.rb Outdated Show resolved Hide resolved
spec/tarpon/client_spec.rb Outdated Show resolved Hide resolved
lib/tarpon/client.rb Outdated Show resolved Hide resolved
@blasut blasut requested a review from igorbelo October 5, 2022 07:51
This makes it more explicit and helps make the code more readable.
As a precaution, to make sure we don't call something we don't expect
being called.
@foca
Copy link
Contributor Author

foca commented Oct 20, 2022

Thanks for your thoughtful review, @blasut. ❤️

@foca foca requested review from blasut and removed request for igorbelo October 25, 2022 21:05
Copy link
Contributor

@igorbelo igorbelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @foca,

First of all thanks for contributing to Tarpon and sorry for the super slow response.
I'll make sure to address any questions you may have as soon as possible so we get it off the ground.

As you said your use case is probably not common but I don't believe it's a reason not to support it anyway, as long as we don't break the current API.

I've left a few comments around configuration, felt a little confused having multiple ways to do so, would like to hear your thoughts about it. I'm all for deprecating .configure in the future though and go with the natural way of instantiating in Ruby.

Have a great one.

README.md Show resolved Hide resolved
lib/tarpon/client.rb Show resolved Hide resolved
lib/tarpon/request/base.rb Show resolved Hide resolved
lib/tarpon/request/subscriber.rb Show resolved Hide resolved
lib/tarpon/request/subscriber/entitlement.rb Show resolved Hide resolved
@igorbelo
Copy link
Contributor

Thanks again for your contribution @foca, I'll be making sure the changes work well in production before releasing it.

@igorbelo igorbelo merged commit 60e8dae into fishbrain:master Nov 29, 2022
@igorbelo
Copy link
Contributor

igorbelo commented Dec 1, 2022

Released in v0.5.0.

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.

None yet

3 participants