fix(types): allow null value for AppEventsLogger.setUserID

[danilobuerger]

AppEventsLogger.clearUserID is deprecated and suggests using "setUserID(null)"
However, setUserID is currently not typed to allow null values.

Test Plan:

no test plan

[mikehardy]

Suggested change

	* The userID is persisted until it is cleared by clearUserID method.
	* The userID is persisted unless it is cleared by calling this method again with a null userID

[mikehardy]

Oh of course! Should have noticed that when I deprecated the clearUserID method during the forward-port - thanks for catching this. I updated the method doc to match, looks +1

[danilobuerger]

@mikehardy Thanks for merging it so quickly. Although I just noticed that calling setUserID(null) now breaks Android. On the android side it doesn't like null but wants clearUserID. Its a shame that iOS and Android are out of sync like that.

[mikehardy]

@danilobuerger are you 100% on that? Because I thought they both made that change

In the current version they internally delegate clearUserId to setUserId(null):
https://github.com/facebook/facebook-android-sdk/blob/ec96d8a0d3fffa3176ef985819650a68e1ba84bb/facebook-core/src/main/java/com/facebook/appevents/AppEventsLogger.kt#L504-L506

and the internal implementation of setUserId explicitly has the id string arg set as nullable
https://github.com/facebook/facebook-android-sdk/blob/ec96d8a0d3fffa3176ef985819650a68e1ba84bb/facebook-core/src/main/java/com/facebook/appevents/AnalyticsUserIDStore.kt#L46

So at least with the current release I expect it should work. However, "the current release" is v13 and we're still pulling in facebook-android-sdk v12 until #208

on their v12.2 branch it appears to work as well though? https://github.com/facebook/facebook-android-sdk/blob/a6e982175b258fbd99ec8313bbe052f1efd90185/facebook-core/src/main/java/com/facebook/appevents/AppEventsLogger.kt#L498-L500

Except one thing I do see is that setUserId() method is marked with not-null in the Kotlin! So perhaps that's the problem? What exact problem did you see?

We could maybe log an issue for them to true up their types and deprecate clearUserId similar to facebook-ios-sdk

In the meantime if this is breaking things or requiring platform-specific code that's a bug in my way of thinking. For Android we could make setUserId(null) at the javascript layer call native clearUserId() - a quick PR and I'd merge it if you liked the idea and posted it up

[danilobuerger]

Except one thing I do see is that setUserId() method is marked with not-null in the Kotlin! So perhaps that's the problem? What exact problem did you see?

Yeah, that seems to be the problem. I get an error exactly there that a null value is passed to a non null parameter in kotlin.

In the meantime if this is breaking things or requiring platform-specific code that's a bug in my way of thinking. For Android we could make setUserId(null) at the javascript layer call native clearUserId() - a quick PR and I'd merge it if you liked the idea and posted it up

Yup, will submit a PR later today

[mikehardy]

Awesome! Thanks in advance. I'll chase down facebook-android-sdk in parallel with an issue over there

[github-actions]

🎉 This PR is included in version 7.3.3 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[mikehardy]

Upstream already merged the fix sweet! It'll be in whatever is after 13.0.0 whenever that is, and whenever we can access it via forward-porting here :-)