-
Notifications
You must be signed in to change notification settings - Fork 188
Add new logger module #466
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
base: master
Are you sure you want to change the base?
Conversation
Replace the legacy logger with the new logger module with a bundled console logger with a unified (with other SDKs) logs output format. BREAKING CHANGES: Legacy logger removed and logging to file disabled by default. feat(logger): provide interface for custom loggers Provide interfaces for custom logger implementation. feat(retry-policy): retry policy moved to the network module Retry policy can be applied for all API endpoint groups (not only `subscribe`). refactor(deprecated): removed `packetSizeForOperation` Removed previously deprecated `packetSizeForOperation` methods group along with `size` builder from the `PubNub` client. refactor(deprecated): removed `retry` from status object Removed previously deprecated `retry` and `cancelAutomaticRetry` methods along with the `automaticallyRetry` property from API processing status objects. BREAKING CHANGES: To retry the last API call, same request object to the corresponding method. refactor(deprecated): removed obsolete properties from `status` and `result` objects. Removed previously deprecated properties: `TLSEnabled`, `authKey`, `origin`, `userID`, `uuid`, and `statusCode`. BREAKING CHANGES: Most of the removed values can be retrieved from the client configuration, and the API processing HTTP status code was removed in favor of `category` fields. refactor(deprecated): removed deprecated configuration options Removed previously deprecated `uuid`, `deviceID`, `applicationExtensionSharedGroupIdentifier`, and `completeRequestsBeforeSuspension` configuration options. BREAKING CHANGES: `uuid` has been removed in favor of the `userID` property.
🎉 Snyk checks have passed. No issues have been found so far. |
Add `limit` and `offset` parameters for `PNHereNowRequest` for pagination support.
Fix the issue where an immediate 'unsubscribe' after receiving a response from a subscription may cause a race condition where the next 'subscribe' is in the process of scheduling and can't be cancelled, causing 'leave' to time out. fix(cocoapods): address classes visibility Fix the issue because of which private classes leaked into public headers and the CocoaPod build failed.
// | ||
// PubNub Real-time Cloud-Hosted Push API and Push Notification Client Frameworks | ||
// Copyright (c) 2013 PubNub Inc. | ||
// Copyright (c) 2013 PubNub Inc./Users/sergey/Documents/Develop/Objective-C/PubNub SDK (master)/PubNub.podspec |
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.
It seems that custom path has been accidentally inserted here
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.
Good catch!
.macOS(.v10_11), | ||
.tvOS(.v10), | ||
.watchOS(.v4) | ||
.iOS(.v14), |
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.
I like that the deployment targets were increased, but I’m curious - was there a specific reason or change in this PR that required increasing them?
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.
Actualize it with deployment targets in PubNub.podspec
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.
It seems that deployment targets haven't been updated in the PubNub.podspec
file as you did in Package.swift
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.
But they are.. I didn't touch section in PubNub.podspec but now Package.swift
finally has the same (after 2y I've finally synced Package.swift
with PubNub.podspec
).
} | ||
|
||
- (void)channelsForGroup:(NSString *)group withCompletion:(PNGroupChannelsAuditCompletionBlock)block { | ||
[self.logger warnWithLocation:@"PubNub" andMessageFactory:^PNLogEntry * { |
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.
Should we log deprecated method calls? The caller will already receive a warning message in the IDE (Xcode)
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.
I was struggling with this decision. It is possible to silence any kind of IDE warnings in settings or during pod install
in Podfile
. If it is excessive, I'll remove it.
return [PNStringLogEntry entryWithMessage:@"Unsubscribe all channels and groups"]; | ||
}]; | ||
|
||
dispatch_queue_t queue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0); |
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 need dispatch_after
here?
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.
There were two approaches:
- Update the transport interface to include functions that will handle race conditions.
- Make a minimum delay to offset events in time (because there is no guarantee that the custom transport module will be properly implemented).
This code is present only in places where cancellation of the active subscription is done (in response to subscribe
or unsubscribe
method calls). The main issue is that unsubscribe
(or presence leave
) may time out because it is sent using the same transport (NSURLSession
which allows only 1 concurrent request to the origin at a time) that is used for subscribe. The reason it happens is that the long-poll subscribe request could be sent earlier than leave
which will cause a timeout (long-poll won't return in 10 seconds unless channels / groups are really active).
Issue reproducible when in subscribe
/ message
status listener user will call unsubscribe
.
I have a general question: should we really add separate logs when a caller invokes a deprecated method? This occurs in many places, and the IDE should already provide a warning. I don't think this should be the logger mechanism's responsibility |
feat(logger): add new logger module
Replace the legacy logger with the new logger module with a bundled console logger with a unified (with other SDKs) logs output format.
BREAKING CHANGES: Legacy logger removed and logging to file disabled by default.
feat(logger): provide interface for custom loggers
Provide interfaces for custom logger implementation.
feat(here-now): add
limit
andoffset
parametersAdd 'limit' and 'offset' parameters for 'PNHereNowRequest' for pagination support.
feat(retry-policy): retry policy moved to the network module
Retry policy can be applied for all API endpoint groups (not only
subscribe
).refactor(deprecated): removed
packetSizeForOperation
Removed previously deprecated
packetSizeForOperation
methods group along withsize
builder from thePubNub
client.refactor(deprecated): removed
retry
from status objectRemoved previously deprecated
retry
andcancelAutomaticRetry
methods along with theautomaticallyRetry
property from API processing status objects.BREAKING CHANGES: To retry the last API call, same request object to the corresponding method.
refactor(deprecated): removed obsolete properties from
status
andresult
objects.Removed previously deprecated properties:
TLSEnabled
,authKey
,origin
,userID
,uuid
, andstatusCode
.BREAKING CHANGES: Most of the removed values can be retrieved from the client configuration, and the API processing HTTP status code was removed in favor of
category
fields.refactor(deprecated): removed deprecated configuration options
Removed previously deprecated
uuid
,deviceID
,applicationExtensionSharedGroupIdentifier
, andcompleteRequestsBeforeSuspension
configuration options.BREAKING CHANGES:
uuid
has been removed in favor of theuserID
property.