Invoke logEvent callbacks for each event when events are actually sent #253
Conversation
blazzy
force-pushed
the
callback-errors
branch
2 times, most recently
from
29c6031 to
25fd0dd
Compare
src/amplitude-client.js
Outdated
| AsyncStorage.setItem(this.options.unsentIdentifyKey + this._storageSuffix, unsentIdentifys); | ||
| } else { | ||
| this._setInStorage(localStorage, this.options.unsentIdentifyKey, JSON.stringify(this._unsentIdentifys)); | ||
| this._setInStorage(localStorage, this.options.unsentIdentifyKey, unsentIdentifys); |
There was a problem hiding this comment.
hmmm.. i worry that someone could be skimming this code and easily overlook that this. _setInStorage expects string and we're running JSON.stringify() above. Would it make sense to move the JSON.stringify() logic to this. _setInStorage?
There was a problem hiding this comment.
You make a good point about the readability. I think I should rename unsentEvents to something like serializedUnsentEvents here instead.
| this._limitEventsQueued(this._unsentIdentifys); | ||
| } else { | ||
| this._unsentEvents.push(event); | ||
| this._unsentEvents.push({event, callback}); |
| if (type(callback) === 'function') { | ||
| callback(0, 'No request sent', {reason: 'API key not set'}); | ||
| } | ||
| this.removeEvents(Infinity, Infinity, 0, 'No request sent', {reason: 'API key not set'}); |
There was a problem hiding this comment.
does this work? maybe i'm mistaken, but doesn't this break on line 1421 above?
where scope = eventQueue = Infinity and scope[eventQueue] will be undefined
There was a problem hiding this comment.
It's confusing. There is a removeEvents function bound to this and there's a _removeEvents function. I'm calling the former which invokes the latter with scope.
The SDK would invoke the logEvent callback when an event was queued. This behavior is really confusing and looks like an error state to most people. Especially since it used status code 0. This confusing behavior makes people think their instrumentation is failing and sometimes it causes them to needlessly build event queues of their own just to send data to amplitude.
With these changes, the callback is only invoked when an event is successfully sent or when the SDK gives up and decides it will never send an event.
The
_unsentEventand_unsentIdentifyqueues will now contain events with their callbacks[{ event, callback }]. Though when we serialize them to localStorage, we only serialize the events.