search

daily-co / react-native-daily-js

https://docs.daily.co/reference/rn-daily-js
BSD 2-Clause "Simplified" License
36 stars 13 forks source link

BUG: Compatibility with Expo 50 #48

Closed hramos closed 8 months ago

hramos commented 9 months ago

Expected behavior

We're currently using Daily with Expo SDK 49 in a React Native app. When upgrading to Expo SDK 50, I am seeing a red screen / error which appears to be due to an outdated react-native-webrtc dependency.

The goal of this issue is to track any blockers preventing React Native apps that use Daily from upgrading their Expo dependency to version 50.

Describe the bug (unexpected behavior)

When a <DailyMediaView /> is rendered, the React Native app throws up a red screen / error:

...with the following logged to the console:

TypeError: undefined is not a function, js engine: hermes

This seems to point to an issue with WebRTC. After some digging, I found a potential dependency mismatch with the @config-plugins/react-native-webrtc config plugin which is used to configure react-native-webrtc in Expo projects.

According to https://www.npmjs.com/package/@config-plugins/react-native-webrtc, Expo SDK 50 requires @config-plugins/react-native-webrtc@8.0.0, which itself requires react-native-webrtc@118.0.1.

In the case of @daily-co/react-native-daily-js, it has a dependency on @daily-co/react-native-webrtc@111.0.0-daily.2, which is a fork of react-native-webrtc@111.0.0.

In short, it looks like the fork needs to be updated to be based off a newer version of the react-native-webrtc package.

Steps to reproduce

  1. Create an Expo app using v50 of the SDK: npx create-expo-app SampleApp
  2. Install Daily for React Native
  3. Try to use <DailyMediaView />

Screenshots

See above.

System information

* Device: iPhone Simulator * OS, version: iOS SDK 17.2 # Additional context
filipi87 commented 9 months ago

Thank you for reporting this @hramos.

We have a task in our backlog to integrate the latest changes from react-native-webrtc into our fork. This may require some modifications to our configuration plugin as well.

I will let you know as soon as we introduce these updates.

cwackerfuss commented 9 months ago

Thank you @filipi87 - I've built a project on Expo 50 and just ran into this today, patiently waiting on a fix :) . In the meantime is the recommended solution to revert back to Expo version 49?

filipi87 commented 9 months ago

Yes, I think keep using Expo 49 would be the best approach for now. 👍

nsjames commented 9 months ago

Any clear timeline on this?

jamsea commented 9 months ago

Hi, @nsjames; this is still something we're working on. The recommendation is not to upgrade Expo in the meantime.

cwackerfuss commented 8 months ago

Thank you for the update @jamsea - Our team has built our app using Expo 50 and Expo Router v3 features, so downgrading to Expo 49 is not a viable option for us and is likely not an option for others, either. This does currently block us from progressing. In the meantime we are sending mobile users to the hosted Daily room, but we really need native functionality since our service centers around video calls

nsjames commented 8 months ago

Hi, @nsjames; this is still something we're working on. The recommendation is not to upgrade Expo in the meantime.

In my case, I didn't upgrade. Expo projects start at 50 from quickstart now. I downgraded to 49, but it took a lot of massaging as I was also using the v3 router heavily.

filipi87 commented 8 months ago

Hi, I have already an open PR to integrate the latest changes from react-native-webrtc into our fork.

Once that is merged, we should be able to add support for Expo 50 in our next react-native-daily-js release.

filipi87 commented 8 months ago

The PR with the webrtc changes has already been merged, and we already have PRs up for both:

They are just waiting for react-native-daily-js, 0.59.0, which we should publish probably later this week or early next week.

filipi87 commented 8 months ago

We are now supporting Expo 50.

You will need to use the latest version of our @daily-co/config-plugin-rn-daily-js 0.0.4, and the required dependencies. More details here.

You can also check this https://github.com/daily-demos/daily-expo-demo/pull/6, where we changed our demo app to use Expo 50.