Add default config in config.exs + ability to configure Tesla middleware#71
Add default config in config.exs + ability to configure Tesla middleware#71ggiill wants to merge 4 commits intochulkilee:mainfrom
config.exs + ability to configure Tesla middleware#71Conversation
ggiill
changed the title
ggiill
changed the title
config.exs + ability to configure Tesla middleware
ggiill
force-pushed
the
configure-tesla-middleware
branch
from
5039dbc to
1a835ce
Compare
chulkilee
left a comment
chulkilee
left a comment
There was a problem hiding this comment.
Actually I think it's better to avoid config at all (I'd rather remove the default api version!) - and let each user decide how to use this library.
I see #71 and #42 were created to customize the Tesla middleware - I believe we should only change ExForce.Client.Tesla to take append_middleware, not introducing any other config.
lib/ex_force.ex
Outdated
| Configure default settings in config/config.exs (optional). | ||
|
|
||
| ```elixir | ||
| # config/config.exs | ||
|
|
||
| config :ex_force, :api_version, "43.0" | ||
| config :ex_force, :adapter, {Tesla.Adapter.Hackney, [recv_timeout: 1_000]} | ||
| config :ex_force, :middleware, [ | ||
| Tesla.Middleware.Telemetry, | ||
| {Tesla.Middleware.Timeout, timeout: :timer.seconds(1)} | ||
| ] | ||
| ``` | ||
|
|
There was a problem hiding this comment.
I'd like to avoid adding package level config like this. This can be done by having own application config and wrapper around ExForce.
There was a problem hiding this comment.
Many Elixir libraries support application-level config. This is optional and can be overridden for each client if need be.
There was a problem hiding this comment.
Updated to:
# config/config.exs
config :ex_force, ExForce.Client.Tesla,
api_version: "43.0",
adapter: {Tesla.Adapter.Hackney, [recv_timeout: 1_000]},
append_middleware: [
Tesla.Middleware.Telemetry,
{Tesla.Middleware.Timeout, timeout: :timer.seconds(1)}
]
lib/ex_force/client/tesla/tesla.ex
Outdated
| ], | ||
| Keyword.get(opts, :adapter) | ||
| ) | ||
| custom_middleware = get_from_opts_or_config(opts, :middleware, []) |
There was a problem hiding this comment.
This middleware is always appended - so it would be better to make that explicit. What would be the better option name? append_middleware?
There was a problem hiding this comment.
Sure - I can change that name and also only scope it to ExForce.Client.Tesla.
| defp get_headers(opts), do: Keyword.get(opts, :headers, [{"user-agent", @default_user_agent}]) | ||
|
|
There was a problem hiding this comment.
Please refrain from moving this function around - as they're defined next to where its first use (not grouped by private funcs).
lib/ex_force/client/tesla/tesla.ex
Outdated
| |> List.flatten() | ||
| |> Tesla.client(adapter) |
There was a problem hiding this comment.
Let's avoid using pipe here as we're not transforming a data here.
We can just ++ by enforcing passing list
[
{ExForce.Client.Tesla.Middleware,
{instance_url, Keyword.get(opts, :api_version, @default_api_version)}}
{Tesla.Middleware.Compression, format: "gzip"},
{Tesla.Middleware.JSON, engine: Jason},
{Tesla.Middleware.Headers, headers},
] ++ Keyword.get(opts, :append_middlewares, [])
lib/ex_force/client/tesla/tesla.ex
Outdated
| adapter = get_from_opts_or_config(opts, :adapter) | ||
|
|
||
| [ | ||
| {Tesla.Middleware.DecodeJson, engine: Jason}, |
There was a problem hiding this comment.
Do we need to change this order?
2 participants
Inspired by #42
Adding the ability to configure default options in
config.exs:As well as the ability to pass in custom Tesla Middleware: