allow callback in time_event to modify tracked time #205
Conversation
src/mixpanel-core.js
Outdated
| if (callback && (typeof(callback) === 'function')) { | ||
| var callbacks = this['props'][EVENT_TIMERS_KEY_CALLBACK] || {}; | ||
| callbacks[event_name] = callback; | ||
| this['props'][EVENT_TIMERS_KEY_CALLBACK] = callbacks; |
There was a problem hiding this comment.
Unfortunately, persisted props need to be serializable - they end up either in a cookie or localStorage (depending on your configuration). The reason the event-timer code currently uses the persistent store is so that it can still work across page loads, but I don't think we'll be able to do that with an arbitrary callback, at least without some wonky workarounds like registering the callback code as part of mixpanel.init() configuration.
If, however, you don't care about the cross-pageload case, then we can avoid all the EVENT_TIMERS_KEY_CALLBACK business and skip the persistent store altogether, just attach the callback to the lib instance directly.
There was a problem hiding this comment.
That makes sense, thanks for the review. I don't think it's worth supporting the cross-pageload case, the hacky implementation nonsense relative to benefit offered (I certainly wouldn't be using it) just doesn't justify it right now. I think I see how instances work, so I'll take a stab at this now, doesn't look like it'll take long.
|
@tdumitrescu, I've updated this as suggested in your comment. I also added a test to demonstrate that it correctly removes the callback after it is fired. I'm not totally sure about whether I handled storing it on the instance correctly. It doesn't feel right to introduce a new key right on the instance but I couldn't find a really clear example of something similar happening, so let me know if I should handle it differently. I'll squash these down to one commit if we can get this approved, I'm just leaving them as independent commits for now so we can see the feature's genesis. |
|
Just checking in here. Any thoughts? |
As suggested in #204, here's a first stab at adding a callback to
time_eventso one can see or manipulate the tracked time. I'm not sure of the standards and best practices that you guys follow so I used my best judgment. I'm not stoked about adding theEVENT_TIMERS_KEY_CALLBACKconstant but I wasn't sure where to stash it and figured it'd be best to just ask for forgiveness than permission... I added one test, too. Happy to modify this however you'd like.