-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
feat: support for Goth modules for FCM #235
base: master
Are you sure you want to change the base?
feat: support for Goth modules for FCM #235
Conversation
We've confirmed that this works with the metadata approach, and have no reason to believe it wouldn't work otherwise. I can test with a service account JSON using Goth if you'd like. I can also complete any updates to this PR that you'd like. Just let me know what you'd prefer, @hpopp Original Description: TODOs
Open Questions to discuss
|
This is excellent work 🍻. Considering how long the release candidates have been out, it's not wise to drop For config validation logic, if Also, I agree with delegating all refreshing to Goth itself 👍 As far as testing, the integration tests have always been a pain point in PRs. Traditionally I've just pulled locally and run them for a sanity check before merge. I do finally need to split them out from unit tests, but that's a bit too much for this PR. |
Ok so I've had some time to think it over and the best I can think of is having Pigeon fall back to supervising a What do you think of some callback or another function on Configurable or something that can affect the supervision tree in the Dispatcher module? I was thinking |
I'm also looking to use Pigeon with Goth metadata instead of a service account and this looks to be exactly what's needed! Is there a chance this PR will be merged anytime soon or are there more changes needed? |
@hpopp any updates on this? would like to use this as well |
Hey, I apologize I forgot about this PR. Let me know if you want me to take a stab at anything. |
Finally getting a chance to catch back up to speed, I've been traveling a lot lately. I think I'm coming around on dropping the |
I'll take a look over the week at writing up some docs and seeing if there's anything I missed from discussions |
dbb0da5
to
e721c10
Compare
- add `PigeonTest.GothHttpClient.Stub` to docs - add example usage of `PigeonTest.GothHttpClient.Stub` to module docs
@hpopp, let me know what you think. I can move stuff around, or if there's preferred word-smithing, I take no offense 😅 Final question I had though was whether we should provide a deprecation warning (and whether hard at compile, or just at runtime) if they provide |
@hpopp, is there anything else you'd like to see? :) |
closes #232
Updates the
Pigeon.FCM
andPigeon.FCM.Config
implementations to delegate the token refresh logic toGoth
, as Goth v1.3+ now handles that directly