fix(android): app crash at boot with old arch

[freeboub]

Summary

Fix app boot due to bad events registered

Motivation

Fix: #4017

Changes

Register also old events

@YangJonghun You introduce the regression with this commit: 3c9b1b5
please correct me if I am wrong but It think the first version I added is only for old arch, and the second for new arch ? maybe we need to add a condition on that to avoid registering too much events.

Test plan

can be tested with the sample provided in the ticket

[freeboub]

@YangJonghun @KrzysztofMoch please handle this small pr in priority, it is blocking a lot of users I think 🙏maybe @yungblud have an idea on this PR also !

[KrzysztofMoch]

We can go with this if

Suggested change

	put(eventType.eventName, MapBuilder.of("registrationName", eventType.eventName))
	put("top${eventType.eventName.removePrefix("on")}", MapBuilder.of("registrationName", eventType.eventName))
	if (BuildConfig.IS_NEW_ARCHITECTURE_ENABLED)
	put("top${eventType.eventName.removePrefix("on")}", mapOf("registrationName" to eventType.eventName))
	else
	put(eventType.eventName, MapBuilder.of("registrationName", eventType.eventName))

If this change fix the issue I am fine to merge this

[freeboub]

I agree with your suggestion, but it is a bit more complicated :)
We need to use the isNewArchEnabled from Application, then we need context to retrieve it. but yes this is what I had in mind !

[KrzysztofMoch]

This is loaded from Application you can see in build.gradle

def isNewArchitectureEnabled() {
    return rootProject.hasProperty("newArchEnabled") && rootProject.getProperty("newArchEnabled") == "true"
}

we are getting this property from rootProject (app) so this should be fine

[YangJonghun]

Sorry for late response.
AFAIK, contional code is not required because if "top" is not a prefix, React Native change it internally.
(onEvent → topOnEvent)
FYI)

• RN0.72 version comments

Also this code should be change for event work properly.

nit; facebook MapBuilder is meaningless it just hashMap wrapper. we can replace it with just use mapOf

put(eventType.eventName, mapOf("registrationName" to eventType.eventName))

[freeboub]

Ok, regarding to the code you are pointing, the override fun getEventName() = "top${event.eventName.removePrefix("on")}" vs override fun getEventName() = "${event.eventName}" should depend on newArch ?

[YangJonghun]

@freeboub
No. That part needs to be handled so that it syncs with value returned by ReactExoplayerViewManager.getExportedCustomDirectEventTypeConstants. This is architecture independent.

[YangJonghun]

I haven't tested it yet, but it seems like a simple solution would be to remove all the parts that add the top prefix.

[yungblud]

I couldn't understand why this issue was relative with Kotlin event changes.
That issue is pointing that "cannot find RCTVideo"
But why this error happened after changing event emitters converting to Kotlin?

[freeboub]

I couldn't understand why this issue was relative with Kotlin event changes. That issue is pointing that "cannot find RCTVideo" But why this error happened after changing event emitters converting to Kotlin?

In fact the real issue is before "cannot find RCTVideo":
There is another log just before 'NativeUIManager.getConstantsForViewManager('RCTVideo') threw an exception.', [Error: Exception in HostFunction: java.lang.UnsupportedOperationException],

[freeboub]

code updated! mapOf was also problematic, I replaced it by : hashMapOf.
And I removed the other "top" prefixing you pointed to me @YangJonghun thank you.
I wonder if this can also be the issue of this ticket: #4005
Thank you

[YangJonghun]

@freeboub
When I reproduced it on my side, problem was mapOf, not code that replaced prefix with top. It looks like just need to fix it with hashMapOf.
(This issue occurs in versions below 0.73.x)

Root cause is React Native's recursiveMerge function
Kotlin's mapOf function return immutable map. but functions that below 0.73.0 tried to put values directly and it is cause of UnsupportedOperationException.
0.73.0's function create new HashMap. (code)

• 0.72.15 version
• 0.73.0 version

I don't think #4005 is related to this issue. I can't reproduce with bare RN project

[freeboub]

@YangJonghun That you for the detailed feedback! Then I put only the necessary fix in place.
I think we are good with that now ! I can go in vacation with free mind now ;)

[KrzysztofMoch]

Thank you guys for this fast fix 🎉 I will make release after weekend with this fix. I will also do improvements to catch issues like this earlier

LGTM ✅

[KrzysztofMoch]

@freeboub @YangJonghun can we merge this?

[freeboub]

Yes, we are good ! Let's merge ! Thank you all for the reviews!

[KrzysztofMoch]

I will do release later today 🙌

[WaheedRumaneh]

@KrzysztofMoch

Hello,
sorry for bothering you, can I know when I can expect to have the new release?
because we are blocked by this issue and waiting for the new release since last week.

[KrzysztofMoch]

I am just making release 🙂

[WaheedRumaneh]

thank you, I appreciate your efforts

[KrzysztofMoch]

@WaheedRumaneh Done

[KKKaisar]

@KrzysztofMoch I have the same problem and that is after release. How do I solve it? Android Expo

[Tommy2103]

I'm also having this problem, even after the latest release. Since I'm new to this library, I tried with the code provided in the docs and it gives me the same error message. I'm also on Expo

[AlvaroZevHau]

Same problem with the new release.

[freeboub]

Can you please provide more information ? I cannot confirm this is the same issue ...