Conversation
Codecov Report
@@ Coverage Diff @@
## dev #110 +/- ##
==========================================
+ Coverage 91.19% 92.54% +1.35%
==========================================
Files 17 18 +1
Lines 590 603 +13
==========================================
+ Hits 538 558 +20
+ Misses 52 45 -7 |
Sources/Edge+PublicAPI.swift
Outdated
| source: Constants.EventSource.REQUEST_CONTENT, | ||
| data: eventData) | ||
|
|
||
| ResponseCallbackHandler.shared.registerCompletionHandler(requestEventId: event.id.uuidString, completionHandler: completion) |
There was a problem hiding this comment.
I think it would be easier if we use the EdgeResponseHandler here. Just need to define a EdgeResponseHandler which caches all the responses and error, and then call the completion on complete.
There was a problem hiding this comment.
I kept the two separated in case we want to stick with one or the other. If we keep the completion handler then we can remove EdgeResponseHandler altogether.
| // Note: ExEdge does not return eventIndex when there is only one event in the request. | ||
| // The event handles and errrors are associated to that request event, so defaulting to 0 here. | ||
| let index = forEventIndex ?? 0 | ||
| guard index >= 0, index < requestEventIdsList.count else { |
There was a problem hiding this comment.
Fixed 🐛 where the response was not returned in the callback when eventIndex was missing in the response.
|
I pushed a few more tests to support these changes and the related #95 |
Tests/FunctionalTests/AEPEdgeResponseHandlerFunctionalTests.swift
Outdated
Show resolved
Hide resolved
Tests/FunctionalTests/AEPEdgeResponseHandlerFunctionalTests.swift
Outdated
Show resolved
Hide resolved
|
Based on our discussion we decided to:
|
| /// - Parameters: | ||
| /// - forRequestEventId: unique event identifier for which the completion handler is registered; should not be empty | ||
| /// - completionHandler: the completion handler that needs to be registered, should not be nil | ||
| func registerCompletionHandler(forRequestEventId: String, completion: (([EdgeEventHandle]) -> Void)?) { |
There was a problem hiding this comment.
Is it possible to potentially register a completion handler twice for a single request id? I don't think there's a use case for this but I wonder if we should be safe and check if an existing completion handler is already mapped to the request id.
There was a problem hiding this comment.
Since this API is internal and used when sendEvent is called and a new Event is created, I think it is safe to say that there will not be multiple completion handlers associated with the same request id.
|
@kevinlind let me now if you have any additional comments, I would like to merge by the end of the day. |
Description
sendEvent(experienceEvent: ExperienceEvent, _ completion: (([EdgeEventHandle]) -> Void)? = nil)Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: