-
Notifications
You must be signed in to change notification settings - Fork 192
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 default events #455
Conversation
@liuyang1520 can you also share the interface updates here? thank you! |
Good call, updated in desc, thanks for reviewing! |
Sources/Amplitude/Amplitude.m
Outdated
}]; | ||
|
||
// persist the build/version | ||
[_dbHelper insertOrReplaceKeyValue:APP_BUILD value:currentBuild]; |
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.
Do we need to update the Build and version if there is no build and version changes?
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.
Good call, we can skip this when there is no change, will update!
if ([activity.activityType isEqualToString:NSUserActivityTypeBrowsingWeb]) { | ||
NSString *urlString = activity.webpageURL.absoluteString; | ||
NSString *referrerString = nil; | ||
if (@available(iOS 11, tvOS 11.0, macOS 10.13, watchOS 4.0, *)) { |
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.
Question for other versions, we will leave referrerString to nil?
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.
Yep, for other version, I didn't find a way to get the referrerString
, so leave it as nil 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.
Should we remove the event properties in this case in stead of leave it as nil?
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.
Good question, when I tested it, I think when sending the event, the nil event property is filtered out already, see this example event. I am not sure which part of our code did it actually (suspecting this code). Also, I think it is not an issue as the TS SDK applies similar strategy, see code.
# [8.17.0](v8.16.4...v8.17.0) (2023-07-05) ### Features * add default events ([#455](#455)) ([9bf9664](9bf9664))
🎉 This PR is included in version 8.17.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Summary
This PR adds the default events:
For more details, refer to doc.
Updates on the public interface:
defaultTracking
and deep link helpers toAmplitude
: https://github.com/amplitude/Amplitude-iOS/pull/455/files#diff-41c92f08b86f532984d5c0f28cec0371ee8a742b4853fbce90f861382a24c865AMPDefaultTrackingOptions
: https://github.com/amplitude/Amplitude-iOS/pull/455/files#diff-05199b5cb6fb101eae4bdd68ebcbba4c75c710d90b3e86a964fca26573e00f44Example:
For more options setting the
defaultTracking
, check: https://github.com/amplitude/Amplitude-iOS/pull/455/files#diff-b12cbff1563d01315b12fe56142072bf3dd6e0f3964d10a48c815ae50e46e1bc.Checklist