Skip to content

Support Lifecycle Tracking#88

Merged
bsneed merged 1 commit intosegmentio:masterfrom
FormidableLabs:feature/LIB-1156-tracklifecycle-events
Jul 23, 2019
Merged

Support Lifecycle Tracking#88
bsneed merged 1 commit intosegmentio:masterfrom
FormidableLabs:feature/LIB-1156-tracklifecycle-events

Conversation

@carloskelly13
Copy link
Copy Markdown
Contributor

Previously the track application call was set on the builder after it was passed to the build with integrations call. We want to call it before ensuring the boolean is set when the builder creates the Analytics singleton.

@f2prateek
Copy link
Copy Markdown
Contributor

Looks ok to me. Some questions:

  1. We have our own implementation in RN (added in Move application lifecycle tracking in Android to the native wrapper #73). Should we remove this? Otherwise I think these events will be triggered twice, once from the RN wrapper, and once from the underlying library.
  2. How was this tested? A video of a running app showing the behavior would be great.
  3. Can we add some tests for this?

@carloskelly13
Copy link
Copy Markdown
Contributor Author

We should not remove that because it's required for the Application Opened event. For Android React Native, the bridge doesn't create the underlying native wrapper in time for the Application Opened event to correctly fire, therefore we have to duplicate that. For all other events, the bridge has created the analytics object and the events fire. The tests for the events are in the Android repo. This just sets the boolean on the builder.

@f2prateek
Copy link
Copy Markdown
Contributor

f2prateek commented Jul 17, 2019

So just to confirm - there are 4 application lifecycle events that may be automatically collected:

  • Application Installed
  • Application Opened
  • Application Updated
  • Application Backgrounded

Can you clarify, that after this change, which will handled in the RN wrapper, and which by the underlying Android library?

w.r.t testing - it'd be great to understand some of the scenarios this was tested against end to end.

@carloskelly13
Copy link
Copy Markdown
Contributor Author

Only Application Opened for React Native will be handled by the wrapper. And that is not with this change, nothing changes with this PR as that was already merged in the prior PR.

@f2prateek
Copy link
Copy Markdown
Contributor

I think I might be missing something here, aren't Application Installed, Application Opened and Application Updated handled in the RN wrapper (which is what we did for https://github.com/segmentio/analytics-react-native/pull/73/files)?

@carloskelly13
Copy link
Copy Markdown
Contributor Author

You are correct. We are doing the backgrounded and opened (foregrounded) events in the Android library as to capture them both for RN and Native.

@f2prateek
Copy link
Copy Markdown
Contributor

Got it - so someone using the React Native library would see the Application Installed events twice (once from the RN wrapper, and once from the Android library)?

@carloskelly13
Copy link
Copy Markdown
Contributor Author

No, because we used SharedPreferences to pull the write key to see what the prior version installed was. Additionally I think like Application Opened the bridge hasn't created the object in time for it to even run in the native library. I went and re-tested it just to verify. The application installed event only fired once in a React Native android app.

Screen Shot 2019-07-17 at 5 28 02 PM

@carloskelly13
Copy link
Copy Markdown
Contributor Author

Insofar as testing and verifying events, I created both a native Android and React Native app and linked the local RN and Android analytics libraries. Created a segment account and used the debugger to see events.

@f2prateek
Copy link
Copy Markdown
Contributor

Ah got it - I didn't realize the same preferences were being used by both libraries.

Since there isn't any explicit synchronization, this does feel a bit racy (as we can't guarantee which order the code will run in), but it feels unlikely this will be a problem in practice given how the RN <> native bridging works.

Thanks for describing the test case - that's exactly what I was looking for!

@f2prateek f2prateek requested review from f2prateek and fathyb July 17, 2019 22:42
Copy link
Copy Markdown
Contributor

@f2prateek f2prateek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but would love @fathyb to take a look as well!

@f2prateek
Copy link
Copy Markdown
Contributor

cc @bsneed @danieljackins for reference

@bsneed bsneed merged commit 25a11f5 into segmentio:master Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants