-
Notifications
You must be signed in to change notification settings - Fork 331
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
Add root error info to public error #4680
base: main
Are you sure you want to change the base?
Conversation
I want to test this more and add some tests next week, but I wanted to get some feedback, specially from iOS folks, to make sure this makes sense and if there are other ideas to expose the root StoreKit error if any to developers 🙏 |
@@ -39,7 +40,29 @@ extension PurchasesError { | |||
/// let errorCode = error as? ErrorCode | |||
/// ``` | |||
var asPublicError: PublicError { |
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.
My main concern with this approach is whether we only expose PublicError everywhere and/or if we use this method somewhere else... Need to research it a bit more.
var rootErrorInfo = [ | ||
"code": rootNSError.code, | ||
"domain": rootNSError.domain, | ||
"localizedDescription": rootNSError.localizedDescription, | ||
] as [String : Any] | ||
if #available(iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0, visionOS 1.0, *) { | ||
if let storeKitErrorInfo = self.getStoreKitErrorInfoIfAny(error: rootError) { | ||
rootErrorInfo = rootErrorInfo.merging(["storeKitError": storeKitErrorInfo]) | ||
} | ||
} | ||
let userInfoToUse = self.userInfo.merging([ErrorDetails.rootErrorKey: rootErrorInfo]) | ||
return NSError(domain: Self.errorDomain, code: self.errorCode, userInfo: userInfoToUse) |
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 am afraid we might lose some keys because of an unwanted overlap. I would just add a prefix like rc_
to any new key we add to:
- be explicit on what we're adding
- avoid dropping useful data.
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.
Right, I added everything within a rc_root_error
which I felt was enough as a namespace... I don't think it's worth adding to all nested properties then? Lmk if you think otherwise though!
case let .networkError(urlError): | ||
return resultMap.merging([ | ||
"urlErrorCode": urlError.errorCode, | ||
"urlErrorFailingUrl": urlError.failureURLString ?? "" |
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 we can add an explicit missing_value
, otherwise people will think we missed it
} else if let storeKitError = error as? StoreKit.Product.PurchaseError { | ||
return ["description": storeKitError.trackingDescription] | ||
} else { | ||
return nil |
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 guess it would be useful to log what other error type we might be missing? 🤔
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.
Right, in this case I'm assuming it's because the root error is not a store kit error (which may happen in several situations). I could add a log here, but I think the situation may happen relatively often... Note that this method is only to add extra data to the userInfo
map in case it's actually a StoreKit error.
@@ -39,7 +40,29 @@ extension PurchasesError { | |||
/// let errorCode = error as? ErrorCode | |||
/// ``` | |||
var asPublicError: PublicError { |
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 know it's a draft, but it would be great to list the keys we'll include in userInfo
so people can just read it from here without going through the code
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.
Hmm so I included it in the docs for the asPublicError
, but that's actually internal... I'm trying to figure out a better way to explain this, but maybe this is something we should add to the docs, for people that care to get the root error.
Description
This is a possible approach to add root error info to public errors exposed by the SDK. Many times, we have nested errors, which makes discoverability of the root error difficult. This also hides actual StoreKit errors from being easily accessible, which some devs have asked about.
This PR adds a new property to the userInfo of the public error exposed by the SDK including relevant information about the root error. If the root error is a type of StoreKitError, we add some additional information related to that.
For example, when simulating an error getting products during the
getOfferings
call using StoreKit, a new property in userInforc_root_error
will be added with the following structure:or