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

chore: improve bridgeless interop layer compatibility #2290

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

Conversation

behenate
Copy link

@behenate behenate commented Apr 5, 2024

Why

In react-native 0.74 when new architecture is enabled bridgeless mode will be enabled by default.
By default modules which use the bridge will use an interop layer but it's not perfect. After creating a project, which uses react-native 0.74.0-rc.6 both Android and iOS don't work at all under bridgeless.

How

I managed to get the features of the example app working on iOS by removing some declarations from RNTrackPlayerBridge.m: getSleepTimerProgress, setSleepTimer, sleepWhenActiveTrackReachesEnd and clearSleepTimer are declared, but never implemented (correct me if I'm wrong), which causes a segmentation fault when using bridgeless. Removing them fixes the issue. All of the functionality seems to be working ok after the fix.

On Android some of the @ReactMethod annotated functions are assigned a value of scope.launch. This causes the interop layer to unnecessarily try to pass the returned Jobs to the JS side, which fails. Added a wrapper for scope.launch, so that the assignment can be kept for better readability, but the function return type is changed to Unit.

Unfortunately some things still don't work well with bridgeless on Android. Some of the events aren't sent correctly (eq. the PlaybackActiveTrackChanged). For some reason all of the notification controls become broken. Those issues are likely easy fixes, but I currently don't have time to learn the codebase and fix them 😕

@behenate behenate marked this pull request as ready for review April 5, 2024 14:56
@behenate behenate requested a review from dcvz as a code owner April 5, 2024 14:57
@behenate
Copy link
Author

behenate commented Apr 5, 2024

@dcvz It would be good to have bridgeless working before 0.74 is released. Do you guys have plans for fixing the Android issues I mentioned?

@brentvatne
Copy link

@dcvz - any chance you could review this? react-native-track-player came up as one of the top 400 most popular packages in the react-native and we're hoping to get as many popular libraries as possible compatible leading up to the launch of [email protected] - reactwg/react-native-new-architecture#167

@lovegaoshi
Copy link
Contributor

thank you for the work @behenate I put together a bare minimum example app
https://github.com/lovegaoshi/RNTPExampleNewArch

for the event issue, it's actually none of the hooks/useEvents work anymore; the addEventListener inside doesnt do anything I believe. however TP.addEventListener as in SetupService works. tbh i'm completely lost with the new arch stuff, if anyone have any insight, thank you very much in advance
hooks are set up as https://github.com/doublesymmetry/react-native-track-player/blob/main/src/hooks/useTrackPlayerEvents.ts

notification is a bigger issue imo bc all of that should be native.

@lovegaoshi
Copy link
Contributor

lovegaoshi commented Apr 24, 2024

I guess somehow bridge and bridgeless are running concurrently? RNTP internal hooks is registered as NOBRIDGE and listeners added via AppRegistry.registerHeadlessTask is registered in the bridge? they use different NativeEventEmitters and the nobridge ones arent updating at all.
the ... count logs are console.log(emitter.listenerCount(event), event, 'count')

(NOBRIDGE) LOG  Bridgeless mode is enabled
(NOBRIDGE) LOG  Running "RNTPExampleNewArch" with {"rootTag":241,"initialProps":{},"fabric":true}
(NOBRIDGE) LOG  deb playback-progress-updated
(NOBRIDGE) LOG  0 playback-progress-updated count
LOG  0 remote-pause count
LOG  0 remote-play count
LOG  0 remote-next count
(NOBRIDGE) LOG  getInitialURL null
LOG  0 remote-previous count
LOG  0 remote-jump-forward count
LOG  0 remote-jump-backward count
LOG  0 remote-seek count
LOG  0 remote-duck count
LOG  0 playback-queue-ended count
LOG  0 playback-active-track-changed count
LOG  0 playback-progress-updated count
LOG  0 playback-play-when-ready-changed count
LOG  0 playback-state count
LOG  0 playback-metadata-received count
LOG  0 metadata-chapter-received count
LOG  0 metadata-timed-received count
LOG  0 metadata-common-received count
LOG  1 playback-metadata-received count
LOG  Event.PlaybackProgressUpdated {"buffered": 1626.357, "duration": 0, "position": 1575.25, "track": 5}
(NOBRIDGE) LOG  deb playback-active-track-changed
(NOBRIDGE) LOG  0 playback-active-track-changed count
(NOBRIDGE) LOG  0 playback-state count
(NOBRIDGE) LOG  0 playback-play-when-ready-changed count
(NOBRIDGE) LOG  1 playback-state count
LOG  Event.PlaybackProgressUpdated {"buffered": 1657.469, "duration": 0, "position": 1577.249, "track": 5}
LOG  Event.PlaybackProgressUpdated {"buffered": 1659.454, "duration": 0, "position": 1579.249, "track": 5}
LOG  Event.PlaybackProgressUpdated {"buffered": 1661.466, "duration": 0, "position": 1581.251, "track": 5}
LOG  Event.PlaybackProgressUpdated {"buffered": 1663.451, "duration": 0, "position": 1583.252, "track": 5}
LOG  Event.PlaybackProgressUpdated {"buffered": 1665.462, "duration": 0, "position": 1585.251, "track": 5}
LOG  Event.PlaybackProgressUpdated {"buffered": 1667.448, "duration": 0, "position": 1587.252, "track": 5}
LOG  Event.PlaybackProgressUpdated {"buffered": 1669.459, "duration": 0, "position": 1589.251, "track": 5}
LOG  Event.PlaybackProgressUpdated {"buffered": 1671.444, "duration": 0, "position": 1591.247, "track": 5}
LOG  Event.PlaybackProgressUpdated {"buffered": 1673.456, "duration": 0, "position": 1593.251, "track": 5}

Copy link
Contributor

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jul 24, 2024
Copy link
Contributor

github-actions bot commented Aug 1, 2024

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this Aug 1, 2024
@Medkhat
Copy link

Medkhat commented Aug 15, 2024

Hi there! @lovegaoshi @behenate

RN version 0.75.1 has already been released. Does anyone have plans to improve this case, or have you found any solution for it?

@dcvz
Copy link
Contributor

dcvz commented Aug 15, 2024

There’s some stuff in the works, but still needs more time to be refined.

@dcvz dcvz reopened this Aug 15, 2024
@Medkhat
Copy link

Medkhat commented Aug 15, 2024

There’s some stuff in the works, but still needs more time to be refined.

Thanks a lot, there are many guys here who are working on their startups and are using your invaluable package. I think this needs to be resolved as quickly as possible. If there are any needs from my side, I'm always happy to help in any way I can. :)

@github-actions github-actions bot removed the Stale label Aug 16, 2024
@migueldaipre
Copy link

Hey guys, any news ? I would like to help if needed

@lovegaoshi
Copy link
Contributor

lovegaoshi commented Aug 29, 2024 via email

@Medkhat
Copy link

Medkhat commented Sep 1, 2024

@lovegaoshi Hi! This issue is not only Android related, it also breaks the connection between the Metro bundler and the native side on iOS)

@lovegaoshi
Copy link
Contributor

lovegaoshi commented Sep 1, 2024 via email

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.

6 participants