feat(all): expand

[martsokha]

Changes so far:

• add features for using both reqwest/native-tls (enabled by default) and reqwest/rustls-tls
• replace feature async with blocking, same as in reqwest
• add crate-specific Result type alias
• service-based client, similar to https://github.com/resend/resend-go
• add base_url support with an env var RESEND_BASE_URL
• add impl Default for a client with an env var RESEND_API_KEY
• add USER-AGENT to the request with RESEND_USER_AGENT

[AntoniosBarotsis]

This is quite a lot of changes 😅. The PR has multiple times more effort put into it than the entire project which is interesting.

I have been taking a few looks at this in the last few days even though there's a decent amount of todos and stuff. Do write any messages in here if you want to discuss anything, I'll be sure to reply asap.

[AntoniosBarotsis]

Most looks great! Just a reminder, you've left some todos for yourself in lib.rs.

Also another thing, in CI I usually do cargo clippy --all-features, not sure how I forgot it in this repo.

I'll also have to rethink CI testing cause obviously without a key that wont work. I will probably create a separate API key just these tests but I'll have to see.

[AntoniosBarotsis]

Would be nice to have a before-after conversion similar to what I had in v0.3. Since there was just 1 method before this PR this should be easy. I have a code example here.

[martsokha]

The weird formatting is a result of me using deno fmt for markdown files.
I'm a bit confused, do you want an example in the client documentation?

[AntoniosBarotsis]

I think Github likes showing 6 lines minimum because I specifically selected the "unreleased" line. Meant to add an example similar to this:

for the "hello world" example I had in the repo before, in case anyone that was using the crate already wants to upgrade. I can add this myself as well.

[martsokha]

• Should we impl Deref for a Client to ClientInner (same as the Client right now) to limit cloning? Could be useful for something like a State extractor in Axum.
• What should we do with Options in responses, for example, CreateAudienceResponse?

[AntoniosBarotsis]

• Don't see why not
• I should've read the API docs more carefully cause I'm guessing in most cases the responses will always have all the fields unless specified otherwise in the docs so we should probably remove them where it makes sense. Were the options added initially because we were not handling error responses?

[martsokha]

• Don't see why not
• I should've read the API docs more carefully cause I'm guessing in most cases the responses will always have all the fields unless specified otherwise in the docs so we should probably remove them where it makes sense. Were the options added initially because we were not handling error responses?

No, options were automatically generated by the openapi-generator from the .yaml file.
https://resend.com/changelog/introducing-resends-openapi-spec

[AntoniosBarotsis]

Ah makes sense. I'm not 100% sure on this but from the docs it does not look like these should be optional which is weird that they then get generated like this. I think it seems safe to remove them but I wonder if they get generated like that with the idea that a response might have a completely different structure if it was an error (thus each param is optional cause it might not exist, to avoid crashes). What do you think?

[martsokha]

Looks like the response fields in resend-go are tagged with json:"omitempty".
I suppose I'll leave them optional in this case.

https://github.com/resend/resend-go/blob/main/emails.go#L12

Edit. Oh well, other are not tagged:

https://github.com/resend/resend-go/blob/main/audiences.go#L28

I think it's reasonable to only remove options from the response fields that aren't tagged.

[martsokha]

• Not sure what to do about the rate limit for a blocking client, should we just ignore it or maybe sleep for 1/reqs_per_sec seconds? Meh.
• Can you check out those last 3 TODOs?

[AntoniosBarotsis]

I think it's best to not tackle the rate limiting at all. It's best (and easier of course) to let the users deal with it, especially since there's different ways to deal with it + there's the possibility some users are not even rate limited to begin with (not sure if some premium plan changes this for example).

As for the todos:

• For the domain test: I guess it's unfortunate if we can't try those out without an actual domain, we could mention this in the module docs perhaps. We technically could mock the responses by copying the ones they have in their examples but not sure there's much point tbh
• The SendEmail::new seems a bit over engineered but fine at the same time and I can't come up with something better. I mean it works so good enough haha
• I'm guessing the third todo was the rate limit note i left. I think it is fine for now since using 1 thread works. I might look into what you can do with a custom test harness or something in the future but not something I'll do anytime soon.

[martsokha]

• Yeah, you can request an additional rate limit; that's precisely why I've added the RESEND_RATE_LIMIT env variable.
  The tower::limit::RateLimit middleware is the obvious choice for an asynchronous client since reqwest::Client implements tower::Service. Another viable option would be a custom tower::Layer<http::Request, Response = http::Response> as per ietf spec.
  I'll get rid of the tower for now; I'll probably come back to it if a better solution comes up.
• Same-same.

Only a few things left then:

• Clippy: this argument is passed by value, but not consumed in the function body, should we ignore it?
• Clippy: this could be a const fn, seems to be worthless in most cases
• Update CHANGELOG.md and README.md.
• Maybe it's worth to message resend to add resend-rs to the community SDK page (https://resend.com/docs/sdks)

[AntoniosBarotsis]

• I'm a bit skeptical with this, are there any points where we are not using references? I feel like even though references are less annoying for users, I can't think of a case where you would want to use the same request object more than once for instance (so I feel like forcing owned values in the parameters might prevent some bugs where people forget to pass the object at all). Another thing that might suck if we use references is if for whatever reason we need to switch to owned values later on which would be a breaking change. I think it is best we ignore the lint (on each instance of the warning though, not globally)
• For the const functions, from what I remember there's no penalty from making them const so why not I guess if non-const code can still call them then why not.
• I'll do these two myself as well at some point
• This would be neat! I could ask on their Discord or something after this is merged

[martsokha]

• As of now, we take *Ids by reference (as Ids are reusable) and request types by value, e.g.

pub async fn create(&self, audience: &AudienceId, contact: ContactData) -> Result<ContactId> { ... }

• Alright, added consts.

Btw, what's the reason for a custom config.toml? Why not clippy with a pedantic flag? Not exactly the same, tho.

[AntoniosBarotsis]

Yeah ig ids are fine as references and we can keep everything else as owned values. Any other thoughts on this?

I've mostly kept the config from back when I was still very new to the language, just out of habit. I should re-evaluate it at some point cause I bet it is annoying to deal with 😅. It is not just pedantic cause i like my editor spamming me with warnings

[AntoniosBarotsis]

I'll take a look at the readme and the changelog but other than that everything is (finally 🎉) done right?

[martsokha]

Yeah, I think so. I'll double-check in the evening.

[AntoniosBarotsis]

LGTM!