-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Add Mixpanel Session Replay settings support #52
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
base: development
Are you sure you want to change the base?
feat: Add Mixpanel Session Replay settings support #52
Conversation
src/MixpanelEventForwarder.js
Outdated
| } | ||
| } | ||
|
|
||
| if (forwarderSettings.recordHeatmapData != null) { |
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.
Seems like there's primarily 2 patterns the settings are following here, either if something is a boolean or if it's a number.
I think you can probably do something like
const boolSessionReplaySettings = ['recordHeatmapData', 'autocapture', etc, etc]
const numberSessionReplaySettings = ['recordIdleTimeoutMs', 'recordMaxMs', etc, etc]
boolSessionReplaySettings.forEach(function(boolSetting) {
if (boolSetting != null) { initOptions[boolSetting] = boolSetting === 'True'}
})
// something similar for the numberSessionReplaySettings
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.
Used dictionaries with { key, mappedKey } since forwarderSettings use camelCase (recordHeatmapData), but Mixpanel's API expects snake_case (record_heatmap_data).
src/MixpanelEventForwarder.js
Outdated
| !isNaN(sessionPercent) && | ||
| sessionPercent >= 0 && | ||
| sessionPercent <= 100 |
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.
I would argue we should just have Mixpanel do the validation on their side and not worry about it on our side.
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.
Updated
5f3b1de to
f2c2ed0
Compare
|
|
||
| // Process string settings | ||
| stringSettings.forEach(function (setting) { | ||
| if (forwarderSettings[setting.key]) { |
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.
Any reason why you're not checkign != null here?
If this was missed, we could make a simple utility function to make this a little readable.
function hasValue(obj, key) {
return obj[key] != null;
}
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.
I didn't add != null for string settings as it would allow empty strings like recordMaskTextSelector: '' to pass through to Mixpanel, which would break test like "should ignore empty string values for privacy selectors".
src/MixpanelEventForwarder.js
Outdated
| var numericValue = parseInt( | ||
| forwarderSettings[setting.key], | ||
| 10 | ||
| ); | ||
| if (!isNaN(numericValue)) { | ||
| initOptions[setting.mappedKey] = numericValue; | ||
| } |
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.
Extra credit: Maybe we can extract this logic into a utility function to improve readability.
function parseIntSafe(value) {
var n = parseInt(value, 10);
return isNaN(n) ? undefined : n;
}
Instructions
developmentSummary
Testing Plan
Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)