PostHog / posthog-ios

PostHog iOS SDK
https://posthog.com/docs/libraries/ios
MIT License
29 stars 37 forks source link

Update device type #63

Closed bddq closed 9 months ago

bddq commented 10 months ago

What does this PR do? This updates the device type property sent by SDK to conform examples seen in documentation.

Where should the reviewer start? Just check PHGPostHogIntegration file.

How should this be manually tested? Trigger any event and check that $device_type is set with one of the implemented values.

Any background context you want to provide? Old value "ios" doesn't represent the a device type. Even less for tvOS apps.

bddq commented 9 months ago

Any news @marandaneto?

marandaneto commented 9 months ago

Any news @marandaneto?

Sorry for the delay, there's a similar issue here https://github.com/PostHog/posthog-flutter/issues/49 I believe the change makes sense, but not sure if using the current approach.

I've been doing this with compile flags, eg.

#if TARGET_OS_OSX || TARGET_OS_MACCATALYST
    ..
#elif TARGET_OS_IOS
    ..
#elif TARGET_OS_TV
    ..
#elif TARGET_OS_WATCH
    ..
#endif

They come from https://github.com/apple/swift-corelibs-foundation/blob/main/CoreFoundation/Base.subproj/SwiftRuntime/TargetConditionals.h IIRC, would you mind testing this out?

but never used userInterfaceIdiom.

Constants that indicate the interface type for the device or an object that has a trait environment, such as a view and view controller.

So it might be that this gives a false positive, what do you think?

bddq commented 9 months ago

Conditional flags mentioned can work but cannot identify a CarPlay device (maybe PostHog will support that).

Because userInterfaceIdiom is read from the current device, it's safe to rely on that value to determine the device type. It can also help the user to differentiate an iPhone and an iPad that are two types of iOS devices.

marandaneto commented 9 months ago

Conditional flags mentioned can work but cannot identify a CarPlay device (maybe PostHog will support that).

Because userInterfaceIdiom is read from the current device, it's safe to rely on that value to determine the device type. It can also help the user to differentiate an iPhone and an iPad that are two types of iOS devices.

That's also possible with the $device_model, so I'd be fine with both solutions. @benjackwhite anything to add? Since this is a breaking change and add new values that are yet not documented, eg CarPlay.

benjackwhite commented 9 months ago

I think adding new values is no big deal. Changing existing data though... Less sure. I feel like we could make this a minor change or wait till the Swift rewrite and include it as part of the major change there 🤔

If it can wait I would personally opt for the rewrite as we may want to make more breaking changes at that point as wel

marandaneto commented 9 months ago

@bddq do you mind adding an entry to the changelog? so we can get it merged.

bddq commented 9 months ago

@marandaneto Done!