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
Storage hardening; Add error handling capabilities. #163
Conversation
f5f2eb3
to
9e85e20
Compare
fd25215
to
81cc70b
Compare
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.
Left a few questions and small comments, but overall I think this looks good!
@@ -95,5 +99,16 @@ public extension Configuration { | |||
values.cdnHost = value | |||
return self | |||
} | |||
|
|||
@discardableResult | |||
func errorHandler(_ value: @escaping (Error) -> Void) -> Configuration { |
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.
Just curious as to why it returns the Configuration
?
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.
The config methods are all chainable and this continues that. Makes is so you can do ...
let analytics = Configuration(writeKey: "1234")
.trackLifecycleEvents(true)
.apiHost("eu1.segmentapi.com")
.whateverElse()
.. instead of having to create an instance and then set them all afterwards.
Sources/Segment/Errors.swift
Outdated
} | ||
|
||
/// Reports an internal error to the user-defined error handler. | ||
public func reportInternalError(_ error: Error, hardStop: Bool = false) { |
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.
nit: maybe critical
instead of hardStop
sounds more exception-like?
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 call.
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.
named it fatal
instead as it seemed more descriptive of what was going to happen, ie: YOUR APP WILL DIE.
// flushInterval and flushAt can be modified post initialization | ||
analytics.store.subscribe(self, initialState: true) { [weak self] (state: System) in | ||
guard let self = self else { return } | ||
self.flushTimer = QueueTimer(interval: state.configuration.values.flushInterval) { [weak self] in |
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.
Not sure how this works in Swift but wouldn't it also have to cancel the previous timer?
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.
QueueTimer is a wrapper on a dispatch source timer. Cancel is handled in dealloc.
errorHandler
to configuration so developer can receive errors we see internally.Affects #154 and #162.