FCE-2808: Fix module resolution#477
Conversation
MiloszFilimowski
changed the title
MiloszFilimowski
requested review from
Copilot
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
Fixes React Native (bare) module resolution and runtime polyfill conflicts for the Fishjam mobile SDK, while keeping web behavior working.
Changes:
- Add
fast-text-encodingpolyfill to the mobile WebRTC polyfill setup. - Update
resolveFishjamUrlto avoid relying on URL-constructor throwing behavior across platforms. - Switch WHIP/WHEP client imports in
livestream.tsto dynamic imports to avoidEventTargetpolyfill conflicts.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
yarn.lock |
Locks fast-text-encoding and adjusts dependency resolutions for RN support. |
packages/ts-client/src/livestream.ts |
Uses dynamic imports for WHIP/WHEP clients to avoid RN EventTarget conflicts. |
packages/react-client/src/utils/fishjamUrl.ts |
Changes Fishjam ID → URL resolution logic to be RN-safe. |
packages/mobile-client/src/webrtc-polyfill.ts |
Adds fast-text-encoding side-effect import before WebRTC globals registration. |
packages/mobile-client/package.json |
Adjusts entrypoints and dependency/peerDependency declarations for RN module resolution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { WHEPClient } = await import('@binbat/whip-whep/whep'); | ||
| const whep = new WHEPClient(); |
There was a problem hiding this comment.
The new dynamic import can now fail at runtime (e.g. bundler/module resolution issues) and will reject with the raw import error rather than a LivestreamError. To keep the public error surface consistent, wrap the import in try/catch and map failures to LivestreamError.UNKNOWN_ERROR (or reject accordingly).
| "peerDependencies": { | ||
| "@fishjam-cloud/react-native-webrtc": "0.25.0", | ||
| "expo": "*", | ||
| "react": "*", | ||
| "react-native": "*" | ||
| "react-native": "*", | ||
| "react-native-get-random-values": "1.11.0" | ||
| }, |
There was a problem hiding this comment.
@fishjam-cloud/react-native-webrtc and react-native-get-random-values are listed in both dependencies and peerDependencies. A dependency does not satisfy a peer dependency, so consumers may still see unmet-peer warnings and can end up with duplicate installs. Consider choosing one model (peer-only for host-provided native deps, or dependency-only for auto-install), and if keeping them as peers, remove them from dependencies and/or mark them optional via peerDependenciesMeta.
| const { WHEPClient } = await import('@binbat/whip-whep/whep'); | ||
| const whep = new WHEPClient(); | ||
|
|
||
| return new Promise<ReceiveLivestreamResult>((resolve, reject) => { |
There was a problem hiding this comment.
Inside this new Promise(...) block, the whep.view(...).catch(...) logic only rejects when the caught value is an Error. If the underlying client rejects with a non-Error value, the promise can remain pending indefinitely. Consider ensuring the catch path always rejects (defaulting to LivestreamError.UNKNOWN_ERROR).
| export function resolveFishjamUrl(fishjamId: string): string { | ||
| try { | ||
| if (/^https?:\/\//.test(fishjamId)) { | ||
| return new URL(fishjamId).href; |
There was a problem hiding this comment.
resolveFishjamUrl can still throw if fishjamId starts with http(s):// but is not a valid absolute URL (e.g. malformed input). Previously this was caught and fell back to the default connect URL; consider keeping a try/catch (or using URL.canParse where available) around new URL(...) so the function never throws for invalid inputs.
| return new URL(fishjamId).href; | |
| try { | |
| return new URL(fishjamId).href; | |
| } catch { | |
| // Fall back to default connect URL if the provided string is not a valid absolute URL. | |
| return `https://fishjam.io/api/v1/connect/${fishjamId}`; | |
| } |
2 participants
Description
Added
fast-text-encodingfor bare react-native projects. Expo has TextEncoder included. This library should skip the polyfill if TextEncoder is available globally.Removed redundant changes in the package.json
Changed how
resolveFishjamUrlworks. In bare react-native the polyfill works differently and won’t throw an error here. The new approach should work both on web and mobile.Changed
livestream.tsto import WHEP and WHIP clients dynamically. Internally WHEPClient extends EventTarget globally (it’s a class) which conflicts with the react-native polyfill. By importing them dynamically at a later stage the polyfill should work as expected.Documentation needs to be updated on transcient deps for bare RN projects and pre Expo SDK 54.
Motivation and Context
Documentation impact
Types of changes
not work as expected)