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

Unify configuration #465

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thiamsantos
Copy link
Contributor

@thiamsantos thiamsantos commented May 17, 2021

Implementation of #234

This is a draft of the implementation, I'm opening to check if the approach makes sense. cc: @teamon

Proposed solution

Use nimble_options to validate the opts given to a middleware. For a option to be fetched from Application Configuration, it needs to set global: true in the config_schema of the middleware.

It follows a clear precedence of first fetching from application and then overriding with the configuration given directly as options to the middleware. It also always fetches the configuration in runtime.

Based on the schema a documentation for the options is generated:

Screenshot_2021-05-17 Tesla Middleware Logger — tesla v1 4 1

TODO

  • Documentation
  • Unit Tests
  • Use in all middlewares

@teamon
Copy link
Member

teamon commented May 25, 2021

Thank you, @thiamsantos, for picking this up!

I think this looks good, especially that it would introduce proper options validation instead of ad-hoc solutions implemented in some middlewares. I'm naturally hesitant with introducing a new dependency, but since it will be used by all middlewares it makes sense to include it (instead of reimplement option validation inside tesla).

In general, there are two cases that need to be taken care of:

  1. Client module in the same codebase - full access to code, configuration can be done via options passed directly to middleware, application env useful when providing different setting for different environments (dev/test/prod)
  2. Client module from third-party library - no direct access to code, configuration only possible if exposed or via application env

Recently, I've been using this pattern to configure first-party clients:

defmodule SomeAPI do
  def get_foo(client \\ new(), id), do: ...
  
  def new(opts \\ []) do
    config = Application.get_env(:myapp, __MODULE__, [])
    opts = Keyword.merge(config, opts)
    
    middleware = [
      {Tesla.Middleware.BaseUrl, opts[:base_url]},
     ...
    ]

    Tesla.client(middleware, config[:adapter])
  end
end

It is especially useful for changing base URL and/or adapter in test.

@hodak @amatalai @rafapaez Would you mind taking a look at this proposal in terms of how would it affect your codebases?

@teamon teamon linked an issue May 8, 2022 that may be closed by this pull request
@teamon
Copy link
Member

teamon commented May 8, 2022

cc @elixir-tesla/tesla

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.

Unify configuration
2 participants