segmentio / analytics-react-native

The hassle-free way to add analytics to your React-Native app.
https://segment.com/docs/sources/mobile/react-native/
MIT License
360 stars 185 forks source link

_logTime incorrectly uses string for iOS, should be unix timestamp (FacebookAppEvents) #875

Closed esdrasetrenne closed 1 year ago

esdrasetrenne commented 1 year ago

The _logTime for iOS should be a unix timestamp but we are using a string which will cause all iOS app events to fail.

@segment/analytics-react-native-plugin-facebook-app-events: 0.5.1 correctly passed undefined for _logTime which allows facebook ios sdk to set it by default. See here.

@segment/analytics-react-native-plugin-facebook-app-events: ^0.5.2 uses an empty string which causes all iOS events to error.

Suggested fix: The return value for the sanitizeEvent function here should either return undefined or the current unix time stamp on iOS

i.e.: Unix Time Stamp:

  return {
    ...params,
    fb_num_items: productCount,
    // IOS requires unix timestamp or undefined
    _logTime: isIOS ? Math.floor(Date.now() / 1000) : unknownToString(logTime) ?? '',
    _appVersion: (event.context as Context).app.version,
  };

Undefined:

  return {
    ...params,
    fb_num_items: productCount,
    // IOS requires unix timestamp or undefined
    _logTime: isIOS ? undefined : unknownToString(logTime) ?? '',
    _appVersion: (event.context as Context).app.version,
  };

Note: I prefer the undefined method and letting the ios sdk handle creating the unix timestamp

In the meantime our app will probably patch this package

oscb commented 1 year ago

Good finding @esdrasetrenne !

I wonder if it's actually more about the bridge sending of the _logtime from the FBSDK Next package to native.

The logTime should never be undefined (under normal circumstances) as it uses the event.timestamp here which is stamped right away on event creation.

I think the actual error rather than the default (which is incorrect though) is that we're sending the stamp as a string when it actually expects a number. Just from a cursory look at the FB Android native SDK code it seems to me the same case as iOS.