SiftScience / sift-react-native

React Native Wrapper for Sift iOS and Android SDKs
MIT License
1 stars 12 forks source link

fix: make Android example app runnable #23

Open mdole opened 1 year ago

mdole commented 1 year ago

Tried to set up the Android example app and ran into a couple problems. First, the ./gradlew file was not an executable, fixed with a chmod +x (second commit in this PR). Second, the build would fail with errors like:

yarn run v1.22.19
$ react-native run-android
info JS server already running.
info Installing the app...

> Task :sift-react-native:compileDebugKotlin FAILED

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

See https://docs.gradle.org/7.5.1/userguide/command_line_interface.html#sec:command_line_warnings
32 actionable tasks: 3 executed, 29 up-to-date
e: Incompatible classes were found in dependencies. Remove them from the classpath or use '-Xskip-metadata-version-check' to suppress errors
e: /Users/mattdole/.gradle/caches/transforms-3/1714374d0a1b6cc0c72f1d9e890ecb22/transformed/jetified-react-native-0.70.6-debug-api.jar!/META-INF/ReactAndroid_debug.kotlin_module: Module was compiled with an incompatible version of Kotlin. The binary version of its metadata is 1.6.0, expected version is 1.1.15.
e: /Users/mattdole/.gradle/caches/transforms-3/2cfe106a332c6a6f2245988cf5920bcb/transformed/jetified-kotlin-stdlib-common-1.6.10.jar!/META-INF/kotlin-stdlib-common.kotlin_module: Module was compiled with an incompatible version of Kotlin. The binary version of its metadata is 1.6.0, expected version is 1.1.15.

...many more lines of that

Specifying the Kotlin version fixes it. Credit to: https://github.com/react-native-webview/react-native-webview/issues/2578

It's possible that this issue is specific to my setup, but I did have a colleague try the setup and he had the exact same issues. So maybe a fix is necessary!

Let me know if I missed things 🙂

viaskal-sift commented 1 year ago

@mdole hi, we actually released a new version of the example recently, you may want to check that one out

mdole commented 1 year ago

Hi @viaskal-sift! I tried deleting and re-cloning the repo and had the same problem as before. That makes sense to me - assuming this is the change you mean (since that is the only PR merged since this one was created that I can see), the change was only to the test setup and would not affect running the app.

viaskal-sift commented 1 year ago

@mdole okay, I see, worth double checking things. Thanks for raising this!

mdole commented 1 year ago

Thanks @viaskal-sift! While you're here: the Android part of the wrapper (not the example app, but the wrapper itself) also does not appear to be correctly sending events to Sift. I've flagged this with our rep, also sharing here in case this hasn't been reported to you yet. I'm happy to open an issue with it if that would be helpful.

I recorded a video demo showing the problem here (3 videos sounds like a lot, but it's 10 min total 😄):

Part 1 Part 2 Part 3

Feel free to reach out if I can provide more info or you have questions!

viaskal-sift commented 1 year ago

@mdole yes, we received this information already and currently working on it so no need to open it here in Github. The problem looks a bit trickier than we originally expected, but our goal is to release the fix later this week (optimistic scenario) or early next week (realistic/pessimistic scenario). And btw, while you're here: Thanks a lot for reporting this in so much convenient way! We really appreciate your effort to record the video with all necessary information. And we'll try to make you happy with the fix as soon as possible :)

mdole commented 1 year ago

Oh that's great to hear! Thanks for the update and for looking into it 🙂 and I'm glad to hear that my videos helped!

viaskal-sift commented 1 year ago

@mdole sorry for being silent for some time. The problem was trickier than we originally thought, but we are about to provide you an example of the integration code that would help to address the problem of not having events for Android case. One thing I would like to double check - what is the navigation lib you're using in your app? Cause in the example we gonna provide we rely on react-navigation

mdole commented 1 year ago

Hey @viaskal-sift, all good! Yes, we use react-navigation

viaskal-sift commented 1 year ago

Hi @mdole ! I am reaching you about the issue you mentioned here - https://github.com/SiftScience/sift-react-native/pull/23#issuecomment-1503039396. We've forwarded the answer some time ago but seems we didn't hear back for some time, so I decided to reach you directly here

Eventually, we can provide the workaround. We have a branch GitHub - SanoopC/sift-react-native at screen_navigation_example with the example of the integration code that should fix the problem. To reiterate - the problem we observed with missing events from Android flow in ReactNative was caused by the limitations of the events propagation between React-Native and Android. The proposed fix is not in the SDK code itself, but in the way we integrate the SDK into an app, thus, so no need to release a new version of the code for now

@mdole The instructions can be found on the branch’s README (Track Screen Navigation section) , so please try this one and let us know if it is working for you. Worth mentioning we've got a feedback from other customer that it was working for them so there is high chance it will address your problem. Looking forward to hearing back from your side!

mdole commented 1 year ago

Hey @viaskal-sift ! Thanks for touching base.

We've forwarded the answer some time ago but seems we didn't hear back for some time, so I decided to reach you directly here

Oh, how did you send it? I don't recall receiving anything! Sorry if I missed it.

The proposed fix is not in the SDK code itself, but in the way we integrate the SDK into an app, thus, so no need to release a new version of the code for now

We tried this out, and it works fine :) that said, it's still relying on manual calls rather than the queueing system the native Android SDK and iOS SDKs have, which automatically upload events after a certain number of events or a certain amount of time. So I hope that Sift will still eventually ship a "real" fix as part of the sift-react-native package, since this is how they provide access to their tools.

Thanks again for your work on this!

viaskal-sift commented 1 year ago

@mdole Thanks a lot for the quick answer!

Oh, how did you send it? I don't recall receiving anything! Sorry if I missed it.

I have an impression we were unfortunate enough to lose some communication steps between our engineering and your team since we communicated the workaround for some already

So I hope that Sift will still eventually ship a "real" fix as part of the sift-react-native package, since this is how they provide access to their tools.

We will try to find a better solution, but unfortunately we have no prospects at the moment of a quick fix since there are some limitations for events propagation between React Native and Android that look like rather a blocker to us at the moment :(

mdole commented 1 year ago

All good, thanks @viaskal-sift :) appreciate the info and the context!

Also, AFAIK the issue that I originally opened this PR to address (the Android example app setup not working out-of-the-box) is still happening - not a high priority or anything, but is this mergeable?

viaskal-sift commented 1 year ago

but is this mergeable?

sure, I will push the team to verify this sooner, sorry for the delay