Adding a new Listeners API #153

jguz-pubnub · 2024-02-13T13:24:24Z

feat(listeners): adding the new Listeners API
feat(subscribe & presence): enabling EventEngine by default

parfeon

Mostly looks good, but there is a question on ability to have multiple listeners on Subscription, SubscriptionSet and PubNub client. For example, one Subscription instance passed in one view between components in it — there is a chance that multiple components would like to have their listener on the same event type.

parfeon · 2024-02-16T16:26:19Z

Sources/PubNub/EventEngine/Subscribe/Helpers/SubscribeInput.swift

+    channels: [PubNubChannel],
+    presenceChannelsOnly: [PubNubChannel],
+    groups: [PubNubChannel],
+    presenceGroupsOnly: [PubNubChannel]
+  ) -> SubscribeInput.RemovingResult {


Can you subscribe solely on presence channels with the latest setup?

I preserved existing behaviors in SDK:

You cannot subscribe to Presence channel without main channel

You can unsubscribe from Presence channel only and still keep main channel

The helper method you highlighted is responsible for returning the new channels and groups due to unsubscribing. I can change its signature to be more meaningful:

func removing( mainChannels: [PubNubChannel] presenceOnlyChannels: [PubNubChannel] mainGroups: [PubNubChannel], presenceOnlyGroups: [PubNubChannel] )

Sources/PubNub/Events/New/Entities/EntitySubscribable.swift

parfeon · 2024-02-16T16:32:47Z

Sources/PubNub/Events/New/EventEmitter.swift

+  /// A closure to be called when the connection status changes.
+  var onConnectionStateChange: ((ConnectionStatus) -> Void)? { get set }
+  /// A closure to be called when a subscription error occurs.
+  var onSubscribeError: ((PubNubError) -> Void)? { get set }


Is this one here for some form of backward compatibility? If there were an error, then depending on from the current state, we would receive different connection statuses: connection error or unexpected disconnect.

Correct, that's because of the old implementation. You're right, it should be enough to read an underlying error from ConnectionStatus enum cases like connectionError and disconnectedUnexpectedly and getting rid of onSubscribeError closure you pointed out. So what I would suggest is improving existing ConnectionStatus like this:

enum ConnectionStatus { case connectionError(PubNubError) case disconnectedUnexpectedly(PubNubError) }

The reason I haven't done it yes is that there's disconnectedUnexpectedly case introduced a long time ago before EE. Now it's time to change it even if it might be a breaking change. I will run it by the team.

parfeon · 2024-02-16T21:45:52Z

Sources/PubNub/Events/New/Subscription.swift

+    let pattern = "^" + self + "$"
+    let predicate = NSPredicate(format: "SELF MATCHES %@", pattern)
+
+    return predicate.evaluate(with: string)


NSPredicate is a powerful tool, but in a way how it used may affect performance compared to regular compare as on line 192.

parfeon · 2024-02-16T21:54:02Z

Sources/PubNub/Subscription/Strategy/LegacySubscriptionSessionStrategy.swift

+    channels: [PubNubChannel],
+    presenceChannelsOnly: [PubNubChannel],
+    groups: [PubNubChannel],
+    presenceGroupsOnly: [PubNubChannel]


Would it be possible to subscribe on presence channels only with the current setup?

The rules are still the same as I described in this comment. It's a helper method only I can always rename. In general, unsubscribing still works like that:

If you unsubscribe from main channel, you should also unsubscribe from its Presence channel

If you unsubscribe from Presence channel only, you should still keep main channel in the Subscribe loop

I noticed it's easier to keep these lists in separate parameters when performing next computations.

parfeon · 2024-02-16T22:02:35Z

Sources/PubNub/Subscription/SubscriptionSession.swift

+    let channelSubscriptions = channels.map {
+      channel($0).subscription(
+        queue: queue,
+        options: withPresence ? ReceivePresenceEvents() : SubscriptionOptions.empty()
+      )
+    }
+    let channelGroupSubscriptions = groups.map {
+      channelGroup($0).subscription(
+        queue: queue,
+        options: withPresence ? ReceivePresenceEvents() : SubscriptionOptions.empty()
+      )
+    }
+    internalSubscribe(
+      with: channelSubscriptions,
+      and: channelGroupSubscriptions,
+      at: cursor?.timetoken
    )
+    channelSubscriptions.forEach { subscription in
+      subscription.subscriptionNames.flatMap { $0 }.forEach {
+        globalChannelSubscriptions[$0] = subscription
+      }
+    }
+    channelGroupSubscriptions.forEach { subscription in
+      subscription.subscriptionNames.flatMap { $0 }.forEach {
+        globalGroupSubscriptions[$0] = subscription
+      }
+    }


As I understand, this one is for PubNub client instance itself?
If subscribing to multiple entities, then it will be more reasonable to create SubscriptionSet with Subscriptions.

I ran it by the team. You're right that this should be kept in SubscriptionSet under the hood. Unfortunately, Swift SDK is the only one that provides functionality like unsubscribing from Presence channels only. And this is what prevents us from using SubscriptionSet for global subscriptions. We have no API/method for SubscriptionSet to unsubscribe from Presence channel only. I could deprecate this method or even remove withPresence parameter, but this decision requires PM input. Can we leave it as it is and fix it in case of further decisions?

Co-authored-by: Serhii Mamontov <[email protected]>

* Removed onSubscribeError closure * Added associaded type for .connectionError and .disconnectedUnexpectedly

* Modifying clone methods for Subscription and SubscriptionSet * Minor names changing for helper functions

jguz-pubnub · 2024-02-19T13:57:46Z

Regarding your comment here, I slightly modified clone() methods for Subscription and SubscriptionSet. Let's suppose that the user would like to pass the same Subscription object to another screen, but have different handling for things like onMessage, onPresence, etc, and not to override any settings from the parent listeners it derives from. Here's the example and it prints "Parent" and "Child" since these two Subscriptions below are independent:

let sub = pubnub.channel("someChannel").subscription() 
sub.onMessage = { _ in
  print("Parent")
}
sub.subscribe()

let screen = SomeScreen(subscription: sub.clone()) // <==== CLONING TAKES PLACE
self.present(screen, animated: true)

class SomeScreen: UIViewController {
  let subscription: Subscription
  
  func viewDidLoad() {
    super.viewDidLoad()
    subscription.onMessage = { _ in
       print("Child")
    } 
  }
}

The clone() method automatically registers its cloned instance in a container that tracks active subscriptions for the given channel, assuming that someone has already called .subscribe() on a parent subscription before, so now it's active. This means that the user doesn't have to call subscription.subscribe() inside the viewDidLoad() method.

parfeon

There are some naming questions and one about whether proper type used in method signature or not.

About multiple listeners of the same type on a single Subscription / SubscriptionSet is possible only through clone() utilization, and impossible to have multiple onMessage listeners on the same instance.

Sources/PubNub/EventEngine/Subscribe/Helpers/SubscribeInput.swift

parfeon · 2024-02-19T22:30:04Z

Sources/PubNub/Events/New/Subscription.swift

+    if let firstIndex = entityName.lastIndex(of: "."), let secondIndex = string.lastIndex(of: ".") {
+      return entityName.prefix(upTo: firstIndex) == string.prefix(upTo: secondIndex)
+    }


Isn't the "original" NSPredicate basically matched the same what is done on the line #192?

Yeah, it should. It compares two String up to the last occurrence of ., assuming both contain the . character. Let's suppose these scenarios:

The message came from channel.item.x and the underlying entity's subscription is channel.item.*:

According to the line 195, I will compare channel.item with channel.item ✅

The message came from a.b.c and the underlying entity's subscription is channel.item.*

I will compare a.b with channel.item 🔴

Ok, I finally got it.
I was totally fine with extension for String - it looked better.

I probably would use payload.subscription ?? payload.channel to match both channel and channelGroup subscription type. If a subscription has been created for wildcard subscription, then the subscription field will hold the same name (if an event for one of the channels from it) and will be no need for pattern match (just plain ==). The only thing which may require some attention is presence channels / channel group names.

Sources/PubNub/Subscription/Strategy/SubscriptionSessionStrategy.swift

techwritermat

enable event engine please :)

