Dont use pendingEventsDispatcher with user defined eventDispatcher #289
Conversation
2 similar comments
jordangarcia
force-pushed
the
jordan/only-wrap-default-event-dispatcher-pending
branch
from
9d1d9de
to
1ed3859
Compare
| }); | ||
|
|
||
| describe('when an eventDispatcher is passed in', function() { | ||
| it('should wrap the default eventDispatcher and invoke sendPendingEvents', function() { |
There was a problem hiding this comment.
Should say 'should not wrap...' right?
| if (config.eventDispatcher == null) { // eslint-disable-line eqeqeq | ||
| // only wrap the event dispatcher with pending events retry if the user didnt override | ||
| eventDispatcher = new eventProcessor.LocalStoragePendingEventsDispatcher({ | ||
| eventDispatcher: defaultEventDispatcher, |
There was a problem hiding this comment.
Given that we we no longer allow a custom eventDispatcher to be used with LocalStoragePendingEventsDispatcher, does passing eventDispatcher to LocalStoragePendingEventsDispatcher still make sense?
There was a problem hiding this comment.
Hm, you mean the idea of having an event dispatcher that wraps another event dispatcher.
Probably isn't the most ideal design, but I dont hate it. If we just made this the default event dispatcher, without wrapping it, then we'd be replacing the existing defaultEventDispatcher implementation. That could be a bit strange for people using the eventDispatcher in a custom manner.
When we made the decision to only wrap the defaultEventDispatcher, I did feel a bit safer about this pattern then replacement.
WDYT?
There was a problem hiding this comment.
That makes sense to me. Initially, I thought this wrapper pattern was weird given that it only ever wraps one thing, but it is safe, and I like that pending-events-related logic is isolated.
jordangarcia
force-pushed
the
jordan/only-wrap-default-event-dispatcher-pending
branch
from
9744b09
to
9a351c0
Compare
jordangarcia
force-pushed
the
jordan/only-wrap-default-event-dispatcher-pending
branch
from
9a351c0
to
e25f922
Compare
mikeproeng37
deleted the
jordan/only-wrap-default-event-dispatcher-pending
branch
Summary
The is an edge case where the
sendPendingEvents()mechanism keeps sending events because a custom eventDispatcher never invoked thecallback()denoting the request completed. This leads to events piling up in localStorage and sending every event each time the SDK is initialized.To prevent this only use the pendingEventDispatcher if the default event dispatcher is used, since this guarantees the callback is properly called.
Test plan
Issues