-
Notifications
You must be signed in to change notification settings - Fork 218
Fix #133 Add React Native Support #193
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
base: main
Are you sure you want to change the base?
Conversation
- Add RN shim with react-native-webrtc peer dependency - Shims now export webRTC classes, and implementation imports these instead of globals - Only call document in browser environment
🦋 Changeset detectedLatest commit: ffdfa9c The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
registerGlobals, | ||
} from 'react-native-webrtc'; | ||
|
||
registerGlobals(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we are using the globals which is good so I would say let's drop the registerGlobals()
that way it's still up to the implementor if they want to register them for additional use cases rather than us magically importing them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkundel-openai When I remove the registerGlobals()
line and test this in expo go on my android, neither the mic nor the speaker work. It does work if I then call registerGlobals()
within my expo code - I believe this is because there are other places in the agents-realtime
package where the globals are called.
It seems to me like our options are:
- Remove
registerGlobals()
here, and then the user must callregisterGlobals()
within their RN app to get the mic/speaker working. - Remove
registerGlobals()
here, but make it so that theagents-realtime
package doesn't use the globals anywhere, and have all of the shims export more stuff. - Keep
registerGlobals()
here, and then the user can optionally callregisterGlobals()
within their RN app if they want to change anything.
Maybe I'm missing something though. Could you let me know which you prefer?
@seratch Just wanted to make sure that I wasn't supposed to mark the other comment as resolved to have someone take a look at this. |
Oh my god, I'm so happy someone finally did this! Can't wait for this pr merge! |
Hello, any updates ? |
Fixes #133
What's New
agents-realtime
: Now we export the relevant webRTC classes for each shim,react-native-webrtc
is a peer dependency for the new RN shim.agents-core
: I usedevent-target-shim
,events
,@ungap/structured-clone
andreact-native-uuid
for polyfills. I figured that because they're in the shim, they will get tree-shaken for other environments.How It Works
WebRTC classes are now exposed through the shims with react-native-webrtc as a peer dependency. Users don't need to change anything about how they use the SDK; it just works.
Testing
I've tested this thoroughly in a create-expo-app starter project and everything works great - WebRTC connections, audio handling, etc. I didn't add integration tests here since that seemed like it might be too much setup complexity for the repo, but I'm totally happy to create a standalone example repo if the maintainers would find that helpful!
The React Native shim automatically calls
registerGlobals()
when imported, which sets up the WebRTC globals. I believe that users could still callregisterGlobals()
within their RN app again if they wanted to. In my testing, everything worked without needing to callregisterGlobals()
in the app code.Notes / Improvements
Note that when running in expo, I see:
These warnings are due to the fact that there is some circular importing going on where the shims are imported into mcp.ts, but mcp also imports from the shims. I figured this wasn't in scope for this PR.