Skip to content

fix(android): register setIDFA function correctly for native bridge#317

Closed
ondreyz wants to merge 1 commit intosegmentio:masterfrom
ondreyz:fix/android-setidfa
Closed

fix(android): register setIDFA function correctly for native bridge#317
ondreyz wants to merge 1 commit intosegmentio:masterfrom
ondreyz:fix/android-setidfa

Conversation

@ondreyz
Copy link
Copy Markdown

@ondreyz ondreyz commented May 14, 2021

Fix for #210 function call to analytics.setIDFA on Android devices

@alanjcharles
Copy link
Copy Markdown
Contributor

@ondreyz this doesn't seem to pass our ci tests. Before we investigate further, can I ask why you need to set idfa on Android devices? It is specific to IOS. For reference:

https://segment.com/docs/connections/sources/catalog/libraries/mobile/android/#analytics-android-and-unique-identifiers

@ondreyz
Copy link
Copy Markdown
Author

ondreyz commented May 18, 2021

@alanjcharles For my project that is meant to be delivered on Android and iOS, I had to call the set idfa function since it is a requirement for iOS. However, invoking the set idfa function resulted in the code crashing on Android devices.

The cause of the error was due to the analytics.setIDFA method being undefined when invoked on Android devices. Upon further investigation, it seems like this package did intend to gracefully handle set idfa function calls on Android by handling it as a no-op method (see https://github.com/segmentio/analytics-react-native/blob/master/packages/core/android/src/main/java/com/segment/analytics/reactnative/core/RNAnalytics.kt#L34-L36). Unfortunately the no-op method was not registered correctly with the native bridge which leads to a runtime error when called on Android devices.

Hence, the purpose of this PR is to fix the original intention of handling set idfa function calls as no-op on Android devices.

@alanjcharles
Copy link
Copy Markdown
Contributor

@ondreyz thanks for clarifying. Unfortunately, we can't commit this as it's not passing the ci tests and is not something we have seen other customers ask for. I would recommend creating some conditional logic to only call setIDFA() on IOS devices so it's never invoked on android and doesn't crash your build.

@ondreyz
Copy link
Copy Markdown
Author

ondreyz commented May 24, 2021

@alanjcharles The ci tests were probably failing due to PR 316 not being merged to master branch at the time when I forked this repository. I have since rebased this branch on top of the latest master branch.

setIDFA() defined as a no-op on Android.

I would also like to point out that in PR 210, one of the listed changes in the PR description is indeed to handle calls to setIDFA on Android as a no-op. This PR will merely fix that.

Since the resultant effect of calling setIDFA on Android leads to a runtime crash error which is a disastrous outcome, I strongly advocate that the no-op handling on Android should be entirely fixed.

However, if you still feel that this fix is unnecessary, then the docs should at least be updated with the example code showing conditional logic to only call setIDFA on iOS devices so that future users of this package avoid experiencing this crash on Android. For reference:
https://segment.com/docs/connections/sources/catalog/libraries/mobile/react-native/#set-idfa

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.

2 participants