Repository navigation
fix: Bug with logEvent callbacks not being called when unsent events are dropped - #342
Conversation
| AmplitudeClient.prototype._limitEventsQueued = function _limitEventsQueued(queue) { | ||
| if (queue.length > this.options.savedMaxCount) { | ||
| queue.splice(0, queue.length - this.options.savedMaxCount); | ||
| const deletedEvents = queue.splice(0, queue.length - this.options.savedMaxCount); |
There was a problem hiding this comment.
is this.options.savedMaxCount guaranteed to be set?
| // } | ||
| } catch (e) { | ||
| // utils.log('failed upload'); | ||
| // utils.log.error('failed upload'); |
There was a problem hiding this comment.
what's this line for if commented out?
There was a problem hiding this comment.
Was commented out before, didn't want to touch it 😂
I just made a change to uncomment it
kelvin-lu
left a comment
kelvin-lu
left a comment
There was a problem hiding this comment.
lgtm! one nit and one conversation but overall makes sense.
| if (queue.length > this.options.savedMaxCount) { | ||
| queue.splice(0, queue.length - this.options.savedMaxCount); | ||
| const deletedEvents = queue.splice(0, queue.length - this.options.savedMaxCount); | ||
| deletedEvents.forEach((e) => { |
There was a problem hiding this comment.
nit: no single letter variable
| deletedEvents.forEach((e) => { | ||
| if (type(e.callback) === 'function') { | ||
| e.callback(0, 'No request sent', { | ||
| reason: 'Event dropped because options.savedMaxCount exceeded. User may be offline or have a content blocker', |
There was a problem hiding this comment.
nit: this is relatively long compared to our other reason strings. I appreciate the helpful direction though. How would you feel if this was in the util logs instead?
There was a problem hiding this comment.
(not necessarily saying it should be in the util logs since I don't know how visible that might be) - and leaving this as-is might be a good idea as well.
There was a problem hiding this comment.
I believe that if it's only in the util logs, users won't be able to have failed upload event handling via callback
## [7.4.1](v7.4.0...v7.4.1) (2021-01-11) ### Bug Fixes * Bug with logEvent callbacks not being called when unsent events are dropped ([#342](#342)) ([f243a92](f243a92)), closes [#142](https://github.com/amplitude/amplitude-javascript/issues/142)
4 participants
Summary
maxSavedCount, the oldest events will be dropped but the associated callback won't be called. This PR fixes thisChecklist