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

Replace Finch for Req as default swoosh client in installer #5882

Merged
merged 1 commit into from
Aug 3, 2024
Merged

Conversation

Gazler
Copy link
Member

@Gazler Gazler commented Jul 26, 2024

Swoosh has supported Req since 1.14.1. Although Req uses Finch under the hood, Req is a more "batteries included" approach. This means that there is no need to start an HTTP client manually, as was required by Finch.

@Gazler Gazler requested a review from josevalim July 26, 2024 16:55
Swoosh has supported Req since 1.14.1. Although Req uses Finch under the
hood, Req is a more "batteries included" approach. This means that there
is no need to start an HTTP client manually, as was required by Finch.
@@ -75,7 +75,7 @@ config :<%= @web_app_name %>, <%= @endpoint_module %>,
# domain: System.get_env("MAILGUN_DOMAIN")
#
# For this example you need include a HTTP client required by Swoosh API client.
# Swoosh supports Hackney and Finch out of the box:
# Swoosh supports Hackney, Req and Finch out of the box:
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should just remove this section from the docs? We already config a client for prod anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, keep the part about configuring the adopter, drop the part about the HTTP Client.

{:swoosh, "~> 1.5"},
{:finch, "~> 0.13"},<% end %>
{:swoosh, "~> 1.16"},
{:req, "~> 0.5.4"},<% end %>
Copy link
Member

Choose a reason for hiding this comment

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

We can probably relax it a bit?

Suggested change
{:req, "~> 0.5.4"},<% end %>
{:req, "~> 0.5"},<% end %>

Copy link
Member Author

Choose a reason for hiding this comment

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

I went for 0.5.x because I thought 0.6 may introduce breaking changes. Happy to go with either though.

Copy link
Member

Choose a reason for hiding this comment

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

As you prefer! It is only for new apps and there will be a lock file anyway. And we can always update it.

{:swoosh, "~> 1.5"},
{:finch, "~> 0.13"}<% end %>
{:swoosh, "~> 1.16"},
{:req, "~> 0.5.4"}<% end %>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{:req, "~> 0.5.4"}<% end %>
{:req, "~> 0.5"}<% end %>

{:swoosh, "~> 1.5"},
{:finch, "~> 0.13"},<% end %>
{:swoosh, "~> 1.16"},
{:req, "~> 0.5.4"},<% end %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just so you know, I'm considering some changes before Req v1.0 that are maybe worth mentioning here:

  1. Since day 1, Req retries on errors by default. I've had some feedback over the years this is a bit surprising. There's a recently opened issue too: Retrying by default is a bit surprising wojtekmach/req#383. If you'd like to chime in, that's appreciated. I added some extra context in Swoosh too: https://github.com/swoosh/swoosh/pull/830/files#r1694016381.

  2. Since day 1, Req brings in jason dependency. I'd like to drop it before v1.0 in favour of built-in :json from OTP 27. I'd be OK even conditionally vendoring it in for pre-27. This is tracked here: json: Support optional use of :json in OTP 27+ wojtekmach/req#386.

Btw, Finch is dropping :castore dependency and once Finch is out with this change, I'm keen on requiring that Finch version as minimum in Req. I'd probably cut Req v0.6.0 for this change. This is effectively requiring OTP 25 (and running on system that have built-in CA) for everything to work out of the box.

{:req, "~> 0.5.4"} is definitely good for now. I'd be happy to send a patch bumping that to ~> 0.6/1.0 in the future.

Dunno how relevant are these for Phoenix but I thought Id mentioned it anyway!

Copy link
Member

Choose a reason for hiding this comment

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

Both Swoosh and Phoenix depends on Jason, so making it optional on Req won't affect us. I also don't think OTP 25 requirement is an issue, this is for new projects and that's the minimum supported OTP version nowadays anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding req retries by default and swoosh, this is all moot, sorry about the noise. In req the default is retry: :safe_transient, safe meaning only for GET/HEAD requests. Sending emails would be typically done using POST. So we are good.

@Gazler Gazler merged commit fd80465 into main Aug 3, 2024
14 checks passed
@Gazler Gazler deleted the gr-use-req branch August 4, 2024 20:56
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.

4 participants