Skip to content
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

Fix broken events in Bridgeless mode (React Native New Architecture) #2370

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

robik
Copy link

@robik robik commented Sep 13, 2024

This PR fixes broken event emitting in React Native new architecture.

EDIT: This is breaking change, and mostly acts as a preview. I will try to come up with a solution for it if time allows.

This is a partial solution, and requires:

@pigeonmal
Copy link

I have test this patch with new arch react native 0.75.3. But i got this kotlin error :
MusicService.kt error: unresolved reference: reactContext

@robik
Copy link
Author

robik commented Sep 16, 2024

I have test this patch with new arch react native 0.75.3. But i got this kotlin error : MusicService.kt error: unresolved reference: reactContext

This probably is caused by the fact that the old RN is used (RN patch was not applied or it was not built from source)

@pigeonmal
Copy link

Thanks for your reply, and how can i fix that ?

My steps :

@pigeonmal
Copy link

Yes the only file i not have the same as you is settings.gradle (i have the default one). If add your modifications, i have an other error :

`Could not determine the dependencies of task ':react-native:packages:react-native:ReactAndroid:hermes-engine:configureBuildForHermes'.

Could not create task ':react-native:packages:react-native:ReactAndroid:hermes-engine:installCMake'.
Could not find sdkmanager executable.`

@lovegaoshi
Copy link
Contributor

so u r having compilation issues. if u dont add that in, ie. https://reactnative.dev/contributing/how-to-build-from-source, ur not compiling RN and not applying any changes. this is outside the scope of this PR and u should seek help in the general RN community.
im not going to pretend compiling RN is easy. its a PITA and im too sane now to troubleshoot someone elses env setup.

@Noitham
Copy link

Noitham commented Oct 29, 2024

Now that 0.76 has been released, should we pick up again on this PR?

Ref. #2290

@lovegaoshi
Copy link
Contributor

with 0.76.1 released these prs can finally be tested. for the lazy folks my example is now upgraded to newarch + 0.76.1.
im still waiting for expo 52 to actually test newarch myself, but with possibly many of my used libs breaking, im not spending a whole lot of effort here.

@syed-shoaib-ali
Copy link

Now that 0.76 has been released, should we pick up again on this PR?

Ref. #2290

in new architecture the app crashes while setupPlayer so will this PR covered that issue as well?

@lovegaoshi
Copy link
Contributor

lovegaoshi commented Oct 31, 2024 via email

@ducatti007
Copy link

Merge that for god sake!

@mihaibulic2
Copy link

I patched this along w/ the required other changes (from 1st comment), and I'm seeing NPEs on reactContext in prod. Does anyone have any guesses as to why this might happen?

Some context:

  • it's pretty rare (1/300 sessions)
  • it seems to happen across all devices manufacturers / all OS versions / foreground & background states
  • it happens after users have already been interacting w/ TrackPlayer for a bit (1-5 minutes, so in my case that means dozens of short tracks played)
  • I'm using the new arch

My thoughts:

  • the old impl was anticipating context potentially being undefined/null (ie reactNativeHost.reactInstanceManager.currentReactContext?.)
  • easiest thing seems to be to just add optional operator (?), but I don't really know anything about what's going on in this file, so maybe that's naive?
Fatal Exception: java.lang.NullPointerException
Attempt to invoke virtual method 'void com.facebook.react.bridge.ReactContext.emitDeviceEvent(java.lang.String, java.lang.Object)' on a null object reference
com.doublesymmetry.trackplayer.service.MusicService.emit (MusicService.kt:744)

@lovegaoshi
Copy link
Contributor

thats an interesting report. i actually use the null safe call myself. I do see from time to time that event emitting becomes unresponsive until the Activity is brought back to the foreground, which explains the reactContext loss and could have the same root as ur report. Though I then deemed the newarch to be unusable and didnt pursue further.

Id start from headlessJsTaskService's getReactContext and find out the status of the reactHost and reactContext (u do need to build ur own RN for this). With the difficulty to reproduce this I wouldnt have high hope though.

@lovegaoshi
Copy link
Contributor

@mihaibulic2 apologies for the ping, did you perhaps see with new arch android the app becomes unresponsive (from being frozen for several seconds, to a whilte screen) when the app is left running at the background for a long period of time (5+ minutes)? I found the longer I left it, the longer it froze/shows a whit screen of death instead. I'm suspecting its my app's issue and about to test the example app, but also curious to see if this is somethign cmmon.

also i dumped more time to testing my android new arch app since react 19 is likely to dump old arch for good, and i'm actually yet to see the js events not being responsive. every event emitted via the notification panel works, despite the view actually crashes out.

@mihaibulic2
Copy link

mihaibulic2 commented Feb 21, 2025 via email

@lovegaoshi
Copy link
Contributor

lovegaoshi commented Feb 24, 2025

thx for the reply! i indeed could not reproduce with the example app. back to the grind.

I narrowed down to anything using useProgress for anyone interested. My current hypothesis and experiments point to useProgress updating states even when the app is in background, causing a lag in state updates when the app is resumed in the new architecture.


a simple if app is background check in useProgress will solve this issue

@SachinRajyaguru
Copy link

Thanks, I did my local changes and it's now working on my one

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.

8 participants