* Fixes in unit tests after enabling EE by default

jguz-pubnub · 2024-02-21T10:22:05Z

@pubnub-release-bot release as 7.0.0

pubnub-release-bot · 2024-02-21T11:05:49Z

🚀 Release successfully completed 🚀

jguz-pubnub requested review from parfeon, wkal-pubnub and marcin-cebo as code owners February 13, 2024 13:24

feat(listeners): adding a new Listeners API

d0ec8a3

jguz-pubnub force-pushed the feat/new-listeners branch from 734f78e to d0ec8a3 Compare February 13, 2024 13:54

Replacing closures names

2a618a7

parfeon reviewed Feb 16, 2024

View reviewed changes

jguz-pubnub and others added 5 commits February 18, 2024 21:08

Update Sources/PubNub/Events/New/Entities/EntitySubscribable.swift

72f55ad

Co-authored-by: Serhii Mamontov <[email protected]>

Update Sources/PubNub/Events/New/Entities/EntitySubscribable.swift

f6298bf

Co-authored-by: Serhii Mamontov <[email protected]>

Getting rid of Predicate

bdbcaf5

PR fixes

413683e

* Removed onSubscribeError closure * Added associaded type for .connectionError and .disconnectedUnexpectedly

PR fixes

8b9ae38

* Modifying clone methods for Subscription and SubscriptionSet * Minor names changing for helper functions

