-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
Rework WebPushService #48
base: main
Are you sure you want to change the base?
Conversation
Introduces a Notification data class representing a web push notification to be sent by a WebPushService. Aims to replace the overloaded multi-parameter methods of WebPushService with a single one, deprecating the old ones to avoid repetition of documentation. Updates the README and BrowserTest to remove references to the deprecated methods.
Allows subclassing of WebPushService and implementation using custom HTTP clients while keeping the same interface to send the web push notifications. Also contributes to interaso#47 by moving JDK specific code into JdkHttpClientWebPushService.
…eptions WebPushStatusException added a statusCode property to provide additional context when handling web push-related exceptions. However, handling of certain responses require further information from the HTTP response than just the status code (e.g., handling a 429 Too Many Requests response using the Retry-After header) which cannot be obtained from WebPushStatusException. Since WebPush.getSubscriptionState does not have access to the (implementation-specific) HTTP response object, throwing more concrete WebPushException subclasses must be the task of the WebPushService implementation.
Hi @andreblanke, thank you very much for the work you invested in this. I don't have enough time to fully review this now, but some points that come to my mind instantly:
|
Thanks for your quick reply! It's understandable you can't immediately review all the changes.
I agree that a new major version would be appropriate. Although the only breaking change (leaving aside the deprecations) would be the instantiation of the Before: val webPushService: WebPushService = WebPushService(subject, vapidKeys) After: val webPushService: WebPushService = JdkHttpClientWebPushService(subject, vapidKeys) It might also make sense to add a static factory method such as
The simplest option would be to add the HTTP headers to the Alternatively, the approach taken by 5d4f7e4 could be used standalone without separating the interface and implementation of
I would like to back off from sending push notifications if the provider responds with an HTTP 429 Too Many Requests status which is usually accompanied by a
I think it'd make sense to have this as functionality within the library itself, yes. |
Hi there again. I am proposing a few more relatively closely related changes to the API of the
WebPushService
class. Namely:a7a8f08 which adds a
Notification
class to hold all data for a web push notification along with aWebPushService.send(Notification)
method. This reduces the amount of method parameters, simplifies the interface of theWebPushService
class, and allows passing around a web push notification as one coherent entity.The multi-parameter
send
methods are kept for now, but deprecated with a suggested replacement. The example in the README has been updated accordingly.7982c15 which turns
WebPushService
into an abstract class whichJdkHttpClientWebPushService
inherits from as an implementation. Like this, code can be written against the interface ofWebPushService
and the underlying implementation be swapped out, e.g., when a custom HTTP client should be used. This is also a step into the direction of Make this library multiplatform #47 since the JDKHttpClient
-related code is isolated.5d4f7e4 which changes the exception handling again because it turned out the
WebPushStatusException
was still not enough for my use case (sorry for that) since I require access to the HTTP response headers as well (e.g.,Retry-After
when an HTTP 429 response is returned), whichWebPush.getSubscriptionState
cannot provide.The method was changed to throw the more generic
WebPushException
again andWebPushStatusException
was removed. Instead, implementations ofWebPushService
should be able to throw their own exceptions inheriting fromWebPushException
which provide more context such as the concrete HTTP response object that caused the exception to be thrown. This new exception class would beJdkHttpClientWebPushService.WebPushHttpException
.Instead of
WebPushService
implementation-specific exception types theWebPushStatusException
could be extended to include aresponse: Any? = null
field, but that felt really unclean.I realize that quite a bit of library code is touched by my changes, but it'd be great if you consider including them.