lavalibs / lavalink.js

A JavaScript wrapper for Lavalink.
MIT License
57 stars 19 forks source link

fix(node): Cache the full VoiceStateUpdate; fixes `Player.moveTo()` #36

Closed Zoddo closed 2 years ago

Zoddo commented 2 years ago

Currently, only the session_id is cached. When needed, a VoiceStateUpdate is reconstructed by the getter Player.voiceState. However, this "reconstructed" VSU is missing the channel_id property. This break Player.moveTo(), because it calls Node.voiceStateUpdate() with this partial VSU, which is basically a noop without a channel_id.

This commit fixes this issue by caching the full VoiceStateUpdate (which includes the channel_id) in the Node.voiceStates Map.

This is the cleanest way I found.

For the record, Player.moveTo() appears to have been broken by the commit 551792856fa110c499d87bc4a34e49d5835e2f09 which changed the behavior of Node.voiceStateUpdate() when the channel_id property is missing.

appellation commented 2 years ago

Ah, excellent catch and thanks for explaining. I think this is probably the most reasonable solution. Initially we were only storing the session ID in an attempt to reduce memory usage; however, especially since it's lossy, I think that was a bit of premature optimization.

551792856fa110c499d87bc4a34e49d5835e2f09 is also an attempt to avoid memory leaks and I believe the logic there is sound; if the VSU packet has a null channel ID, the connection is being terminated.

Zoddo commented 2 years ago

If we want to reduce a bit the memory usage, we can do something like:

this.voiceStates.set(packet.guild_id, {
  guild_id: packet.guild_id,
  channel_id: packet.channel_id,
  user_id: packet.user_id,
  session_id: packet.session_id,
});

This would notably remove the member object from the VSU, which is likely half of its size

appellation commented 2 years ago

Is there a need for the member object? If not, I suggest we move forward with cutting it out.