react-native-webrtc icon indicating copy to clipboard operation
react-native-webrtc copied to clipboard

Created Expo config plugin

Open EvanBacon opened this issue 3 years ago • 10 comments

Why

Note I've updated the package to support Expo SDK 46 and React Native 69. This no longer requires react-native-webrtc to depend on @expo/config-plugins directly. Users can continue to use the out-of-tree plugin while we wait for this PR to be merged.

With Expo SDK 41 we've rolled out an interface called config plugins which lets users add native modules that aren't in the Expo Go app to their native cloud builds and locally when prebuilding.

This is a highly requested package so I've created the plugin personally.

How

  • Added a app.plugin.js as the main entry point to the plugin. Plugins must run in at node LTS environments (currently that means 14 and greater).

Test Plan

  • Run npm pack in the package
  • Install the package in a managed Expo project yarn add react-native-webrtc@../react-native-webrtc/react-native-webrtc-1.89.1.tgz
  • Then add react-native-webrtc to the plugins array and build the native app locally with expo prebuild and yarn ios, yarn android. We plan to further automate these steps with expo install and expo run commands.
  • App built correctly without any errors.
    • Running expo config --type introspect showed all of the desired config changes in AndroidManifest, Info.plist, and entitlements files.

Modified some values and in the config plugin and updated with expo prebuild and yarn ios, yarn android.

EvanBacon avatar May 26 '21 23:05 EvanBacon

Looking good! I’m on leave ATM but will review this once I’m back!

saghul avatar Jul 06 '21 18:07 saghul

People can use this out-of-tree solution in the meantime @config-plugins/react-native-webrtc. It's not versioned with the code so YMMV over time, but for now it works great! We also have one for callkeep.

EvanBacon avatar Jul 13 '21 19:07 EvanBacon

@saghul the dependency you're concerned about has many dependencies in common with a standard React Native project so it's just accessing the code that's already in the node modules folder.

We can stick with the @config-plugins/react-native-webrtc package for a while, but it would be good to have it versioned somehow, and we should also land the markdown changes so users have clear documentation on how to get things working.

Further, I've noticed that the config plugin doesn't fully work with package due to a couple of issues. The main one being that Bitcode is not available on iOS, causing the binary to not be fully compilable in Release mode:

❌  ld: '/Users/expo/Library/Developer/Xcode/DerivedData/CustomGo-ghgjpyndamtfsoeycnvxcexmmzsi/Build/Intermediates.noindex/ArchiveIntermediates/CustomGo/BuildProductsPath/Release-iphoneos/XCFrameworkIntermediates/WebRTC/WebRTC.framework/WebRTC' does not contain bitcode. You must rebuild it with bitcode enabled (Xcode setting ENABLE_BITCODE), obtain an updated library from the vendor, or disable bitcode for this target. file '/Users/expo/Library/Developer/Xcode/DerivedData/CustomGo-ghgjpyndamtfsoeycnvxcexmmzsi/Build/Intermediates.noindex/ArchiveIntermediates/CustomGo/BuildProductsPath/Release-iphoneos/XCFrameworkIntermediates/WebRTC/WebRTC.framework/WebRTC' for architecture arm64

I guess running this script fixes that, but we've spec'd config plugins to avoid running fetch requests. Generally all of the native code is shipped in the node module. I'd love to get this fixed, so let me know if you have any ideas for when this should run.

Another issue is that this module often causes the yarn.lock/Podfile.lock to get corrupted, requiring the lock files to be nuked.

EvanBacon avatar Aug 09 '21 19:08 EvanBacon

@saghul the dependency you're concerned about has many dependencies in common with a standard React Native project so it's just accessing the code that's already in the node modules folder.

Good point, I'll check it out again.

The main one being that Bitcode is not available on iOS, causing the binary to not be fully compilable in Release mode:

Does Expo default to using bitcode? Why? It's not mandatory and the advantages are not a clean cut IMHO (Jitsi Meet is built with bitcode, but have no other choice because we have an Apple Watch target). The WebRTC framework with bitcode is ~1GB uncompressed...

I guess running this script fixes that, but we've spec'd config plugins to avoid running fetch requests. Generally all of the native code is shipped in the node module. I'd love to get this fixed, so let me know if you have any ideas for when this should run.

As I mentioned, the framework is just huge, I don't see a way around it TBH. Well yes, defaulting to no bitcode :-)

Another issue is that this module often causes the yarn.lock/Podfile.lock to get corrupted, requiring the lock files to be nuked.

I have never run into this myself, can you show an example? Podfile thrashing is usually due to a mismatched CocoaPods version. Can't speak about yarn since I don't use it.

saghul avatar Aug 10 '21 11:08 saghul

@saghul is there any way that the bitcode could be shipped as a separate NPM package? If it's not possible to ship an optional/addon package perhaps a react-native-webrtc-heavy could be created as an alternative, similar to what `react-native-v8 does: https://github.com/Kudo/react-native-v8/#v8-variants

jesse-savary avatar Nov 23 '21 00:11 jesse-savary

@EvanBacon Looking good 👍🏻 Thanks for your contribution.

We will be looking to move most of the documentation over to here eventually. Mind if i include your documentation changes once the pr has been merged?

8BallBomBom avatar Aug 17 '22 14:08 8BallBomBom

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Oct 17 '22 10:10 stale[bot]

Needs updating to reflect the bitcode removal but maybe include in the next stable update?

8BallBomBom avatar Oct 24 '22 11:10 8BallBomBom

maybe include in the next stable update?

👍 I won't have time to work on this though.

saghul avatar Oct 24 '22 13:10 saghul

Never really used Expo but i can definitely test the pr and ensure things are up to scratch for inclusion 👍🏻

8BallBomBom avatar Oct 24 '22 13:10 8BallBomBom

Will need any up to date changes bringing in from here. To be included in a future release.

8BallBomBom avatar Mar 21 '23 11:03 8BallBomBom