GeezerVison is showing github.com — Add PushDeliveryTarget by GErP83 · Pull Request #5 · feather-framework/feather-push
Conversation
GErP83
commented
Aug 18, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
- Use a single PushClient interface for all delivery targets
- Add
PushDeliveryTarget for device tokens and topics
- Add unsupportedTarget for providers that do not support a target type
- Improve public API documentation and error descriptions
- Update README examples and explain FCM/APNs targeting differences
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
tib
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we introduced another push client protocol?
This is already VERY confusing to me:
- PushClient -> i know i need a client to send push...
- TopicPushClient -> ???
Shouldn't we use an enum to differentiate delivery targets?
enum PushDeliveryTarget {
case deviceToken(String)
case topic(String)
}This way push client code could be:
public protocol PushClient: Sendable {
func send(
notification: PushNotification,
to target: PushDeliveryTarget
)
}Also call site would look a lot better:
pushClient.send(
notification: PushNotification,
to: .deviceToken("my-device")
)
pushClient.send(
notification: PushNotification,
to: .topic("my-topic")
)WDYT @viaszkadi ? 🤔
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
GErP83 and viaszkadi reacted with thumbs up emoji
|
+1: I'm well aware that APNS topic has to be the bundle identifier, and it's completely different what Firebase defines as topic.
Still from a framework perspective, I'd like to have 1 interface to handle things, not multiple abstraction protocols. If APNs has no support for real topics, then we're screwed anyways, so we could simply trigger a fatalError("Topics are not supported using this driver.") when matching that case...
If someone uses the FCM driver then it'll be just fine and we should be able to send to both platforms using topics...
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
tib
tib approved these changes
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.