-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add react native autocapture lifecycle events #108
Conversation
30b3ee3
to
ccfc5a9
Compare
ecef450
to
21adb52
Compare
return posthog | ||
const resolved = await posthog | ||
await resolved._setupPromise | ||
return resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the async fix
if (appBuild) { | ||
if (!prevAppBuild) { | ||
// new app install | ||
this.capture('Application Installed', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Persistance now happens regardless
} | ||
}) | ||
|
||
this.setPersistedProperty(PostHogPersistedProperty.InstalledAppBuild, appBuild) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning added
@@ -22,6 +22,7 @@ export type PostHogOptions = PosthogCoreOptions & { | |||
| PostHogCustomAppProperties | |||
| ((properties: PostHogCustomAppProperties) => PostHogCustomAppProperties) | |||
customAsyncStorage?: PostHogCustomAsyncStorage | |||
captureNativeAppLifecycleEvents?: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's now gone, I do call a private method in test code now though
21adb52
to
671f5c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks for the back and forth here
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
initAsync
would return beforesetupAsync
had 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
captureNativeAppLifecycleEvents
which, when set, will cause the SDK to automatically capture 'Application Installed', 'Application Updated', 'Application Opened', and 'Application Backgrounded'.