feat(GuildMemberUpdate): emit on user updates #6782
Conversation
src/structures/GuildMember.js
Outdated
| @@ -62,7 +62,7 @@ class GuildMember extends Base { | |||
| * The user that this guild member instance represents | |||
| * @type {?User} | |||
| */ | |||
| this.user = this.client.users._add(data.user, true); | |||
| this.user = this.client.users._add(data.user, true)._clone(); | |||
There was a problem hiding this comment.
Always cloning the user here seems like a bad idea... Now you won't be able to do member.user === client.user for example (yes I KNOW niche case), AND you'll now create clones of the user object N times (where N is the number of guilds that user is in) instead of referencing to the same 1... Why not clone the user when you process the payload instead? @discordjs/the-big-3 thoughts?
There was a problem hiding this comment.
The issue with doing that is that you’d only be able to use the cloned user to compare it to the new user, but the user attached to the member object would be updated by the time guildMemberUpdate gets emitted (since userUpdate comes first and that mutates the user object)
There was a problem hiding this comment.
So change the code in guildMemberUpdate to
const old = member._clone();
old.user = member.user._clone();
member._patch(data);or something
There was a problem hiding this comment.
Sounded like a hacky solution but it’s definitely better, we’ll test
There was a problem hiding this comment.
Yea sorry @vladfrangu that won't work.
If we apply your suggestion, old.user will already be updated by the time it gets to guildMemberUpdate because presenceUpdate always seems to fire first (at least from our tests) which then emits a userUpdate and modifies the user object.
Even if a user doesn't have the GUILD_PRESENCES intent, your suggestion would still only work for the first guild that receives the update and not the ones that come after it (when the user is in more than 1 guild).
Hence why we now ._clone() the user objects to fully detach them from eachother.
Now you won't be able to do
member.user === client.userfor example (yes I KNOW niche case).
yes this is true, but the .equals function can be used instead (member.user.equals(client.user)).
There was a problem hiding this comment.
@vladfrangu I added a setting for it, hopefully you could rereview your standpoint as it's better as a setting since most peeps indeed don't need this.
|
@SpaceEEC Could you please give some details as to why you downvoted? |
|
I agree with y'alls concerns for memory usage going up, hence why I think it would be a good idea to make this a ClientOption. |
|
@imranbarbhuiya thanks for notifying, fixed the issues, also rebased the pr to fix all merge conflicts. |
|
This needs a rebase |
|
We decided to not move forward with this and rather have a different take on this. |
Please describe the changes this PR makes and why it should be merged:
This PR changes the guildMemberUpdate event in a (hopefully)
semver: minorway in order to emit it on user updates like Discord intends us to. TheUSER_UPDATEevent will still be emitted on both theGUILD_MEMBER_UPDATEand thePRESENCE_UPDATEgateway events, however we will not fire presenceUpdate events as presences do not exist on users like it was discussed here. There is only 1 possible issue with this which is that the events will fire when we don't have a user's flags cached and they have flags in reality. This is due to the fact that Discord does not send thepublic_flagsproperty on startup and thus it doesn't get cached. The only alternative would be not to emit the event when the old flags are not present, however this would become an issue in scenarios where the first userUpdate received on a session is a flags update.Status and versioning classification: