Skip to content
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

fix: audio/video stream in Safari by coping attachmediastream to source in ESM format #10912

Merged
merged 2 commits into from Nov 16, 2023

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Nov 15, 2023

☑️ Resolves

The original attachMediaStream provides CJS and AMD versions to use in RequireJS and Browserify.

The CJS version has an issue with imported inside webrtc-adapter with export default behavior in ESM-CJS interop. webrtc-adapter defines it as __esModule: https://npmfs.com/package/webrtc-adapter/8.2.3/dist/adapter_core.js. This is a commonly used by bundlers approach, but it is not a standard (there is no standard, it is a known undefined behavior).

But old CJS version attachMediaStream doesn't respect __esModule and imports webrtc-adapter wrong. We fixed it with a babel plugin in the past.

The AMD version seemed to work. But for an unknown reason breaks the audio stream in Safari.

See also: f2b67fe

The solution: making an ESM version of a attachMediaStream that uses webrtc-adapter with a native ES import.

🏁 Checklist

Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise looks good, all required files are updated.
If it works for Safari, I'm up for it

@ShGKme ShGKme force-pushed the fix/fork-attachmediastream branch 3 times, most recently from 3e76399 to 91589c3 Compare November 16, 2023 11:26
@ShGKme ShGKme marked this pull request as ready for review November 16, 2023 11:32
@ShGKme ShGKme requested review from nickvergessen and removed request for nickvergessen November 16, 2023 11:32
@ShGKme
Copy link
Contributor Author

ShGKme commented Nov 16, 2023

  • Rebased onto main
  • Squashed fixups
  • Added JSDoc to fix ESLint warnings

@ShGKme ShGKme changed the title Fix/fork attachmediastream fix: audio/video stream in Safari by coping attachmediastream to source in ESM format Nov 16, 2023
ShGKme and others added 2 commits November 16, 2023 12:47
Original library provides CJS and AMD versions for Browserify.

The CJS version has issue with imported inside webrtc-adapter with undefined `export default` behavior in ESM-CJS interop.

AMD version seemed to work but for unknown reason breaks video audio stream in Safari.

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Co-authored-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@nickvergessen nickvergessen merged commit fa18c0c into main Nov 16, 2023
35 checks passed
@nickvergessen nickvergessen deleted the fix/fork-attachmediastream branch November 16, 2023 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review browser: Safari bug feature: call 📹 Voice and video calls feature: WebRTC 🚡 WebRTC connection between browsers and/or mobile clients high regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can not hear nor see people in Safari
4 participants