Minor fix in clone() method in SubscriptionSet

0b9dfef

parfeon approved these changes Feb 19, 2024

View reviewed changes

techwritermat reviewed Feb 20, 2024

View reviewed changes

jguz-pubnub added 3 commits February 20, 2024 15:16

Refactor some names for the new Listeners API

0c8c389

Enabling EventEngine for Presence & Subscribe

36dcec4

* Removed retain cycles in SubscribeRouterTests

50654d9

* Fixes in unit tests after enabling EE by default

PubNub SDK 7.0.0 release.

4fcd086

jguz-pubnub merged commit d5c5083 into master Feb 21, 2024
10 checks passed

jguz-pubnub deleted the feat/new-listeners branch February 21, 2024 10:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a new Listeners API #153

Adding a new Listeners API #153

jguz-pubnub commented Feb 13, 2024 •

edited

Loading

parfeon left a comment

parfeon Feb 16, 2024

jguz-pubnub Feb 18, 2024

parfeon Feb 16, 2024

jguz-pubnub Feb 18, 2024 •

edited

Loading

parfeon Feb 16, 2024

parfeon Feb 16, 2024

jguz-pubnub Feb 18, 2024

parfeon Feb 16, 2024

jguz-pubnub Feb 19, 2024

jguz-pubnub commented Feb 19, 2024 •

edited

Loading

parfeon left a comment •

edited

Loading

parfeon Feb 19, 2024

jguz-pubnub Feb 20, 2024

parfeon Feb 20, 2024

techwritermat left a comment

jguz-pubnub commented Feb 21, 2024

pubnub-release-bot commented Feb 21, 2024

Adding a new Listeners API #153

Adding a new Listeners API #153

Conversation

jguz-pubnub commented Feb 13, 2024 • edited Loading

parfeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jguz-pubnub Feb 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jguz-pubnub commented Feb 19, 2024 • edited Loading

parfeon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

techwritermat left a comment

Choose a reason for hiding this comment

jguz-pubnub commented Feb 21, 2024

pubnub-release-bot commented Feb 21, 2024

jguz-pubnub commented Feb 13, 2024 •

edited

Loading

jguz-pubnub Feb 18, 2024 •

edited

Loading

jguz-pubnub commented Feb 19, 2024 •

edited

Loading

parfeon left a comment •

edited

Loading