fcm-rust icon indicating copy to clipboard operation
fcm-rust copied to clipboard

feat: key not from file & other improvements

Open chris13524 opened this issue 1 year ago • 5 comments

This adds support for reading the service account key from a ServiceAccountKey rather than the default using the dotenv variable. Also:

  • Adds a default feature flag for the dotenv dependency and functionality for Client::new(). new() now returns a Result since loading the key is performed immediately rather than later. Remove Default implementation since it doesn't support errors
  • Add ClientBuilder pattern which enables more complex constructors such as allowing to provide a custom reqwest HTTP client
  • Rename and re-export types in client::response to enable callers to match on the error variants. Re-export gauth to enable access to ServiceAccountKey
  • Share the HTTP client with gauth
  • Pull project_id from ServiceAccountKey rather than double-parsing the value
  • Remove .pool_max_idle_per_host(usize::MAX) configuration on reqwest since this is the default
  • Take advantage of gauth now returning bearer token directly for simplified code
  • Switch to .json() helper from reqwest to construct JSON request bodies over doing this by hand with .body(), Content-Type header, and serde
  • Resolves various clippy and fmt errors

Resolves #5

Depends on https://github.com/makarski/gauth-rs/pull/29

Example usage in the project I'm working on: https://github.com/WalletConnect/push-server/pull/316

chris13524 avatar Apr 18 '24 20:04 chris13524

Looking good!

Thank you for this. Shall we merge once your PR in gauth-rs is merged and published?

We might also want this fork published, but I haven't done that before and I'm not sure about a proper crate name... I'm also not particularly comfortable with being the only maintainer or owner :)

rj76 avatar Apr 20 '24 10:04 rj76

Yeah let's wait for the gauth-rs PR to be merged before merging this. I'm working on a few more tweaks/fixes and I'd also like to test all this out in prod on my end first to make sure everything is solid.

I think we should also change the crate version from 1.0.0 to 0.10.0 since this PR makes breaking changes, and there could be more breaking changes going forward. Not sure it's a good idea to commit to a stable API quite yet.

Regarding publishing the fork, it's possible you could get ownership of the crate name from the previous maintainer since that repo appears to no longer be maintained and the current version of FCM it uses will no longer work past June. Alternatively fcm_v1 might be a fine name.

I'm happy to help maintain; my company also maintains the a2 library for APNs.

chris13524 avatar Apr 23 '24 02:04 chris13524

Could you also have a look at #10? The yup-oauth2 package is well maintained and also has what you need, if I'm correct.

rj76 avatar May 22 '24 09:05 rj76

Could you also have a look at https://github.com/rj76/fcm-rust/pull/10? The yup-oauth2 package is well maintained and also has what you need, if I'm correct.

It doesn't look like that PR provides a way to get a service account key from a string, also looks like there will be a lot of merge conflicts here.

Perhaps we should drop gauth and switch to yup-oauth2?

chris13524 avatar May 22 '24 14:05 chris13524

Perhaps we should drop gauth and switch to yup-oauth2?

Yeah, that was also what I was thinking, but wanted to check with you because of this PR.

rj76 avatar May 22 '24 14:05 rj76