meetecho / janus-gateway

Janus WebRTC Server
https://janus.conf.meetecho.com
GNU General Public License v3.0
8.25k stars 2.48k forks source link

Javascript tweaks #3211

Closed i8-pi closed 1 year ago

i8-pi commented 1 year ago

A few improvements in the Javascript that should not impact functionality

The first commit

The second commit swaps plain objects with JS Maps where the objects are used as arbitrary key-value stores. Plain objects come with some predefined keys

const obj = {};
typeof obj.toString === 'function' // true

which can be a problem if the keys come from user input, while Maps are specifically designed to be arbitrary key-value stores The code changes are mostly mechanical

obj[key]
// becomes
map.get(key)

obj[key] = value
// becomes
map.set(key, value)

delete obj[key]
// becomes
map.delete(key)

for (const key in obj)
  if (obj.hasOwnProperty(key))
    body
// becomes
for (const key of map.keys())
  body
lminiero commented 1 year ago

THanks! I'll let @atoppi comment on this, since most of the JS stuff I'm clueless about.

i8-pi commented 1 year ago

I will update Janus.sessions to use Maps as well. This will be externally visible though, unlike the other two changes, so maybe we should mentions that in the changelog

atoppi commented 1 year ago

I will update Janus.sessions to use Maps as well. This will be externally visible though, unlike the other two changes, so maybe we should mentions that in the changelog

Good point. That might break things for people using that specific object. Anyway mixing things up (e.g. using objects here, maps there etc.) for similar structs is not a good design imho.

Pinging @lminiero to help taking a decision, since he is the main maintainer/user/hacker of the library.

Side note: in case we decide to proceed with sessions Map, we should also update the types.

i8-pi commented 1 year ago

I've made the requested changes. The Janus.sessions change is there if you want it. The typescript files don't mention Janus.sessions and no changes should be required for that

atoppi commented 1 year ago

Thanks, merging!