-
Notifications
You must be signed in to change notification settings - Fork 24.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Android] Emiting Events from HeadlessTask on Android in New Arch + Bridgeless + interop layer #46050
Comments
cc @robik is this something you could help us investigate with? |
@cortinico sure thing! I will look into it |
@cortinico Hi there! This issue is not only Android related, it also breaks the connection between the Metro bundler and the native side on iOS) |
Hey, @lovegaoshi I've tested your repro on React Native I did not use the exact changes from the above PR, but something similar (made all function return |
thank u for the attention! @robik I just tried w/ rn 0.75.3 and the same issue persists as described in log with RN 0.75.3: (where NOBRIDGE event logs are not being registered)
my repo linked above uses the patched branch already. with RN PR patched the same error persists in 0.75.3.
|
Hey, I've checked it again and yeah, I missed the events. 😅 I've set up two PRs that should fix the issue (on both React Native and RNTP):
Let me know if anything else is broken I forgot to check :) cc @lovegaoshi |
Summary: AppRegistry was not treated as a Callable Module in bridgeless mode. This is breaking headless tasks on Android. Fixes: - #46050 ## Changelog: [ANDROID] [FIXED] - Made AppRegistry callable from Native code in Bridgeless (fixes headless tasks) Pull Request resolved: #46480 Test Plan: Used repro from linked issue Reviewed By: javache Differential Revision: D62637486 Pulled By: cortinico fbshipit-source-id: 756527003ac6d712e76c02c188e280d15c010068
omg tysm! at a short glance everything seems to be in order. I do see the old arch now breaks - bc the old arch's appregistry related code is being removed. Could we add that back in? I can pitch the PR but im wildly not familiar w/ how FB does it. in addition I'm assuming the new RNTP PR now breaks RN backward compatibility due to depending on this addition of getReactHost in headlessJsTaskService? I dont know how RNTP will take it, but I will def use it in my maintained fork. my current patches look like this ofc I need to test this heavily in prod, but unrelated, what are the chances these changes can be included in RN 0.76? cc @cortinico again ty for the work!! @robik |
As this is a bugfix, we can include it inside 0.76. |
It should not break it as the
Well, the PR is mostly for testing and preview, forgot to mention the part about backward compatibility. Maybe some gradle sourceSet trick can work.
If anything breaks feel free to ping me here 👍 |
I can open the pick request. I will do it after I test what is wrong with the old arch (if anything). |
ty! @robik
|
@robik @lovegaoshi can we consider this close after #46480 ? |
#46480 now breaks headlessJsTaskService in the old arch; I can make mine work by not removing the old arch related line from #46480 but @robik wrote it shouldnt break |
@robik can we follow up and making it non-breaking for Old Arch? |
@cortinico I'll be testing it today as it should work on both archs |
I believe I tested it thoroughly and it works on both platforms. @lovegaoshi are you sure you rebuilt it from source and that the patches were not overriden? I retested it on your repo directly with patches applied. Let me know if it worked for you or not so that I can proceed with pick request :) |
@robik My repo's patch didnt delete this line from #46480 (where the else handled the old arch AppRegistry), so it works on both architecture; if you delete that line as #46480 is, the old arch breaks with this log:
appreciate the attention as always <3 |
@lovegaoshi This is interesting. The else branch should not affect this, as the new code calls Can you try on fresh app applying the patches manually? |
flase alarm; I must have not moved the registerAppRegistry part out from the if, my apologies:S tysm for your work! |
Closing then 👍 |
@robik Thank you very much for your priceless support! |
Summary: AppRegistry was not treated as a Callable Module in bridgeless mode. This is breaking headless tasks on Android. Fixes: - #46050 ## Changelog: [ANDROID] [FIXED] - Made AppRegistry callable from Native code in Bridgeless (fixes headless tasks) Pull Request resolved: #46480 Test Plan: Used repro from linked issue Reviewed By: javache Differential Revision: D62637486 Pulled By: cortinico fbshipit-source-id: 756527003ac6d712e76c02c188e280d15c010068
Description
Hi! this is a continuation thread of #44255 to resolve the new arch (being pushed as in RN 0.75.1) compatibility to RNTP. I finally have react native compile from source set up and tried out the patch outlined in #45100 using RN 0.75.1 with the PR patched in. While old arch still functions AFAICT, the new arch now shows this error:
many thanks in advance for looking into this!
Steps to reproduce
yarn run
React Native Version
0.75.1
Affected Platforms
Runtime - Android
Areas
TurboModule - The New Native Module System, Bridgeless - The New Initialization Flow
Output of
npx react-native info
Stacktrace or Logs
Reproducer
https://github.com/lovegaoshi/RNTPExampleNewArch/tree/fix
Screenshots and Videos
No response
The text was updated successfully, but these errors were encountered: