innoveit / react-native-ble-manager

React Native BLE communication module
http://innoveit.github.io/react-native-ble-manager/
Apache License 2.0
2.13k stars 768 forks source link

Add turbo modules support #1285

Open lucaswitch opened 3 weeks ago

lucaswitch commented 3 weeks ago

This PR is still on development process so things can be broken.

Things are being done using the official docs and keeping backwards compat in mind. New arch support can be tested using the examplenewarch app and old arch using the default example app. Reference: https://github.com/reactwg/react-native-new-architecture/blob/main/docs/appendix.md

marcosinigaglia commented 1 week ago

Hi, I migrated the Android side but the swift side is not so simple. The documentation and the examples are not correct (https://github.com/react-native-community/RNNewArchitectureLibraries/tree/feat/swift-event-emitter) so I have to find out other examples.

lucaswitch commented 1 week ago

Hi, I migrated the Android side but the swift side is not so simple. The documentation and the examples are not correct (https://github.com/react-native-community/RNNewArchitectureLibraries/tree/feat/swift-event-emitter) so I have to find out other examples.

I guess we can still use the same RctEventEmitter or am i wrong about that? Because most of libraries that i looked into was still using the old "sendEvent" and still works on 0.76.

lucaswitch commented 1 week ago

Hi, I migrated the Android side but the swift side is not so simple. The documentation and the examples are not correct (https://github.com/react-native-community/RNNewArchitectureLibraries/tree/feat/swift-event-emitter) so I have to find out other examples.

I guess we can still use the same RctEventEmitter or am i wrong about that? Because most of libraries that i looked into was still using the old "sendEvent" and still works on 0.76.

Actually i'm wrong about that, look: https://github.com/react-native-community/RNNewArchitectureLibraries/issues/19#issuecomment-2474160931

lucaswitch commented 1 week ago

Screenshot 2024-11-13 at 16 05 10 I'm still trying to figure it out how to solve that for RCT_EXTERN_MODULE macro. @marcosinigaglia can you help with that?

Also i guess we will probably have to add typesafety in all events to be able to send events to codegen.

marcosinigaglia commented 1 week ago

The new 0.76 API is not using the old emitter (https://github.com/reactwg/react-native-new-architecture/blob/main/docs/turbo-modules.md#add-event-emitting-capabilities), I'm changing the Android side and then I'll check the iOS.

marcosinigaglia commented 1 week ago

The Android side with the new event API is working now. The iOS code is crashing so probably the old way to connect the swift code is not working, I'm trying to solve this issue.

marcosinigaglia commented 6 days ago

I think now is working, the new structure doesn't make me crazy but the only way I found is to keep a reference between objects in object-c and swift. Implementing the new classes in swift was not enough because the methods that handle events were giving error, if they change in the future you can go back to a structure only in swift. @lucaswitch if you can try to do some testing too. I will remove the new example folder shortly because with expo you can test the new architecture simply without having to have a specific example. For now we don't have time to support the old architecture as well so it will be a breaking change.

Marvedog commented 5 days ago

@marcosinigaglia This is some good work. We are currently working on moving our code to the new architecture, so I can fit in some testing of these changes as well on our use case if that helps landing this! I noticed some changes to the API in the docs, do you have an overview of the breaking changes or does these API changes suffice

marcosinigaglia commented 5 days ago

Hi @Marvedog , it would be great if you could do some testing as well. The only breaking changes are in event handling, I have already updated the doc in this PR. I have published version 12.0.0-rc.0 if you need it.

GabrieleMazzola commented 5 days ago

Hey @marcosinigaglia, just like @Marvedog I am also going to test this out over the next days for a new arch app we're building.

Will let you know if anything strange pops up! Thanks for the great work!

lucaswitch commented 5 days ago

I think now is working, the new structure doesn't make me crazy but the only way I found is to keep a reference between objects in object-c and swift. Implementing the new classes in swift was not enough because the methods that handle events were giving error, if they change in the future you can go back to a structure only in swift. @lucaswitch if you can try to do some testing too. I will remove the new example folder shortly because with expo you can test the new architecture simply without having to have a specific example. For now we don't have time to support the old architecture as well so it will be a breaking change.

Ok, i'll check on the devices that i have here.

marcosinigaglia commented 4 days ago

I've tested with a real app ios/android and after few fixes it (12.0.0-rc.2) it seems working. I am waiting for a few more tests to merge.

GabrieleMazzola commented 3 days ago

Hey, I'm just going to write here things as I test, hoping it helps.

iOS 18 iPhone 14 Pro

I tried to use the lib in my app with the new architecture, and the app was crashing when calling the BleManager.startmethod, without showing any useful info in the logs. I then tried to run the iOS app with xcode, and I got the following: image

Since I am using expo 52 (with prebuild), I've now added this to my app.json:

"infoPlist": {
  "NSBluetoothAlwaysUsageDescription": "This app requires access ..."
}

After launching prebuild and re-building the app, now I can scan successfully.

Will post more updates as I test, but perhaps this error can be better notified when running the app not natively with xcode.

weese commented 9 hours ago

I got an error in Xcode with version 12.0.0-rc.2. It is caused from a wrong filename of the Swift->Objective-C bridge header. I opened the PR in the source repo of this PR:

https://github.com/lucaswitch/react-native-ble-manager/pull/1