fix: remove void promises from appsflyer plugin #1025
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi there!
Opening a PR here to solve an issue we've been having when trying to integrate with the Segment Appsflyer plugin.
We're running an Expo app using React Native 0.74.5 with Hermes.
When adding the plugin to our Segment instance (and going through the rest of the integration steps as described in the documentation), we'd see the attribution data on Appsflyer, together with all of our Segment tracking events, indicating the integration went well. What was missing, though, were the
Install Attributed
andOrganic Install
events on Segment. Interestingly, this was only the case on production builds, development clients were working just fine.After diving into this issue, I figured out that the
onInstallConversionData
callback is called as expected with the right data, but theanalytics.track
calls for emitting those missing events never end up getting called.I ended up patching the library on our side, adding
.then
handler to thetrack()
calls, which suddenly made the Install events appear on Segment.My hypothesis is that Hermes does some sort of performance optimization on production builds that keeps it from calling the promises all together, thinking they are not in use because of the lack of a promise handler (like
then
orcatch
). I used thethen
handler for an info log, but technically the content of it shouldn't matter here.I'd love to get this merged so that we can remove the patch from our repo! Let me know what you think :)