Skip to content

Add PushDeliveryTarget - #5

Merged
GErP83 merged 2 commits into
mainfrom
feature/beta2
Aug 19, 2026
Merged

Add PushDeliveryTarget#5
GErP83 merged 2 commits into
mainfrom
feature/beta2

Conversation

@GErP83

@GErP83 GErP83 commented Aug 18, 2026

Copy link
Copy Markdown
Contributor
  • 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

@GErP83
GErP83 requested review from tib and viaszkadi August 18, 2026 16:23

@tib tib left a comment

Copy link
Copy Markdown
Member

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 ? 🤔

@tib

tib commented Aug 18, 2026

Copy link
Copy Markdown
Member

+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...

@GErP83 GErP83 changed the title Add TopicPushClient Add PushDeliveryTarget Aug 19, 2026
@GErP83
GErP83 requested review from tib and viaszkadi August 19, 2026 06:22
@GErP83
GErP83 merged commit 10b6c4e into main Aug 19, 2026
31 of 34 checks passed
@GErP83
GErP83 deleted the feature/beta2 branch August 19, 2026 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants