search

versatica / mediasoup

Cutting Edge WebRTC Video Conferencing
https://mediasoup.org
ISC License
6.27k stars 1.13k forks source link

announced_ip can be a hostname as per spec #1322

Closed ibc closed 9 months ago

ibc commented 9 months ago

Fixes #1321

As explained in the ticket, only Rust changes are needed since in Node (and Worker) announcedIp was already defined as optional string.

ibc commented 9 months ago

Since this is a breaking change for Rust API anyway, maybe rename announced_ip to announced_hostname since it is not guaranteed to always be an IP? Makes sense otherwise.

It should be announced_address instead (to match the naming used in WebRTC).

However I'd prefer if we don't rename it in Rust now. It would be a bit unexpected to have announced_ip in Node and announced_address in Rust. Do you mind if we do this in the future in a new version with breaking changes?

nazar-pc commented 9 months ago

However I'd prefer if we don't rename it in Rust now. It would be a bit unexpected to have announced_ip in Node and announced_address in Rust. Do you mind if we do this in the future in a new version with breaking changes?

We can also rename it in Node.js, but also keep the old field for backwards compatibility reasons. Ugly, I know, but we can get people upgrade to it already and maybe even print warnings in logs of those who didn't upgrade to the new field name.

Not critical or a blocker of course.

ibc commented 9 months ago

We can also rename it in Node.js, but also keep the old field for backwards compatibility reasons. Ugly, I know, but we can get people upgrade to it already and maybe even print warnings in logs of those who didn't upgrade to the new field name.

This deserves a separate story since it's huger than expected:

https://github.com/versatica/mediasoup/issues/1323