Add failure reason to messaging NSError #5511
Conversation
| NSError *error = [NSError fcm_errorWithCode:FIRMessagingErrorInvalidRequest | ||
| userInfo:@{NSLocalizedDescriptionKey : description}]; | ||
| NSError *error = [NSError | ||
| messagingErrorWithCode:(FIRMessagingInternalErrorCode)FIRMessagingErrorInvalidRequest |
There was a problem hiding this comment.
Casting to FIRMessagingInternalErrorCode doesn't look nice and leads to a couple questions:
- If we don't care of the actual code why we require it in the method interface
- If we do care of the code, then after casting from an unrelated enum the code may actually change meaning.
I wonder if there is a reason for this type casting?
There was a problem hiding this comment.
- I guess we don't have to require in the method interface, so should I just use NSUInteger?
- This was the old structure where somehow we kept two list of error codes, an internal and an external. My guess is adding an external requires API change, also external one is the one will be exposed to external API. I also notice the internal one makes a copy of the external one for easy casting.
I think we should deprecate the external error codes as they don't really mean anything to users, especially once failure reason is set and shown, the error doesn't even show the error code anymore, which is much clearer to users.
WDYT?
There was a problem hiding this comment.
Specific codes for specific errors can be useful for the case when developers can actually react to them. Taking this into account I would suggest to reduce amount of public error codes to the cases that can be handled differently. Ideally, the public codes should go through API review.
As for NSUInteger, I'm a bit hesitant doing it since usually error codes are meaningful within an error domain. Can we just use a code from FIRMessagingInternalErrorCode enum so we don't have to cast and can be sure that the code is interpreted properly in the proper domain?
There was a problem hiding this comment.
You mean using internal code, but has a public code with the same value to represent the meaning of it?
Because if we use internal code, we are not actually using the public one at all.
There was a problem hiding this comment.
I've updated to use internal one only (renamed to FIRMessagingErrorCode)
… in the name convention
|
Submit for now for hackweek and let's talk more on the public error code |
Currently Messaging returns a NSError with an internal error code (integer) and they don't mean anything to developers and very hard to track the cause. e.g. when subscribing to a empty topic, it returns error code 8.
Add failure reason to each error to what went wrong.
Also remove unused the error code.
At some point we should deprecate the FIRMessagingErrorCode as many is unused and not very useful for developers to debug.