refactor(Users): remove global user cache #8020
Conversation
github-actions
bot
requested review from
iCrawl,
kyranet,
SpaceEEC and
vladfrangu
Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com> Co-authored-by: Synbulat Biishev <syjalo.dev@gmail.com> Co-authored-by: A. Román <kyradiscord@gmail.com>
| if (user instanceof GuildMember || user instanceof ThreadMember) return user.user; | ||
| if (user instanceof Message) return user.author; | ||
| return super.resolve(user); | ||
| if (user instanceof User) return User; |
There was a problem hiding this comment.
| if (user instanceof User) return User; | |
| if (user instanceof User) return user; |
| @@ -63,7 +64,7 @@ class GuildMember extends Base { | |||
| * The user that this guild member instance represents | |||
| * @type {?User} | |||
There was a problem hiding this comment.
When will this be nullable? Even if data.user would be undefined it will still have an (albeit invalid) user class
There was a problem hiding this comment.
it won't set the prop if data.user is undefined, there's a lot of places where we use ?Type to indicate undefined instead of explicitly making it null still.
| * The owner of the webhook | ||
| * @type {?(User|APIUser)} | ||
| */ | ||
| this.owner = this.owner ? { ...this.owner, ...data.user } : data.user; |
There was a problem hiding this comment.
Couldn't we technically always create a user class? (we'd probably need to have a fake users manager on the webhook client I suppose..) Is that even worth it?
There was a problem hiding this comment.
Can't be a fake manager, but also can't be a real one without a some rework, i.e. for createDM, deleteDM
| ? guild.client.users._add({ id: data.user_id }) | ||
| : guild.client.users.cache.get(data.user_id) | ||
| ? guild.client.users._obtain({ id: data.user_id }, guild) | ||
| : logs.users.get(data.user_id) |
There was a problem hiding this comment.
What’s logs meant to be? I don’t see it declared anywhere
There was a problem hiding this comment.
Its the first parameter in the constructor
There was a problem hiding this comment.
Ah I see
But then what’s the type? Is users.get a function?
There was a problem hiding this comment.
This comment was marked as spam.
This comment was marked as spam.
|
After internal discussions, we are going to postpone this to the typescript rewrite after all since it will afford us a lot more flexibility in the implementation, |
Please describe the changes this PR makes and why it should be merged:
Most of what this PR achieves and why is outlined perfectly by that issue, differences and other notes here:
UserManagerextendsBaseManagerto not have a falsecacheproperty, but still retainsresolve(Id)Invitewhere we're creating aUserinstance that can get stale quickly, is this okay? Yes, it is highly likely that the exact same thing was happening before but went unnoticed.client.userhas been changed into a getter instead of removing it completely for nowStatus and versioning classification: