-
Notifications
You must be signed in to change notification settings - Fork 133
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
fix: Bug with logEvent callbacks not being called when unsent events are dropped #342
Changes from all commits
b767fc6
afa7907
30f52d8
8d67d5b
9fc5e5b
5235439
86d5e6e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1384,7 +1384,14 @@ var _generateApiPropertiesTrackingConfig = function _generateApiPropertiesTracki | |
*/ | ||
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); | ||
deletedEvents.forEach((event) => { | ||
if (type(event.callback) === 'function') { | ||
event.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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this is relatively long compared to our other There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe that if it's only in the util logs, users won't be able to have failed upload event handling via callback |
||
}); | ||
} | ||
}); | ||
} | ||
}; | ||
|
||
|
@@ -1394,6 +1401,7 @@ AmplitudeClient.prototype._limitEventsQueued = function _limitEventsQueued(queue | |
* @callback Amplitude~eventCallback | ||
* @param {number} responseCode - Server response code for the event / identify upload request. | ||
* @param {string} responseBody - Server response body for the event / identify upload request. | ||
* @param {object} details - (optional) Additional information associated with sending event. | ||
*/ | ||
|
||
/** | ||
|
@@ -1684,7 +1692,7 @@ AmplitudeClient.prototype.sendEvents = function sendEvents() { | |
// here. | ||
// } | ||
} catch (e) { | ||
// utils.log('failed upload'); | ||
// utils.log.error('failed upload'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's this line for if commented out? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was commented out before, didn't want to touch it 😂 I just made a change to uncomment it |
||
} | ||
}); | ||
}; | ||
|
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.
is
this.options.savedMaxCount
guaranteed to be set?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.
Yup, the default config for it is 1000