feat: Add react native autocapture lifecycle events #108
Conversation
robbie-c
force-pushed
the
add-react-native-autocapture
branch
from
30b3ee3
to
ccfc5a9
Compare
robbie-c
changed the title
robbie-c
force-pushed
the
add-react-native-autocapture
branch
from
ecef450
to
21adb52
Compare
| return posthog | ||
| const resolved = await posthog | ||
| await resolved._setupPromise | ||
| return resolved |
There was a problem hiding this comment.
Here's the async fix
benjackwhite
changed the title
| if (appBuild) { | ||
| if (!prevAppBuild) { | ||
| // new app install | ||
| this.capture('Application Installed', { |
There was a problem hiding this comment.
Doesn't this mean anyone rolling out the new version will get these events across all their apps? I guess thats just "how it is" but we should be careful with our communications here.
There was a problem hiding this comment.
This function is only called when people explicitly set options?.captureNativeAppLifecycleEvents, so anyone doing an update without reading the docs and making code changes just won't get these events.
Maybe one slight optimization would be storing the build number in storage regardless, so that people enabling this flag in future don't get a flood of events.
There was a problem hiding this comment.
Persistance now happens regardless
| } | ||
| }) | ||
|
|
||
| this.setPersistedProperty(PostHogPersistedProperty.InstalledAppBuild, appBuild) |
There was a problem hiding this comment.
Given that this logic is super dependent on persisted values, maybe we need a check to make sure that we have "real" storage?
There was a problem hiding this comment.
I think we know this just based on it being a React Native app, we use expo-file-system or @react-native-async-storage/async-storage and throw if neither one is available. The user can provide their own but if they do that I don't think it's unreasonable to epxect that it's "real".
On second thought it prob does make sense to add a warning if _persistence is set to "memory" and these events are turned on.
| @@ -22,6 +22,7 @@ export type PostHogOptions = PosthogCoreOptions & { | |||
| | PostHogCustomAppProperties | |||
| | ((properties: PostHogCustomAppProperties) => PostHogCustomAppProperties) | |||
| customAsyncStorage?: PostHogCustomAsyncStorage | |||
| captureNativeAppLifecycleEvents?: boolean | |||
There was a problem hiding this comment.
I feel like the naming here clashes with the PostHogAutocaptureOptions. No good option comes to mind but it feels like we might get messier and messier over time... I wonder if we shouldn't just include them as part of the "lifecycle" events, even though they're not strictly dependent on react lifecycles...
There was a problem hiding this comment.
I wanted it to be a separate flag so as to not spam people who update. We could definitely not do that and have it under the same flag, but as you mentioned above we would need to be careful about how we communicate the change, whereas with a new flag it's less of a big deal.
There was a problem hiding this comment.
Re: the naming conflict around the word "lifecycle", I don't have solid ideas. It's called Lifecycle on Android, it doesn't really have a name on iOS, the closest thing in RN is maybe "app state"?
posthog-core/src/index.ts
Outdated
| if (options?.__onConstructed) { | ||
| options.__onConstructed(this) | ||
| } |
There was a problem hiding this comment.
This feels like a hack and I'm not sure what it is providing different from awaiting the SDK via initAsync? At some point someone will see this and use it, even though its meant to be "just for testing".
Is there no alternative?
There was a problem hiding this comment.
It's now gone, I do call a private method in test code now though
robbie-c
force-pushed
the
add-react-native-autocapture
branch
from
21adb52
to
671f5c3
Compare
Problem
Fixes #107
Note: please merge #109 first, as this depends upon that.Changes
Adds these events that were present in v1. I put it behind an options flag, so that users will need to opt-in. This will prevent people from getting an unexpected surge of these events when updating the sdk.
Note: I found that due to ignoring the returned promise on line
posthog-js-lite/posthog-react-native/src/posthog-rn.ts
Line 108 in 54178fc
initAsyncwould return beforesetupAsynchad completed. This is something that is a bit awkward to get right, when there in a constructor which needs to do something async. I fixed this by storing the promise and awaiting it ininitAsync.Release info Sub-libraries affected
Bump level
Libraries affected
Changelog notes
captureNativeAppLifecycleEventswhich, when set, will cause the SDK to automatically capture 'Application Installed', 'Application Updated', 'Application Opened', and 'Application Backgrounded'.