-
Notifications
You must be signed in to change notification settings - Fork 45
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
Supports outOfSession via integration option & logEvent #52
Conversation
@@ -95,9 +95,17 @@ - (void)realTrack:(NSString *)event properties:(NSDictionary *)properties integr | |||
{ | |||
NSDictionary *options = integrations[@"Amplitude"]; | |||
NSDictionary *groups = [options isKindOfClass:[NSDictionary class]] ? options[@"groups"] : nil; | |||
if (groups && [groups isKindOfClass:[NSDictionary class]]) { | |||
bool outOfSession = options[@"outOfSession"]; |
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.
See the check for groups above and how it is retrieved. We need the same here.
@f2prateek made those 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.
Code is ok and we can merge it.
However, we might be able simplify the code branches a fair bit. We should do this in a follow up PR.
Internally it's likely that Amplitude's logEvent methods call each other with default arguments.
in analytics-ios track:@"foo"
internally just calls track:@"foo" properties:nil
which itself just calls track:@"foo" properties:nil options:nil
.
So instead of having multiple branches like:
if properties != nil && options != nil {
[analytics track:@"foo" properties:properties options:options];
} else if options == nil && properties != nil {
[analytics track:@"foo" properties:properties];
} else {
[analytics track:@"foo"];
}
You can just do:
[analytics track:@"foo" properties:properties options:options];
and internally the library handles all those cases.
The Android one does something similar to reduce the number of branches.
Out-of-session events are not considered part of the current session, meaning they do not extend the current session. This might be useful if you are logging events triggered by push notifications, for example.
This supports calling
logEvent
withoutOfSession
passed in as true if the integration specific option is passed in:integrations.amplitude.outOfSession
There are four ways to log an event in Amplitude: