-
Notifications
You must be signed in to change notification settings - Fork 38
Add promise checking for error case #58
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
Conversation
Add promise checking for error case
| Identity.getExperienceCloudId(new AdobeCallbackWithError<String>() { | ||
| @Override | ||
| public void fail(AdobeError error) { | ||
| promise.reject("AdobeError", error.getErrorName()); |
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 think we can also provide an extension name when calling reject in Java code.
Line 115 in 36806b1
| promise.reject(getName(), FAILED_TO_CONVERT_EVENT_MESSAGE, new Error(FAILED_TO_CONVERT_EVENT_MESSAGE)); |
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.
Rewrote it, the sample output for JAVA case is
WARN Possible Unhandled Promise Rejection (id: 0):
Error: general.extension.not.initialized
| [AEPMobileEdgeIdentity getExperienceCloudId:^(NSString * _Nullable experienceCloudId, NSError * _Nullable error) { | ||
| resolve(experienceCloudId); | ||
| if (error) { | ||
| reject(EXTENSION_NAME, FAILED_TO_CONVERT_EVENT_MESSAGE, 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.
Instead of returning FAILED_TO_CONVERT_EVENT_MESSAGE, can we read the AEPError message from error?
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.
Change to write it as this way right now
reject(EXTENSION_NAME, nil, error);
Sample output:
WARN Possible Unhandled Promise Rejection (id: 0):
Error: The operation couldn’t be completed. (AEPCore.AEPError error 1.)
If use
reject(EXTENSION_NAME, FAILED_TO_CONVERT_EVENT_MESSAGE, error);
Sample output would be generic
WARN Possible Unhandled Promise Rejection (id: 0):
Error: Failed to get Experience Cloud ID
Let me know what you think
add promise reject with error
.../main/java/com/adobe/marketing/mobile/reactnative/edgeidentity/RCTAEPEdgeIdentityModule.java
Outdated
Show resolved
Hide resolved
.../main/java/com/adobe/marketing/mobile/reactnative/edgeidentity/RCTAEPEdgeIdentityModule.java
Outdated
Show resolved
Hide resolved
.../main/java/com/adobe/marketing/mobile/reactnative/edgeidentity/RCTAEPEdgeIdentityModule.java
Outdated
Show resolved
Hide resolved
Updated for callback error
update to final for handleError parameter
emdobrin
left a comment
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.
👍 looks good @cacheung , left a few small comments for typos etc.
| #pragma mark - Helper methods | ||
|
|
||
| - (void) handleError:(NSError *) error rejecter:(RCTPromiseRejectBlock) reject errorLocation:(NSString *) location { | ||
| NSString *errorTimeOut = [NSString stringWithFormat:@"%@ call time out", location]; |
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.
typo: ... call timed out...
|
|
||
| - (void) handleError:(NSError *) error rejecter:(RCTPromiseRejectBlock) reject errorLocation:(NSString *) location { | ||
| NSString *errorTimeOut = [NSString stringWithFormat:@"%@ call time out", location]; | ||
| NSString *errorUnexpected = [NSString stringWithFormat:@"%@ all returned an unexpected error", location]; |
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.
typo: ... call returned an unexpected error ...
| if (error && error.code != AEPErrorNone) { | ||
| if (error.code == AEPErrorCallbackTimeout) | ||
| reject(EXTENSION_NAME, errorTimeOut, error); | ||
| return; |
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.
this is not needed if there is an else branch.
| } | ||
|
|
||
| if (error && error.code != AEPErrorNone) { | ||
| if (error.code == AEPErrorCallbackTimeout) |
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 is recommended to always add accolades for if block, even when having one sentence inside that if.
update with review feedback
Add promise checking for error case
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: