-
Notifications
You must be signed in to change notification settings - Fork 18
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
Upgrade project structure and update native trackers to version 5 (close #199) #200
Conversation
2952d5d
to
c2f8f0e
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.
On iOS, there's a problem with lifecycle tracking. I'm getting background events but no foreground, and the printed out session info says "backgroundIndex":0,"foregroundIndex":0
.
Nit: for anonymous tracking, the previousSessionId
is null, I was expecting it to be all 0s like the userId
Nit: some of the UUIDs are coming up capitalised on iOS. Is that normal for iOS tracker?
Thanks a lot @mscwilson! Regarding the missing foreground event, I think that is a problem with the underlying iOS tracker. I know it does not happen in the iOS tracker demo app, but my guess is that's just a coincidence. Everything that is relevant to the foreground/background event tracking is handled by the iOS tracker. I could reproduce what happens in the iOS tracker and have created a task for it there including a hypothesis that I have: snowplow/snowplow-ios-tracker#818 The I don't know where the difference in casing in the UUIDs is coming from, but that is just a visual difference because UUID is case insensitive. |
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.
LGTM! This is a big improvement for this tracker.
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 is awesome work @matus-tomlein !
I see this PR has been merged into release/2.0.0
already, so feel free to ignore the comments below until the release PR.
- '*.*.*' | ||
workflow_run: | ||
workflows: [Build] | ||
branches: [main] |
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.
Our default is master
not main
.
Also does that mean that we cannot run a deploy run from other branches (e.g. for rc
releases)?
In addition to upgrading the native trackers to v5, this PR brings bigger changes in the project which are aimed to make it easier to maintain it in the future.
Project structure
I ran into a series of problems when trying to update the project and demo app dependencies which ultimately made me choose to recreate the project from a blank template (using create-react-native-library scaffold).
I decided to do that because the way that we built the demo apps before was quite fragile. First, we built the main project using
npm
and the demo apps usingyarn
. The demo apps included the main project using afile:..
reference which copied the wholenode_modules
from the parent project that we then had to fix in apostinstall
hook.Instead, the new structure uses
yarn
for the whole project. All the build commands are executed from the top level (root folder) without switching into the example app sub-folder. In case one wants to execute a script that is defined in the example project (e.g.,android
to start the app on Android emulator), they can still do it like this:I think that this simplifies the project and will make it easier to maintain and update in the future. There are also various other improvements that make it easier to work with the project such running
yarn
also installs the pods.Translated Objective-C to Swift and Java to Kotlin
The second reason why I thought it's better to recreate the project is that this allowed me to adopt Swift and Kotlin (instead of Objective-C and Java) for the native code. Since the native trackers version 5 also transition to Swift and Kotlin, I thought it'd be better to make the change here too. This should also make it much easier to maintain the project and add new features.
Removed TV demo app
Another change aimed to improve the maintainability was to remove the TV demo app from the project. The TV demo app was exactly the same code as the other demo, it just had a different target to run on tvOS and Android TV. Having the same project duplicated twice was a burden for us to maintain, keep the dependencies up to date in both places and test. I don't think it provided enough value for us to justify it. We may decide to put the TV demo app in a separate repository with other examples (similar like we do for the JavaScript tracker).
Upgraded mobile native trackers to version 5
Finally, this PR upgrades the underlying native trackers to version 5, addressing issue #199.
One side-effect of upgrading to version 5 is that now it is necessary for users to add
FMDB
to theirios/Podfile
file (unless using Expo Go). They will basically need to add this line to the file:The reason for it is that the new iOS tracker is pure Swift and pure Swift projects in CocoaPods require that all dependencies have modular imports. However, the iOS tracker itself can't make define that, so we need to do it in the user app. I will make sure to document this in the install guide.
Reviewing
I appreciate that this is a huge change across many files that is almost impossible to review thoroughly. I tagged multiple reviewers because I thought it might be better to have more people do a light review to make it more likely to spot any problems. Thanks and please let me know if there is any way I can help with making it easier to review!