mixpanel / mixpanel-js

Official Mixpanel JavaScript Client Library
https://mixpanel.com/help/reference/javascript
Other
884 stars 308 forks source link

track() destroys my data #107

Open bbarr opened 7 years ago

bbarr commented 7 years ago

https://github.com/mixpanel/mixpanel-js/blob/master/src/mixpanel-core.js#L1070

First, perhaps a more unique, less clash-y name for this internal property? No one can track a property of "token" currently.

Second, and far more important, this method mutates given data, impacting other code in my app. I am happy to put a PR in to fix this issue, but I haven't had the time to see how pervasive this pattern is around the library. Is this just a one-off that slipped through?

Thanks!

tdumitrescu commented 7 years ago

Unfortunately, the token field is used in the properties object as part of Mixpanel's HTTP API spec: https://mixpanel.com/help/reference/http#tracking-events, so even if your value didn't get changed, you still wouldn't be able to track with a custom token property. The main property-setting code in track() is non-mutating (https://github.com/mixpanel/mixpanel-js/blob/master/src/mixpanel-core.js#L1090-L1095), and if you'd find it useful we could certainly remove the mutation from the rest of the method, but the data that gets sent over the wire will always have your Mixpanel token in the token field rather than your custom value. It would definitely be useful in a future version of the HTTP API to separate API options from the properties object, but for the moment that's a hard limitation.

Quuxplusone commented 7 years ago

The "can't track a property named token" part is a limitation of the remote API that can't be fixed client-side, but the "mutates the given object" part seems like a real, fixable bug in the JS client library. Unless there'd be a performance penalty to making a copy of the given object first...?

ksloan commented 7 years ago

The main property-setting code in track() is non-mutating (https://github.com/mixpanel/mixpanel-js/blob/master/src/mixpanel-core.js#L1090-L1095),

@tdumitrescu, that's not where the problem is... properties is clearing being mutated here https://github.com/mixpanel/mixpanel-js/blob/master/src/mixpanel-core.js#L1066-L1067

mixpanel.track should be creating a copy of the properties object before altering it's attributes.

The temporary workaround is to pass a copy of properties into mixpanel.track.

tdumitrescu commented 7 years ago

@ksloan this issue hasn't been closed. You probably misread https://github.com/mixpanel/mixpanel-js/issues/113 which was closed as duplicate.

Yes, it is known that the track() method currently mutates its properties argument ("if you'd find it useful we could certainly remove the mutation from the rest of the method") - it makes a silly assumption that users will be passing either object literals or extensions of existing objects, but clearly not everyone uses the lib this way. We'll get a patch in place, but I can't give you an ETA right now.