Removing SubscribeSessionFactory and SubscriptionConfiguration #162
Conversation
jguz-pubnub
requested review from
parfeon,
wkal-pubnub and
marcin-cebo
as code owners
| private extension DependencyContainer { | ||
| func resolveSession(session: SessionReplaceable, with operators: [RequestOperator?]) -> SessionReplaceable { | ||
| session.defaultRequestOperator == nil ? session.usingDefault(requestOperator: MultiplexRequestOperator( | ||
| operators: operators.compactMap { $0 } |
There was a problem hiding this comment.
This is a private extension and can be sure of what we are passing, do we really need to map values again? We did it once when called resolveSession.
Can we leave compactMap only in resolveSession and just pass arrays from defaultHTTPSession, httpSubscribeSession, and httpPresenceSession?
There was a problem hiding this comment.
Indeed, you're right. I will send you the updated version shortly.
| container.register(value: session, forKey: DefaultHTTPSessionDependencyKey.self) | ||
| container.register(value: subscribeSession, forKey: HTTPSubscribeSessionDependencyKey.self) | ||
| container.register(value: fileSession, forKey: FileURLSessionDependencyKey.self) |
There was a problem hiding this comment.
Actually, this one bothered me for a pretty long time: what further actions should be done if session will get invalidated (there are numerous non-manual reasons for it)?
Same with session which is stored in container by default, how invalidation handled for it?
There was a problem hiding this comment.
The concrete type for subscribe session, presence session, and regular session is the HTTPSession class. These values are retrieved from DependencyContainer in the PubNub's constructor . HTTPSession is a class (reference) type, so there's the same reference in the DependencyContainer and later in the PubNub class after assigning any session to the appropriate field. And also, what I'm considering right now after your comment is to store weak references in DependencyContainer and checking what's the impact. The idea is that you have to keep the value you resolved under the strong reference. For example, this is what happens in PubNub class.
You're right that for example, current networkSession used for regual requests different than subscribe/heartbeat is exposed as a public property. It means that the caller might invoke invalidateAndCancel() method that was designed in SessionReplaceable/HTTPSession and the consequence is that you cannot reuse such a session. On the other hand, this happens only on explicit caller's request and we can't do anything about it. They should be aware that creating the PubNub client from scratch is necessary in this use case.
refactor(subscribe): removing
SubscribeSessionFactorywithSubscriptionConfigurationrefactor(subscribe): making
SubscriptionSessionclass aninternal