#archive-library-discussion

1 messages · Page 6 of 1

tacit crypt
junior pumice
#

okay

tacit crypt
#

uh, ok that doesn't load apparently Thonk

junior pumice
#

can anybody explain to me how they made the VoiceState class bc i wanna make one for slashcmds myself

junior pumice
unique axle
#

#archive-offtopic for things not related to the development of discord.js

  • the source code is public, you can look at the class implementation
junior pumice
#

makes sense

tired valve
vagrant holly
pearl basin
#

my question is, how should this be approached, should/will there be a new event just for direct messages that doesn't have channels?

vernal atlas
#

oh interesting

pearl basin
#

or maybe fetch the channel and then emit message create?

#

I guess another option could be to just create a DMChannel with id (the only data discord provides) and go with it JUSTLOL

unique axle
#

no auto fetching of incomplete structures

pearl basin
#

only problem is that pinnedMessages is null

vivid field
#

could also add recipients: [data.author] to the partial channel

real jetty
copper laurel
#

message.channelID and get channel()

frank turret
#

Ref: Latest pin in this channel now deleted

copper laurel
#

overwritePermissions completely replaces all overwrites on the channel
createOverwrite and updateOverwrite are fundamentally very similar, however I think createOverwrite is supposed to completely replace a single overwrite if it exists, which updateOverwrite is supposed to merge with a single one that exists, or create if it doesnt

#

overwritePermissions is the most un-similar to the other two

ruby terrace
#

At a low level createOverwrite and updateOverwrite are the same thing. However, because the overwrites are cached, they do both need to exist (and it makes it slightly clearer for users).
Further: the API endpoint those are calling updates based on the overwrite id, which will only ever be one overwrite at a time. Discord expects us to use the edit endpoint if we need to edit all overwrites (which is what overwritePermissions does)

#

Perhaps an updateOverwrites method would be better

copper laurel
#

Yeah, if you want to build a method which will analyse and diff the existing permissions rather than completely overwrite them, that should be updateOverwrites

exotic flicker
#

Imo the play method should give a few more options to specify the type of the input and throw more errors if it doesn't know what to do with it/invalid input

80% of the questions in #archive-djs-v12-voice-deprecated are caused by d.js (or prism? Idk) swallowing errors and not throwing errors when something is wrong

vagrant holly
#

Is there a reason why a lot of the structs exposed by the library aren't frozen? E.g Permissions.FLAGS?

slate nacelle
#

I doubt it

real jetty
#

Hey guys, I'm not sure if this is a good idea or not, so that's why im like putting it here instead of making a pr for it

Alright so basically, you guys know how the message object has a url property right? to keep it consistent wouldnt it be better to also make it so that the channel property also had a url property as well? imo it doesnt make much sense to have one url formed for you, and be forced to form the other one manually.

(I searched the docs for a channel.url property but couldn't find one, sorry if i missed it)

#

(Same thing for guilds as well)

oak quail
#

eh

#

with channels you'd usually use a mention

#

which mirrors the client

#

the client only provides message urls and you can use mentions for channels

#

I don't actually see why you would want a channel url, it's pretty much only useful for exploiting client quirks like the old video chat stuff lol

real jetty
#

idk abt any client quirks, the only reason this came to mind was cos someone was asking for it in support for an embed, and tbh it was more of a like consistency thing, but ig ur right and it would be kinda useless

#

ty!!

oak quail
#

yw

strange moat
#

would probably be a better idea to just generate an invite link for that channel, so users who aren't already in the server can still gain access to it

ornate topaz
#

even if, that's definitely not something that library would do on it's own

real jetty
#

wait r you talking abt the forming a url? Or making an invite link

ornate topaz
#

Invite

real jetty
#

ohh okey gotchu

ruby terrace
#

I noticed during some debugging today that any method in a manager that deals with a Collection or Array of items will throw an error if even just one of those items is invalid.
This is fine, however the error it gives back suggests that the whole passed Collectoin or Array is invalid, not just a single item, which makes it difficult to debug.
I could see two solutions to this:

  1. Reject the Promise (currently behavior is throw, but I believe this is changing) with a newly defined TypeError that indicates that one or more of the internal items is invalid
  2. Silently throw out invalid objects / snowflakes and continue execution as long as there is at least 1 valid item left (obviously less desireable)

For reference, I found out about this by removing roles using an array of snowflakes. It threw a type error because one of the snowflakes was associated with a newly deleted role, but the error message made me think my database structure was messed up (I have handled this error before it gets to the method now)

Any thoughts?

snow crypt
#

@vernal atlas what if you made all BitFields use BigInts in v13 for consistency?

snow crypt
vernal atlas
#

well the only thing with that is its just useless conversion between bigint and number all the time

snow crypt
#

we can always break it altogether and not accept numbers

vernal atlas
#

thats the best way i found of solving it

snow crypt
#

maybe an additional param to resolve?

vernal atlas
#

it seems silly to convert between bigints and numbers when you don't have to

snow crypt
#

it is

vernal atlas
#

so then why suggest changing it all to bigints?

snow crypt
#

you can do that without converting

vernal atlas
#

what

snow crypt
#

just bigint or it fucks

vernal atlas
#

the api sends numbers for things like userflags

oak quail
#

could just check loose equality

#

since 0 == 0n

snow crypt
#

equality of what

oak quail
#

oh wait

#

nvm

warped crater
#

for a start, why is this being done on the base Bitfield class

oak quail
#

wasn't there gonna be a separate BigIntBitField? were they merged into one or can we just not tell the difference in this situation

warped crater
#

given this.constructor.resolve is being used all across the board

#

the Permisisons class could just... overwrite static resolve

vernal atlas
#

but then it would just be entirely duplicated code except for replacing 0 with 0n

warped crater
#

default resolve can default to 0, extended resolve can behave like:

if (typeof bit === 'undefined') return 0n;
return super.resolve(bit);```
#

and it'd probs not be a huge deal

real jetty
ruby terrace
#

I much prefer the option to provide additional scopes

#

rather than forcing all invites to request application.commands

real jetty
#

I would prefer it to

unique axle
#

[...] forcing all invites to request application.commands
yeah, that's not gonna happen. array of scopes is probably what i'd go for
reminder though that interactions are still only in public beta

ruby terrace
#

although we could definitely add that array of copes now and add application.commands once its finalized as I imagine there are already some use cases where more than the bot scope is necessary

unique axle
#

sure, v13

ruby terrace
#

I'l go ahead andl work on a PR for that

real jetty
#

The slash commands PR has been merged, even if the slash commands itselves are in beta it is not a reason to not integrate it

proven briar
#

Does Discord.js not support import {} from '' syntax? And if so why? If so what benefit is require() allowing?

raven juniper
#

That's more a node thing not supporting it in js yet, ts does support it though, and d.js does support ts

proven briar
#

oh ok so what is the default export i assume just "Discord"

#

ah wait nvm sorry am a bit tired 😅

oak quail
#

but you'll have to use .mjs or put "type": "module" in package.json

#

(and its unfinished so wont work for everything)

raven juniper
#

Well fair yeah, but as you said, it's still not 100% working, so no need to be pushing it until it is (just like interactionsmmLol )

proven briar
#

Is there any plans for a Deno Package 👀

lofty birch
proven briar
#

lol

clever crypt
solemn oyster
#

Supporting Deno is a headache with JavaScript, maybe once we go to TypeScript (discord.js-next), we'll have an easier way to support it. Regardless of that, we use a lot of things Node.js has that Deno doesn't.

tacit crypt
#

deno does have a node compat layer tho

lofty beacon
#

👋
User.presence.clientStatus returns an object, and this has got a major flaw:
For example, if someone logs in on mobile and logs in on mobile on another device, clientStatus would be mobile: 'status'

There is only one mobile object, meaning, the 2nd one wouldn't be displayed

This'd therefore be incorrect, would a PR changing this to something like an array of objects, with device as a string and status as a string be an idea people like?

Ping me if you have a question.

lofty birch
#

discord doesn't expose if there's more than one mobile device logged in

#

so that's impossible

lofty beacon
#

Really?

#

Must've read it wrong, sorry

vivid field
ornate topaz
#

oh there it is, i just wasted 3 minutes trying to find it on User or Guild

tacit crypt
lofty birch
#

It'd be nice if there was an option to broadcastEval on all available shards when not all of them have finished spawning instead of just erroring, should I PR one?

vagrant holly
#

I'd personally be against adding something like that, given you can broadcastEval to a specific shard. I think it'd be better to leave that to each user to write their own util to broadcastEval to all shards individually and handle the error. Having a util in the library that does that might lead to folks missing issues, like a shard not starting etc.

lofty birch
#

The error is still thrown even if it's to a specific shard

vagrant holly
#

Yes, but you can then catch that error and handle it yourself

lofty birch
#

I meant even if the shard has spawned already

#

there's no way to b-eval on any shard until all of them have spawned

vagrant holly
#

🤔

#

That feels like an oversight on my part if that's the case

lofty birch
#

the error is thrown on the third line here - your added code is after that

vagrant holly
#

Oh i forgot sharding in progress was a thing

#

maybe we just drop that err, cause each shard will throw a no process error iirc

lofty birch
#

if you dropped that line - from my understanding it would then just run for each available shard

#

and ignore the ones that are yet to spawn

#

(which would be the option I was suggesting)

vagrant holly
#

Oh, this is when everything first starts

lofty birch
#

Yes

vagrant holly
#

Personally, I'd be in favour of pulling out the SHARDING_IN_PROGRESS error, but then iterating over all expected shards and throwing an error further down if a shard hasn't yet spawned

#

I'm very against the idea of silently just ignoring shards that haven't spawned, but agree on ensuring its possible to exec on a specific shard before everything has spawned

lofty birch
#

I could work with that

vagrant holly
#

Ig the for loop at the bottom would iterate over this.shardList instead, trying to find the shard in this.shards, and throwing something like SHARDING_SHARD_NOT_SPAWNED if its not there?

lofty birch
#

I'd just move the 3rd line under your addition for specific shards

#

it would be a lot simpler

vagrant holly
#

Actually tbh yeah that achieves the same outcome

lofty birch
#

So should I PR that or will you?

eternal vessel
#

Why is Guild.owner nullable but Guild.ownerID isn't?

old hill
#

because the owner member may or may not be cached

eternal vessel
#

I got that part, but if it's not cached, how can ownerID be guaranteed?

old hill
#

because that’s provided in the guild data from discord

eternal vessel
#

Hmmm ok. Thanks

tacit crypt
#

Guild.owner is a getter on guild.member.cache with the guild.ownerID

vagrant holly
lofty birch
#

Cool, will do

warped crater
#

@vernal atlas re https://github.com/discordjs/discord.js/issues/5227#issuecomment-762727900
Yeah I can def see this being an inconsistently reproduced issue depending on bot size and other factors.
If you check MessageManager#delete all it does is make the HTTP call - at which point d.js waits for the gateway MESSAGE_DELETE packet to actually mark the message as deleted

vernal atlas
#

yeah

lofty birch
#

Yeah, it does depend on size, I can replicate with all code samples myself

ruby terrace
#

It's an error due to eventual consistency

torpid bramble
#

Cheers for bringing it up in here!

warped crater
#

I don't particularly know how to even go about "fixing" this.

#

Most things in the library work this way, since Discord flat out doesn't really give this data properly in the http response

torpid bramble
#

Yeah, in my case I'm thinking I'll just have to queue up my own collected events and process them FIFO

warped crater
#

You could theoretically set it to true if Discord replies with a code 2xx (promise resolves)

#

but then that creates inconsistency

#

and lots of head scratching in other areas of the library

ruby terrace
#

Yeah, we'd have to rely on the response code being 204 (in this case)

torpid bramble
#

But even if I queue up what I'm doing, there's still race conditions around it

warped crater
#

:p for now you could just do something like

# 
const deleted = await message.delete().then(() => true).catch(() => false);```
#

the catch part varying on your scenario

#

which would rely purely on the HTTP request

torpid bramble
#

The issue is, the message is deleted in one collect event, then the error is thrown in the next collect event - if I react between the fist event firing and the delete finishing.

warped crater
#

ah, message collectors - that's a headache

torpid bramble
#

Reaction collector

#

But yeah.

warped crater
#

mhm

unique axle
#

i'm not even sure if this can be considered a bug to begin with - neither do i understand in which use case this would be an issue

#

afaik the user client works the same way, messages are considered deleted and evicted from view and cache once the respective event arrives

warped crater
torpid bramble
#

My case is:
A message embed with paginated reactions. One reaction is assigned to call await message.delete() on the message the reactions are being collected on.
If you react with one of the other reactions prior to the delete finishing, the collect event fires again.
I have:
await reaction.users.remove(user.id at the top of my collect event, and this hits an error as it tries to get the message, which is deleted half way through the call

ruby terrace
#

Yeah, I would agree with that, the message is not instantly deleted with the response, we could speculate on the order that discord processes things, but that wouldn't get us anywhere

warped crater
#

ah

torpid bramble
#

even if i check message.deleted first

#

it doesn't catch it

warped crater
#

in that case you could just end the collector along with the mesasge.delete() call, though

torpid bramble
#

That's true.

warped crater
#

so that next reaction never comes through

torpid bramble
#

Good point

#

But then if the delete fails, the collector is lost pre-emptively, state wise it can still end up inconsistent

warped crater
#

and if the end user deletes the message... well... I guess there is the off-chance for the reaction to come in before the MESSAGE_DELETE, but that's so incredibly unlikely I'd just dismiss that edge case

torpid bramble
#

but that is definitely a preferred option

warped crater
#

well, it's your bot's message

#

the delete would only fail due to a sudden permission change or just Discord failing the request due to a 5xx or something similar

#

either case gets a shrug from me ¯_(ツ)_/¯

ruby terrace
#

either case gets thrown into a rejected promise too IIRC

#

On a completely separate note, is integrating HTTP fetches for getters of user / member etc not in the cache planned for v13 or was that something that is happening later?

unique axle
#

i don't yet see the necessity to introduce library side handling for this (message/other deletion)

#

pardon? you mean auto-fetching uncached partial data? has been discussed but been decided against for what it's worth

ruby terrace
#

Ah, i must have missed the resolution to that conversation then

unique axle
#

for larger use cases auto fetching uncached data could result in massive amounts of api fetches behind the scenes, with intents now being in the picture the whole ordeal becomes arguably worse (seeing that you might need to go with different approaches based on the data you receive)

#

abstracting that away into auto-fetches might even result in the developer never realising how spammy their bot even is. fetching data should be a conscious decision

ruby terrace
#

yeah, I definitely see why it wouldn't be desirable, especially considering intents, just remembered seeing something about it and noticed there wasn't a PR

#

Again, going to a slightly different subject (but indirectly related) when passing a snowflake to something like Channel#createOverwrite(), the user has to be cached, even theough the API only needs the snowflake of the role or user.
This is actually due to attempting to type the data.
This is only one example, but I assume there are others that fall under the same category.
Since v13 will include improved type safety for Snowflakes, would it be feasible to pass Snowflakes directly if no cached object is available?

unique axle
#

i was blissfully unaware that that's the case - with intents happening that's rather bad shibaThinking

#

but i do for this one understand where it's coming from, since the type seems to be required, but can't really be determined based on the snowflake passed, might need to require that to be passed explicitly

#

@ruby terrace should prob. create an issue so that's on the radar (if you can find other places where similar things are happening do include those)

ruby terrace
#

I’d be happy to (even to make a PR once the decision has been made) I’m just headed off to bed though, so will do in the morning

left moat
pearl basin
#

because 1000 kills the session on discords end

#

1000 and some other close codes

warped crater
#

^

#

1000 means "all good, im out"

#

discord wont let you reconnect off of it

#

i... found that out the hard way.

long mason
#

it closes with 4000 so its able to resume its session because the session doesnt close immediately

bright solstice
#

I know v13 should be Soon™️, but do we have any documentation of the changelog or an update guide in the works? (I'd like my framework to be ready and don't see an in-dev branch on the doc site.)

vagrant holly
bright solstice
#

Wonderful - thank you!

real compass
#

is there a reason it’s User.send instead of User.dmChannel.send?

#

or is it just for convenience?

frank turret
#

User#send exists only because User implements TextBasedChannel which has the send method. However, in the code for TextBasedChannel#send you can see that if the object you're calling send on is a user or a member, it will just wrap around createDM and send to the returned dmChannel

real compass
#

ah I see

#

so basically dmChannel.send wouldn’t work if createDM hasn’t been called, and .send does that for you

#

so dmChannel would be undefined

frank turret
#

Not necessarily? createDM will return the DMChannel if it's in the cache, otherwise it will fetch it

unique axle
#

null, but pretty much, yes

subtle vector
#

oh i should probably move my discussion here

#

i didnt see this chat earlier

#

why Guild#addMember instead of GuildMemberManager#add ?

copper laurel
#

BaseManager#add already has specific functionality which that would clash with

#

But your point is still valid, it could be on the manager with a non-clashing name

subtle vector
#

yeah i opened up a fork and looked around to try and do a PR for it and I found that

#

so my question now is why is there a BaseManager class that 2-3 managers decide not to implement

copper laurel
#

Those are fake managers lol

subtle vector
#

uh

#

is that concept explained somewhere

copper laurel
#

Probably not no

subtle vector
#

what's a fake manager then

#

and why the need for it

#

just looks very off to me at a first glance to have one manager reimplement part of the base without extending it or something like that

ruby terrace
#

by fake managers are you talking about things like ActionManager?

copper laurel
#

no

subtle vector
copper laurel
#

nvm I got a little mixed up

#

But Im not sure why this is a concern for you?

#

This manager does have custom a custom add method

subtle vector
#

well its a concern to me because i was going to go in and move the code on over from addMember to members.add

#

and then i see that, so I'm like, "okay lets look at the base manager to see what this does"

vivid field
#

those managers are pseudo-managers, meaning that they don't have their own cache but instead get/filter it from another cache

copper laurel
#

Right, nvm, its because these have a fake cache

#

Yeah

subtle vector
#

so i look at it and i see it doing the cache, and I say "oh cool, but wait, you can do member.roles.add, so maybe I'll look at that and see how that's implemented"

#

so i go there

#

and it's completely different

keen cosmos
#

fake managers are needed for the getters correct? like GuildMember#roles

copper laurel
#

Well that is the manager

#

roles.cache is the getter, yes

keen cosmos
#

ah okay

subtle vector
#

yeah ok I see how this works now

copper laurel
#

So they dont extend BaseManager because you cant override a real property with a getter

subtle vector
#

so how are we going to fix the addMember issue then

#

best way would probably be a non-clashing name right?

#

that seems fine but I don't agree with naming it anything other than "add" though, seeing as the discord API call is literally named "Add Guild Member"

ruby terrace
#

you could use GuildMemberManager#join maybe?

subtle vector
#

that doesn't make semantic sense either

#

I was thinking of like

#

User#join

#

or joinGuild

#

joinGuild probably works better

#

if we have methods available such as member.ban() and member.kick(), I dont see why user.joinGuild() is inconsistent

frank turret
#

having it be a method on User requires the user to be currently cached, no?
The current method accepts a UserResolvable which includes ID only as an option

ruby terrace
#

^

subtle vector
#

well to stay in line with consistency as well as fulfilling the requirement of it being a userresolvable, the only option is GuildMemberManager.add

#

for my use case at least, the user is cached beforehand

frank turret
ruby terrace
#

in most cases it won't be though

subtle vector
#

yeah i agree

#

you can add both, seeing as you have member.ban() and guildmembermanager.ban(userresolvable)

#

why is it not a valid option @frank turret

#

like, semantics and design wise

frank turret
#

pretttyyy sure member.ban is just a wrapper over guildmembermanager.ban

subtle vector
#

yeah that's my point

frank turret
#

i mean sure, a convenience method is fine

subtle vector
#

at the end of the day i just see a massive inconsistency here

#

everything was migrated to use a manager system in v12 then out of nowhere we get a .addMember

#

lol

frank turret
#

which brings us back to the option of guildmembermanager#join lol

subtle vector
#

yeah but i dont agree with join

#

the api call isn't called join

ruby terrace
#

Here's two possible thoughts:

  1. add a parameter to the add method in that indicates the user is not already in the guild (I strongly dislike this)
  2. add a new function named something descriptive, perhaps like forceAdd
subtle vector
#

forceadd also semantically doesn't make sense

frank turret
#

it might be inconsistent, but we have no choice, we can't override a basemanager method that would literally break it's functionality

subtle vector
#

the only way I see design wise to be consistent (if we exclude the issue with implementation for now), is if we add guildmembermanager.add and user.joinGuild to wrap around it

#

pls dont mind my lazy typing for those methods

frank turret
#

mmmmm then what do you propose we do about the existing basemanager method then?

#

cause if we override it, there's tons of places in the lib that are gonna be broken

subtle vector
#

@ruby terrace my thought is that having add and forceAdd both exist is a bad idea because that implies that:

  1. add is an action that may fail
  2. forceAdd does the same thing as add except in some way forces it to not fail
#

@frank turret the issue here is with the fundamentals of the manager system at this point

#

i also disagree with .join because the API method is named add

#

and, read this: guild.members.join(user)

ruby terrace
#

oh, addUser might work, since you are adding a user to the guild, not necessarily a member

subtle vector
#

the guild's members aren't joining the user

frank turret
#

🤔

vivid field
#

maybe rename manager.add to manager._add?

subtle vector
#

@ruby terrace no that's also bad, because again, the API method is named "Add Guild Member"

vivid field
#

dunno if its considered private

subtle vector
#

you are not adding a user, you are adding a member, therefore you don't use addUser

#

users aren't supposed to exist within the context of guilds, only members

ruby terrace
#

you cannot add a member though, since that member doesn't exist yet, you are adding a user, which creates that member

frank turret
subtle vector
#

well at that point that's a gripe with the discord api docs

ornate topaz
#

doesn't guild.addMember require oauth

frank turret
#

it does

subtle vector
#

i'd assume d.js to follow the docs , and not opinionate them

ruby terrace
#

it does

subtle vector
#

yes, it does

#

shouldn't matter though

ornate topaz
#

so why are we trying to force it so hard into base d.js

frank turret
#

because oauth is apart of base d.js? The method already exists on guild so you can't argue that it isn't already in base djs

subtle vector
#

you want to have proper api coverage right?

#

like, on the homepage of djs website?

#

almost 100% coverage?

frank turret
#

the only thing this convo is talking about is moving said method to the more consistent placing

subtle vector
#

i mean the coverage is there but the implementation is flawed

ornate topaz
#

what flaw

#

where

subtle vector
#

read above

ornate topaz
#

i did

frank turret
#

"inconsistent naming"

unique axle
#

there is a lot of words above, which don't mean much

subtle vector
#

to my knowledge, Guild.addMember is the only use case of similar methods not being in <manager>.add

#

the ONLY case

#

and it's an inconsistency

frank turret
#

facepalm_2 only because of the clashing base method

subtle vector
#

yup

#

so the problem at the core here is the fact that internal manager method naming clashes with potential API naming

#

that was something that shouldn't have happened to start with

frank turret
#

so then i like Jan's suggestion of moving basemanager#add to basemanager#_add

#

since it is only used internally either way

subtle vector
#

I think that's a step in the right direction, but I feel it might still be confusing and there might be a better solution

#

maybe something where internal manager code is abstracted from api calls

frank turret
#

well so far you haven't really provided any other alternative, you've only said to replace the add method, no solution as to where we are gonna move the existing method

subtle vector
#

the issue doesn't disappear if I don't have an alternative

#

and I was typing something

unique axle
#

how has API coverage even come up as argument in this discussion... adding members through oauth2 is covered.

subtle vector
#

because someone above asked "why are we trying so hard to force X discord API method into discord.js"

ornate topaz
#

that was not what i said

subtle vector
#

could you rephrase then because I may have misinterpreted you then, mb

unique axle
#

so the real argument is Guild#addMember should be Guild#members#add?
okay. yeah, sure

subtle vector
#

yeah

#

that's what I think

unique axle
#

so can that be the actual point in the discussion please?
not... whatever else what discussed up there

subtle vector
#

well that's what i brought up initially but it got trailed off

frank turret
#

well that's what i'm trying to discuss

subtle vector
#

@frank turret if you want my idea for an actual implementation that prevents the name clashing, i suggest an abstraction between core manager code (managing caches, etc.) and 1:1 wrappers around API methods

#

for example, one could use the guild.members to not return the entire basemanager, but rather a scoped proxy or something on the basemanager that maps calls to manager._api.X or something like that for example

#

that way you could have manager.add, manager.cache, etc. all that stuff, and have all 1:1 discord API mappings outside of that layer

#

that way there would be no possibilities of a name clash

oak quail
#

the point of managers is to separate cache and api Thonk

subtle vector
#

well, then we have an issue, because that's exactly what DOESN'T happen when an api method named .add can't be implemented on a manager because it's cache needs it to have an .add method

#

that's exactly what we are discussing

#

in this case, they seem to be implemented incorrectly

oak quail
#

whats blocking you from just doing guild.members.add

ornate topaz
#

where is that api method named add again

unique axle
#

why are you bringing a 1:1 mapping djs <-> discord API up? that's not a design goal

frank turret
subtle vector
#

we got this

#

and basemanager.add yeah

oak quail
#

oh is there an undocumented basemanager#add

subtle vector
#

yeah its undocumented

oak quail
#

it doesnt show in docs even with private on

frank turret
#

yeah i just tried it, it's weird

subtle vector
#

i think these comments need to be fixed

#

i dont know how this jsdoc and that method are related

unique axle
#

not at all

frank turret
#

tbh i'm confused as to why add is a thing, isn't that blurring the line between separation of cache and manager?

unique axle
#

so there isn't anything to fix either, if anything there is docs to add - to add

subtle vector
#

no

#

the issue is that .add does something other than the add api call

oak quail
subtle vector
#

@frank turret yes, that is my entire point

frank turret
subtle vector
#

that's what i've been trying to say

frank turret
#

Then i guess i've finally understood you, mb

#

though your solution is going a lot farther than what i'd think the solution would be, which would be to just remove .add

you're thinking of the next managers revolution type stuff

subtle vector
#

well thats what you get when you find a fundamental violation of separation of concerns

#

my point: if the goal is to have an abstraction layer between an internal cache mechanism and API consumption, the very fact that there is a possibility for a name clash to occur on the same class between the two proves that the implementation is fundamentally flawed

ornate topaz
#

looks like BaseManager.add is a left-over from DataStore

vivid field
#

manager.add was added as "create a new instance of this.holds, and add to this.cache if it doesnt exist yet"

subtle vector
#

@frank turret the solution may also very well be to just remove add, if it is a leftover from something. however, i dont know what impact that would have as I'm not too familiar with the inner workings of the lib, so that would need to be checked by like everyone

ornate topaz
#

the very fact that there is a possibility for a name clash to occur on the same class between the two proves that the implementation is fundamentally flawed
not exactly? this would mean that a manager has to have exactly 0 properties on it, as virtually anything could clash

subtle vector
#

the manager doesn't have to have exactly 0 properties, the object given to the user when requesting the manager should

#

abstract the api away from the caching

frank turret
#

tbh that just feels like a layer of abstraction too far for the end user to have to learn

ornate topaz
#

so how would you manage cache of a manager again

subtle vector
#

I explained above

ornate topaz
#

no

copper laurel
#

weeew I missed a lot

#

This channel does not need this much argument, god damn

subtle vector
#

you have all caching mechanism-related methods on the manager, using the existing add/set/remove/whatever it needs. then, in the API that the end user uses, for example guild.members, you return a wrapper around the manager that exposes only dAPI methods instead of the manager itself, using a proxy or something

frank turret
#

It's just discussion, overly drawn out discussion at that

subtle vector
#

that wrapper then would control the interaction between the two

#

so from the user's side of things, they would ideally only get access to API methods

copper laurel
#

Yeah but this is now so far removed from what the discussion was supposed to be about

subtle vector
#

I dont see how that is, I was answering the question of how I'd implement a solution to the problem personally

#

the discussion's still about the core problem

ornate topaz
#

and how do you access actual members via this propertyless manager proxy?

oak quail
#

there isnt much on BaseManager

#

add is the only thing i see thats problematic

subtle vector
#

yeah

#

that's the whole issue

#

but seeing as it's used internally and causes a name clash with a real api method, here we are

copper laurel
#

We can call our stuff whatever we like though

subtle vector
#

yeah im not denying that, i just figure in the name of consistency that it wouldnt make sense to have Guild.addMember

oak quail
#

just renaming it to _add should resolve the problem

#

or if we dont want to break too much

subtle vector
#

ya that too

oak quail
#

making it _add in guildmembermanager specifically

#

guildmembermanager already has a ```js
add(data, cache = true) {
return super.add(data, cache, { id: data.user.id, extras: [this.guild] });
}

frank turret
#

then comes in the question about consistency

copper laurel
#

How would that work?

unique axle
#

but this argument is about a fundamental flaw in the method existing in the first place - which it really isn't. the manager manages the data saved in the cache.
meaning half of this way too long thread is void.

subtle vector
oak quail
#

so we could just make that _add and overwrite add to be addMember and be done with it

ornate topaz
#

but cache where

subtle vector
#

on the manager

ornate topaz
#

but how do you access that

subtle vector
#

the manager will still manage the cache

ornate topaz
#

for example guild.members, you return a wrapper around the manager that exposes only dAPI methods instead of the manager itself, using a proxy or something

subtle vector
#

yes

oak quail
copper laurel
ornate topaz
#

enlighten me

#

how do you fit a manager.cache into manager that has no .cache

oak quail
subtle vector
copper laurel
#

I'm not a fan of having both add and _add

subtle vector
#

nor am I

#

here, I'll write out my idea + some code basics

oak quail
#

addToCache?

subtle vector
#

since it's going to be a lot easier for me to explain it that way

copper laurel
#

Sorry wrong reply

subtle vector
#

addToCache bad because you have .cache prop

frank turret
# oak quail does anyone have any objections to this

only objection i have is on the basis of consistency with the other managers, why not do the same change for all the managers in the event dAPI ever makes another endpoint that clashes. Why not make all manager specific methods have that _ notation

oak quail
#

i dont have any issues with that

#

(other than its more changes)

subtle vector
#

@frank turret the future proof-ness of "in case they ever make a new one that clashes" was also a core factor in my solution

copper laurel
#

This is supposed to be a channel for raising suggestions / issues to the actual maintainers and developers of the library. Arguing over the naming of a property is exhausting

subtle vector
#

well it's not just about the naming anymore at this point

copper laurel
#

What you're now proposing belongs in the -next lib imo

subtle vector
#

the naming is a problem due to another underlying issue and that's what i'm talking about

frank turret
#

I'll take my leave from the discussion, as my last message is probably my final stance that i've got on it

subtle vector
#

not too familiar with -next personally

#

is that where you do like

#

v20 or something

#

I do agree that it would definitely be a change big enough to the point of it probably belonging in a repo like that

copper laurel
#

Its a typescript rewrite

oak quail
#

its a planned ts rewrite, pretty empty rn

subtle vector
#

ah

#

neat

oak quail
copper laurel
#

So you'd be able to properly flag properties as private

subtle vector
#

so .cache.add()

copper laurel
#

No

subtle vector
#

but that clashes, because .cache is collection

copper laurel
#

We're not going to be changing Collections

#

Rule that right out

subtle vector
#

I didn't suggest that, I'm bringing up the issue of what @oak quail is saying

#

I agree with you lol

unique axle
#

but this argument is about a fundamental flaw in the method existing in the first place - which it really isn't. the manager manages the data saved in the cache.
meaning half of this way too long thread is void
again.

copper laurel
#

But its not what he said waitWhat

#

He said .addToCache

subtle vector
#

ok I want to reply to like 4 different people but it's so difficult to keep track of this

copper laurel
#

Yeah, GuildMemberManager#add is the edge case here, not BaseManager#add

subtle vector
#

@unique axle i was going to type a reply to that when you sent it but I got caught off guard by everything else

#

me calling it a fundamental flaw was based on my assumption that there was a goal of separating caching from apis

unique axle
#

then do me a favour, stop writing, read
and then decide if a response is still needed.
this entire chat at this point reads more like a rant than like a legitimate suggestion, which i am very much not a fan of.

copper garden
#

Is there a reason that method exists in the first place? I mean, it’s the only OAuth method that exists in the library afaik

subtle vector
#

i have had "give me like 5 mins, I'll write something up" pasted in my clipboard for the past three minutes

oak quail
subtle vector
#

but i have to keep overwriting myself because everyone's bringing up like 5 different things

copper laurel
#

It requires an oauth token from the user to use, but its a bot API method

subtle vector
#

yeah tbh I agree we should keep that api method in the lib

#

it is a bot method

unique axle
#

that has been discussed before, it stays, it's part of the bot API, not the oauth2 api - despite requiring an oauth2 flow to receive the required token

subtle vector
#

@unique axle so the core thing that I based my argument on originally was me hearing somewhere that managers existed as a way to abstract api methods from caching

#

if that's the case, then I believe what I say to be valid

#

but if it isn't then i'm wrong

#

and I remember reading something like that in this chat before I made that point, and that was my reasoning

#

I can try and find it rq

subtle vector
#

I know less about the intended design of the stuff than you guys do so if you say that this isn't the point then that's fine, but that was the presumption that I made my points with

unique axle
#

again, cache is a data structure, nothing more
the manager acts as... manager, to manage said cache, add and evict data - however complicated that might be
which also makes Basemanager#add completely valid to be on the manager
it allows the adding of data to the underlying cache without having a ton of code duplication when the api is called from various places

subtle vector
#

yeah, I understand that, I never proposed we get rid of caching from the manager

#

but my question now is that if the manager acts to manage the cache, then why is it also responsible for wrapping API calls

unique axle
#

you argue that #add should not be on the manager - for whatever reason

copper laurel
#

Because thats what API calls do

#

Manage cache

#

fetch? into cache

#

kick? remove from cache too

#

edit? update cache

subtle vector
#

I dont think you guys are fully understanding the solution that I was proposing to the issue

#

There's a rather big nuance here

unique axle
#

a rather big nuance is as well that you are proposing a solution to a problem that doesn't exist

copper laurel
#

We're allowed to understand but not agree

subtle vector
#

I didnt say that method shouldn't be on the manager itself entirely, I said it shouldnt be on whatever the end user gets to work with

unique axle
#

the only thing that actually should be discussed is moving Guild#addMember to GuildMemberManager#add and renaming the Basemanager#add which conflicts

copper laurel
#

You're proposing yet another level of abstraction between cache/manager/wrapper of that and I fundamentally disagree that its necessary

subtle vector
#

@unique axle well if you find a way to do that that would be good, but assume later down the line for example discord somehow adds an API method to whatever you rename BaseManager#add to (assuming you dont call it _add or something, which tbh I disagree with because then you'll have .add and ._add on the same class and that'll get a bit weird)

vivid field
#

considering there's e.g. Base#_clone and GuildChannel#clone too, i dont think _add would be a problem

subtle vector
#

since it's calmed down a bit here, an example of my solution would be to still have the manager handle all the cache-related stuff retaining the problematic BaseManager#add, etc. - the difference is that the implementations of all the API methods would be on <IndividualManager>#api or something, and when somebody requests for example guild.members it returns a proxy who's entire job is to facilitate cooperation between the api and the manager's cache itself

#

But the proxy would only actually return methods defined in the api child, not any of the internal manager methods involving cache manipulation

#

and maybe a .cache object as well that acts the same way as it does now

#

That way it would be effectively impossible for there to be any name clash

#

keep in mind that idea is overkill for this little thing, and would definitely be better suited in a rewrite. if renaming and moving around a method or two is all it takes to fix the .addMember inconsistency then I'm all for it

#

but just something to consider for the future

subtle vector
copper laurel
#

I disagree that's necessary. The manager is already facilitating the cooperation between the API methods and the cache itself. I don't see the perceived issue with the user being presented with methods that operate on the cache also.
The only demonstrable example of this is add which can be addressed by renaming it to _add

subtle vector
#

my idea comes more from my POV that code that may need to be fixed in this exact same way if another example of this happens at some point is not maintainable

copper laurel
#

I dont think a major refactor is the answer to that

subtle vector
#

i understand that it's not the most efficient way to deal with that in respect to time constraints and whatnot, and if you can get away with renaming a method because you got a name clash and it'll save time then by all means do it, but there's no guarantee that this kind of thing won't pop up again even if rarely

#

in my eyes I don't consider patching this one instance out as a true fix to the underlying issue, that's all

#

that's also why I said it's definitely much better suited to a rewrite if you're already doing that

#

I really disagree with the pattern of adding underscores to a conflicting method to fix the conflict, because that becomes a huge pain in the ass really quickly from my experience

#

but it's your library - as you said, you don't have to agree with anything, I'm just bringing my input to the table

copper laurel
#

The underscore is normally used to denote that a property is only intended for internal use, since Javascript has no private typings for class methods (or didnt until a very recent TC proposal)
I'd say its more that a naming clash has flagged that this method should have been underscored, because BaseManager#add is intended for internal use

subtle vector
#

well what classifies internal use vs non internal use in this case

#

like, exact same thing as private keyword in other langs?

copper laurel
#

Roughly

subtle vector
#

i'm also not too sure how you rely on a system like that

#

javascript doesn't have actual classes

copper laurel
#

Exactly

subtle vector
#

it's not real OO

copper laurel
#

Exactly

subtle vector
#

so i dont know why you're trying to feign private accessors with _'s in the first place

copper laurel
#

Because it communicates to users that its not intended for use

#

This is no longer library discussion, really more about general practices. This isnt a uniquely djs thing

subtle vector
#

seeing as you use these practices in the library i'd say it's discussion related to the library though

#

Tbh if that's the only inconsistency then there's no issue besides fixing it

copper laurel
#

Respectfully I dont really care what you say it is or isnt and I, as staff, am trying to move tangents out of this channel and stop this circular discussion from covering the same points all the time

oak quail
#

.referencedMessage READ-ONLY
The Message this crosspost/reply/pin-add references, if cached
seems weird to say "if cached" when its provided by the api?

vivid field
#

it's only provided for replies, not crossposts etc

frank turret
#

is it weird? Cause looking at dAPI docs on the you only get the message id, channel id, and guild id, so wouldn't we not be able to provide that prop if it isn't cached?

oak quail
#

not always though

frank turret
#

facepalm_2 can't believe i overlooked that

copper laurel
#

Does it when you fetch

#

Its also possible to have a reference deleted

#

tl;dr there are valid cases where it wont exist in cache

#

Also a really obvious callout - messages that are not replies

#

I have no idea if message cache sweeping would clear them out at the same time or not

vernal atlas
#

@pearl basin i pushed a fix to send the message event - but for the moment it requires having the channels partial, is the best route just removing the channels partial and then dm channels can just always be partial?

oak quail
#

yeah

#

tbh, would probably be a good idea to just allow anything to be partial and to stop dropping events by default

pearl basin
#

what is the use of the channels partial even? I mean... When is the channel not in cache except for DMs?

unique axle
#

you just answered your own question - reactions on uncached messages in DMs

pearl basin
#

so only DMs interesting

#

idk I'd remove the partial and just put "events that happen in DMs might have an incomplete channel object" in the changelog

old kelp
#

TYPING_START on an uncached DM channel is also probably a partial (?)

ruby terrace
#

I think long term that may be a good idea. Furthermore, slash commands might affect decisions around this, if the bot isn't it the guild, can't fetch the channel, so almost guaranteed partial unless they decide to send the channel object with interactions

unique axle
#

how would removing functionality be a good idea?

#

if you don't need the partial, don't use it - i don't see any reason for it to be removed, it covers an edge case that exists in various use cases

pearl basin
#

Having to enable a partial to be able to receive a DM peepee

gUys heLp mY bOt nO reCeIve dMs

{200 lines of shit code here}

ruby terrace
#

Oh, I don't mean removing partial functionality, I mean having partials be expected (since that's how the API handles it) especially with interactions where there is no bot user

pearl basin
#

it's a small change but it's still gonna cause a lot of confusion for those who... Uh... want to make a bot but don't want to learn programming

unique axle
#

i have no idea what you are trying to get at...
you don't have to enable partials to receive dms, you have to enable partials to receive events which depend on dm channels

pearl basin
#

which are DMs

#

lol

#

(since v8)

wild flax
#

Still no idea what you want or are saying

pearl basin
ruby terrace
#

Sum up: DM channels will always be partial unless manually fetched with v8 gateway, meaning users won't get DM messages unless they have channel partials enabled

vivid field
snow crypt
#

@wild flax having strings for bitfields still usable is defeating the entire purpose of the pr

vernal atlas
#

well yeah thats what the type is for isnt it? (referring to ping above, forgot to reply mmmmm)

like:
GuildAuditLogs.Actions.BAN_ADD should be a number not number | undefined

wild flax
#

I think we can evaluate completely eliminating strings with the typescript rewrite

#

but this is def a step in the right direction

snow crypt
#

so do I add a deprecation warning?

wild flax
#

Nah

#

Just what I said will be totally fine

snow crypt
#

will do

wild flax
#

We can always add it later when we have a good path for the rewrite

#

new permissions and all are just pure strings now because of big ints

#

why should we accept a number?

vernal atlas
#

it was to accept compatibility when extending the class
so that for instance UserFlags still uses numbers but Permissions uses bigints

wild flax
#

ah

#

all good then

clever crypt
#

would you say discordjs/collection#31 warrants a version bump/new release?

rich iglooBOT
wild flax
#

yeah

#

a major one even

#

because of the node 14 bump

wild flax
ruby terrace
#

Ah, thank you, forgot those were a thing

vivid field
#

any thoughts on making Collection#array() and Collection#keyArray() getters?

tacit crypt
#

any thoughts on removing them altogether? No..? just me? ok

vivid field
#

they're used internally for some other methods 02Shrug

tacit crypt
#

and they're more less a cached [...coll.values/keys()]

vagrant holly
vivid field
#

at what size does that caching mechanism actually become noticeable?

tacit crypt
#

I mean you'll probably notice it at any size if you really monitor each byte of ram used, however, I'd expect a bunch of ram used if you, say, call members.array() on a 100k members guild (assuming you have all members cached)

solemn oyster
#

Yeah... there are cases where the caches bring more bad than good, members.array() is a good example

frank turret
#

So what about a potential bool to indicate whether a cache is warranted or not

unique axle
#

what's a use case for caching this anyways?

solemn oyster
#

But it's mostly useless, because:

  • When the collection is small, the values are barely added/removed, but creating arrays on the fly are so fast it's negligible.
  • When the collection is big, the values are added/removed very often, which voids the array almost instantly, forcing re-arrays anyways.
#

Having this cache layer also adds overhead in the set/delete calls, even for those who don't use the array caching

vivid field
#

so is it better to remove just the cache or the whole methods real_eyes

solemn oyster
#

Both

#

I believe .array() was originally done because we didn't have [...iterator] back in 2017

unique axle
# rich igloo

we need a major for this anyways, so might as well do it now

tacit crypt
#

we can keep the method as a shortcut

#

but drop the caching aspect of it

glacial quiver
#

Hi, i have a question about ratelimit/api on this function .remove(arrayofroleids) when using an array of role ID's.
Does this count as 1 request? (see gif)

vagrant holly
#

if (roleOrRoles instanceof Collection || Array.isArray(roleOrRoles)) { ... return this.set(newRoles, reason);, single API request

glacial quiver
#

ahh like that thank you so much! ❤️

vagrant holly
#

If you pass a single, the lib sends a delete request to remove the role from the user. If you pass multiple to remove, the lib sends a single request to set the users roles to what remains

glacial quiver
#

oh wow, thats interesting. thanks for clarifying!

#

but does it still on the api side use one rate limit request?

oak quail
#

yes

#

but the two endpoints have different buckets

#

well theyre the same but separate

vagrant holly
oak quail
#

so if youre just adding/removing one role at a time you can technically use both endpoints to do 2x the amount without getting ratelimited

glacial quiver
#

im surprised how detailed of an answer you both gave me fjdiosaj thanks again! it was definitely helpful

#

have a nice day : D

#

oh just wondering, is it also possible to remove/add an array of roles to an array of members with only one request?

#

nvm probably not

vagrant holly
#

nop

unique axle
#

no

glacial quiver
#

haha thanks :p

unique axle
#

questions regarding library usage go into the GET HELP HERE category as well

glacial quiver
#

got it! sorry

vivid field
#

collection exports are still kinda broken btw BOYEbomb

exports.Collection = Collection;
Collection.default = Collection;
module.exports = Collection;
exports.default = Collection;
#

should probably just remove the module.exports = part

copper garden
#

module.exports = messes up the exports referencing module.exports

ornate topaz
#

And this is after @warped crater already "fixed" it fufu

warped crater
#

looking back - yeah, it was obvious that would've never fixed it

#

module.exports is being used here since export = (the proper syntax for this) would throw an error

#

since TS does not like that along with any other exports in the file

#

me and I think Vlad(?) also suggested removing module.exports =, but we were shut down by Crawl due to it being a breaking change.

wild flax
#

they can be removed now

warped crater
#

how do you wanna go about versioning?

#

per semver since we're on 0.y.z we can just bump y for breaking changes

ornate topaz
#

things are getting deleted literally now

warped crater
#

oh, lovely

#

I'll PR then

wild flax
#

we cant remove the default export

#

so doing export class X wont work

#

except TS does some magic that you can do export class X and then still do export { Collection }

#

not sure if it allows that

vivid field
#

it allows you to do both export class X and export default X

tacit crypt
wild flax
#

thats not what im saying

#

pure class X acts as an assignment

#

like const x = class X

tacit crypt
#

uhuh..

wild flax
#

but does export class X do the same

#

so you can have export class X {} and then later do export default X

tacit crypt
# 
let AttachmentArchive = class AttachmentArchive {
    ticketID;
    originalURL;
    archiveURL;
};
// decorators here l o l
exports.AttachmentArchive = AttachmentArchive;```
#

short answer, yes

wild flax
#

so TS itself, before compilation doesnt complain?

tacit crypt
#

it doesn't

wild flax
#

aight then, sure

warped crater
#

ah, so we're doing that? sure.

real jetty
#

wait so is v13 gonna be a ts-rewrite? or naw

snow crypt
#

no

raven juniper
#

d.js-next will be a ts rewrite. v13 is planned for the api v8 release, which is coming out sometime in the near future

real jetty
#

ohhh kk gotchu

clever crypt
#

Is there a specific reason why some api-route crafting functions use foo[id] instead of foo(id)
For example:
https://github.com/discordjs/discord.js/blob/8a7abc9f06d44bf693e35a615bb6ba2c3eb1d6e7/src/managers/MessageManager.js#L84 is ```js
this.client.api.channels[this.channel.id].pins.get()`

<https://github.com/discordjs/discord.js/blob/8a7abc9f06d44bf693e35a615bb6ba2c3eb1d6e7/src/managers/MessageManager.js#L126> is ```js
this.client.api.channels(this.channel.id).messages(message).delete({ reason })
oak quail
#

probably not

#

i think the convention is to use () for ids and [] for url parts

snow crypt
#

there really is no convention

snow crypt
#

i honestly believe that we should remove the possibility of () for those

#

keep calling the function to the request itself

snow crypt
#

I've just PR'd a change to enforce brackets

unique axle
#

why?

snow crypt
#

@unique axle you commented on #5256 referencing #5256?

#

i've updated what you pointed out, though

unique axle
#

i'm smart.

#

can you please wait for feedback though, before you apply reviews regarding a change you propose on other prs?

#

this is far from decided and suggesting that as required changes on the api v8 PR is not a good thing to do yet

snow crypt
#

you're correct, my bad

#

and as for why it is needed

#

consistency, mainly

real jetty
ornate topaz
#

because API sends member object with the message

#

See

** The member object exists in MESSAGE_CREATE and MESSAGE_UPDATE events from text-based guild channels. This allows bots to obtain real-time member data without requiring bots to store member state in memory.
under link above, and
The field user won't be included in the member object attached to MESSAGE_CREATE and MESSAGE_UPDATE gateway events.
under https://discord.com/developers/docs/resources/guild#guild-member-object

real jetty
#

I see, thank you!

old kelp
#

is there a reason why PermissionOverwrites doesn't extend Base? assuming they do have IDs

unique axle
#

Represents a data model that is identifiable by a Snowflake (i.e. Discord API data models).
PermissionOverwrite is not. The ID you refer to is the ID of the target of the overwrite (user or role)
this ID does not uniquely identify an overwrite (you also require information about the channel)

vivid field
#

what about e.g. voicestates? same thing should apply there

unique axle
#

going by the description of Base, yes

#

unless i'm misunderstanding the description and whoever wrote that does mean a snowflake to be used as key for a map containing the structure, which then would also make scoped unique valiable

vivid field
#

i think you're right about the description but it's not really consistent there

#

there can even be some Bases without any id, like reactionemojis representing unicode emojis

unique axle
#

indeed potatodetective
consistency is noticeable by lack thereof

old kelp
#

so would the decision be to change the description of Base or go forward to make the class extend Base?
considering that's (almost) the same case with voicestates. or rather, the description seems quite misleading at this point unless your first guess was correct that it holds "structures mapped by IDs"

wispy forum
#

Just a little bit of curiosity, why is it that the client events don't pass some sort of indication of who did the said action, rather than just giving the old and new object? Is this subject to change, am I missing something? Or is currently the only way of getting the user that did an action just fetching from audit logs for it?

unique axle
#

we can only provide information we get from the WS, discord does not give that information without requesting the audit logs

wispy forum
#

Ah, alright thanks

snow crypt
#

What if we removed Util methods from the default export?

#

I really can't find a case where I preferred to use them over importing Util

patent halo
#

i think someone uses it

#

the Util.mergeDefault comes in handy

snow crypt
#

i'm not saying remove util

#

but you can currently call Discord.mergeDefault like Discord.Util.mergeDefault

oak quail
#

why make -next a monorepo?

copper laurel
#

Developer convenience, theres no reason for it not to be right now I think was the general sentiment

tacit crypt
wild flax
oak quail
#

separate repos?

warped crater
#

but why

#

monorepos are so much easier to work in

#

its the common approach nowdays for large projects

snow crypt
#

apart from the fact that you can't install a single package from the branch like npm i discordjs/discord.js

#

so you can only work with the repo itself / the releases

wild flax
# oak quail separate repos?

so you have to update 4 different repos, then go to the main repo, change all the deps to latest and push another update?

#

essentially switching 4 repos for 1 change?

oak quail
#

well not every repo would need to be updated for every change, I assume most changes wouldn't need that

solemn oyster
#

We would, @oak quail

#

For example, most of the monorepo's packages will depend on @discordjs/core, however, there are dependencies between them, e.g. @discordjs/rest depends on @discordjs/core and @discordjs/collection, similarly, @discordjs/ws depends on all the previous (including rest)

vivid field
#

are we going to rewrite collection too?

tacit crypt
#

It's already rewritten? Unless I'm missing something

snow crypt
#

@mystic meadow what if you had to import a version for discord-api-types and just import 'discord-api-types' would not be possible?

vivid field
tacit crypt
snow crypt
#

you currently can import any of:

import ... from 'discord-api-types';
import ... from 'discord-api-types/v6';
import ... from 'discord-api-types/v8';
#

i am sort of suggesting to remove the first option

#

so no api version confusion ever

tacit crypt
#

To remove the default version export? while doable, you need to specify SOMETHING on the main/types properties of the package.json

snow crypt
#

you don't

tacit crypt
#

I'm still against dropping it as there are still exported properties that aren't versioned (JSON errors for one)

snow crypt
#

then you could have disocrd-api-types/common or discord-api-types/errors or any other thing

#

just eliminate the default version

real compass
#

when the ts recode comes eventually, how will the typings look? will there be one typings file or several?

#

and I’m assuming they’re still being exported

vivid field
#

the purpose of writing source code in typescript is that you wont have to create declaration files at all, the compiler does that for you

real compass
#

oh so everything is just gonna be defined on its respective class

real compass
#

How will they be exported?

clever crypt
#

export * from '...'

real compass
#

Yeah but how is that gonna work on like a Message structure

clever crypt
#

I assume that'll exist within the core

tacit crypt
#

Structures that are meant to be extended will be exported possibly the same way as they are right now, CC @solemn oyster

wild flax
#

Extending structures in -next is gonna be completely different to how it works right now

#

There is no way this will be comparable

tacit crypt
warped crater
#

I'd assume just a DI approach such with something like tsyringe

wild flax
#

I wrote this like a month ago, mind you, Im sure I can come up with something better, but youll get the gist

# 
import "reflect-metadata";
import { injectable, inject, container } from 'tsyringe';

@injectable()
class CustomChannels {
  public test = 'test2';
}

container.register('CUSTOM_CHANNELS', CustomChannels);

@injectable()
class Channels {
  public test = 'test';
}

@injectable()
class Client {
  channels = container.resolve(this.customStructures.CHANNELS ? this.customStructures.CHANNELS : Channels);

  public constructor(public customStructures: any) {}
}

const client = new Client({ CHANNELS: 'CUSTOM_CHANNELS' });
console.log(client);
#

but it goes into that direction

warped crater
#

👍 from me

tacit crypt
#

That would only work with TS tho

wild flax
#

Nah, tsyringe also works with javascript

warped crater
#

mmm, I remember reading something about that

#

but what are the approaches as an alternative to the decorators?

exotic flicker
#

you can just register them to the container I guess

wild flax
#

Not that I care about pure JS users

warped crater
#

not... certain I share that sentiment given djs was always known as being very user friendly and easy to get into, compared to other libraries

tacit crypt
#

then again, extensions are not easy to get into

warped crater
#

as much as I also hate vanilla js

tacit crypt
#

as long as its doable in raw JS, i'm down

ornate topaz
#

discord.js that doesn't fully work on js 🤔

wild flax
#

so we could either use inversify, or just write our own decorate helper

#

Its not nice, but i mean thats what you get when you write JS

#

nvm we dont even have to

#

we can just use Reflect lmao

exotic flicker
# wild flax ```ts import "reflect-metadata"; import { injectable, inject, container } from '...

I mean, you dont really need the decorators

require("reflect-metadata");
const { injectable, inject, container } = require('tsyringe');

class CustomChannels {
  test = 'test2';
}

container.register('CUSTOM_CHANNELS', CustomChannels);

class Channels {
  test = 'test';
}

class Client {

  constructor(customStructures) {
    this.customStructures = customStructures;
    // kinda fucky wucky idk
    this.channels = container.resolve(this.customStructures.CHANNELS ? this.customStructures.CHANNELS : Channels);
  }
}

const client = new Client({ CHANNELS: 'CUSTOM_CHANNELS' });
console.log(client);

http://images.chilo.space/rw0x

wild flax
#

wait so you dont need to mark them as injectable?

#

I thought that was a requirement

exotic flicker
#

you do, by using register

warped crater
#

only if they actually need to.. inject something

#

oh, am I misunderstanding the decorator too?

wild flax
#

oh, and since we need users to register it anyway... I guess that makes sense

warped crater
#

I thought @injectable did implicit resolving so you didn't have to do @inject on every parameter karenconfused

#

yeah nevermind, it makes sense now

exotic flicker
#

uhhh yeah dd is right, but I guess as long as we dont do that?
either way you can just fall back to container.resolve in the constructor

wild flax
#

we need to always resolve anyway for all custom structures

warped crater
#

am I? I can't even tell anymore.

#

oh, that's right

#

@inject all the parameters.

exotic flicker
#

Class decorator factory that allows the class' dependencies to be injected at runtime. TSyringe relies on several decorators in order to collect metadata about classes to be instantiated.
buut, if I'm not mistaken, you can just do

injectable(CustomChannels)

and get the same result

solemn oyster
#

@wild flax whatever we use, we'll need something to auto-inject or something

compact spade
#

is there a reason to use DI over Structures.get/extend?

warped crater
#

Yes, Structres.get/extend is budget DI

wild flax
#

Yeah, the current structures are ass

solemn oyster
#

Client for example is something that needs to be able to resolve in all structures

compact spade
#

what are the advantages of using "complete DI" over "budget DI"

wild flax
#

Structures arent DI

compact spade
#

thats why i said budget DI

wild flax
#

We just hack together JS to make what they are right now

warped crater
#

yeah.. it may look "clean" on the outside, but internally the current structures system is very difficult to navigate and understand

compact spade
#

whatever they are, they seem to work fine rn. are there any major shortcomings with them?

#

hmm

warped crater
#

this is similar to a lot of things in the package, for that matter

#

such as actions

#

nobody has any idea how they work anymore

wild flax
#

Yeah, it won't be achievable like this in TypeScript without a lot of Type-hacking

#

And its certainly just a pain in the ass

solemn oyster
#

We still need to augment modules, regardless of whether we use DI or not

exotic flicker
#

what I dont get rn tho, what about the types?
lets say you extended Channels, message.channel would still have the old type, no?

wild flax
#

thats why you have to augment

compact spade
#

do you have to augment even with tsyringe

wild flax
#

yes

solemn oyster
#

Logically, yes

#

TypeScript doesn't have anything to do that automatically, types are static, not dynamic

#

To be honest, I hope we go for resolving the client in all structures on demand, rather than storing a reference to it as a field in all of them

wild flax
#

that only works well in TS

solemn oyster
#

We can always make our own solution if you want

#

So it works well for both JS and TS

wild flax
#

You make it sounds so easy

solemn oyster
#

I mean... nothing stops us from doing```ts
import { container } from 'tsyringe';
import { Tokens } from '#lib/util';

export class Base {
public get client() {
return container.resolve(Tokens.CLIENT);
}
}```

compact spade
wild flax
#

Why would you?

compact spade
#

big bots who microservice parts of their code would find it very useful, and i don't see a reason not to make that an option

wild flax
#

?

#

You dont seem to understand -next very well

#

You can already use the rest package independent

solemn oyster
compact spade
#

that's not what im talking about at all...

solemn oyster
#

And that

compact spade
solemn oyster
#

REST is just a controller, but the hard work is done in IHandler

wild flax
#

Yeah its just that your explanation why you want it modifiable didnt make much sense

exotic flicker
compact spade
#

well handler is just a request handling strategy right? as-in burst or sequential?

solemn oyster
#

Generally, DI containers are global

#

And... what even is your use case for more than one client per process?

compact spade
#

thats not what im talking about kyra, im talking about REST being an interface so people can use their own controllers (maybe one could proxy requests to a server that does ratelimit handling, like spectacles' proxy)

solemn oyster
#

I'm talking to CHY, sorry

compact spade
exotic flicker
solemn oyster
#

If you want to make your own REST, just do so, Stitch

wild flax
solemn oyster
wild flax
#

Except you use the exact same symbols

compact spade
#

okay cool, that works

solemn oyster
#

LGTM on the usage of symbols for this

tacit crypt
tacit crypt
#

IF we want to truly make REST extendable, the interface needs to be VERY strict

compact spade
#

technically i can already do that extending REST, can't i?

tacit crypt
#

Unless you overwrite the RequestManager's function that returns a different handler, no

#

that would mean going down the chain

#

even then

#

what you need is to just change api endpoint

#

to your proxy

#

and leave the rest to us

compact spade
#

the point of the proxy is it handles ratelimits, not whatever process that is using it

tacit crypt
#

yeah and?

#

You forward all requests to the proxy

#

and return some form of headers that just tell us we can keep requesting (limit 5, remaining 5, boom done)

compact spade
#

okay fair enough i guess

tacit crypt
# 
const proxiedRest = new REST({ api: 'http://localhost:1234' });

proxiedRest.get('/gateway/bot');
compact spade
#

the one proxy that im referring to returns no ratelimit info

#

so it wouldn't work with your proposal

tacit crypt
#

we have defaults anyways

compact spade
#

are they set to zero for everything? XD

tacit crypt
# 
        // Update the total number of requests that can be made before the rate limit resets
        this.limit = limit ? Number(limit) : Infinity;
        // Update the number of remaining requests that can be made before the rate limit resets
        this.remaining = remaining ? Number(remaining) : 1;
        // Update the time when this rate limit resets (reset-after is in seconds)
        this.reset = reset ? Number(reset) * 1000 + Date.now() + this.manager.rest.options.offset : Date.now();
#

Infinite limit, 1 remaining, if no headers are present

#

so basically

compact spade
#

handling ratelimits in the rest manager would defeat the purpose of a proxy

tacit crypt
#

it'll work just fine

solemn oyster
#

Thanks

solemn oyster
#

I don't recall any place in our code taking an https.Agent instance (which iirc is required for proxies to work)

wild flax
#

I mean there is no point

#

its not even how spectacles works

#

you dont use spectacles REST with spectacles proxy

solemn oyster
wild flax
#

you use only spectacles proxy and write your own quick rest handler

compact spade
#

yes

#

and if i wanted my d.js bot with it, that wouldn't be possible if the d.js rest-handler is enforcing ratelimits itself

wild flax
#

...?

tacit crypt
#

Stitch

wild flax
#

Why would you do that?

tacit crypt
#

you do understand that, if no headers are present, it'll just...not..deal..with ratelimits right

#

A limit of Infinity and remaining of always 1 means it'll just pass through

compact spade
#

ah is that so? nice

wild flax
#

Thats like me saying, I have the same bot on an eris music bot, but all the other functionality on a d.js bot

#

Why cant I use the same proxy?

#

Thats such a weird request

compact spade
#

it's not as weird as you make it out to be. kyra, for example, wants to microservice everything Moderation-related from skyra. if the new service is doing anything REST related, it's gonna need a proxy for both applications to keep track of ratelimits

wild flax
#

Yes, but he uses libraries that are meant to work that way

#

You can't shoehorn this solution

#

Its the reason I use spectacles, and not a mix of spectacles and djs and eris

#

I just have a single message broker for example that makes sure all things go into the correct proxy

#

If we'd support every use case for this, this would mean our "unknown error" ratio would go through the roof because we can not guarantee someone doesnt mess with something we have no control over and suddenly want support for it

compact spade
#

but wouldn't -next's rest handler being customizable be a good solution to this? it uses the proxy, and we can still use all the convenience methods d.js provides. and if vlad is right, we can do that already

wild flax
#

I don't think you understand how this works

#

If you want multiple bots to proxy into a single ratelimit, you would build a server that WRAPS -next' rest handler

#

And then make requests to that wrapper

#

Not the other way around

compact spade
#

yes, and making REST an interface would solve that?

wild flax
#

Thats exactly how spectacles works

#

No?

#

How?

#

You wouldnt need any of the rest handlers logic

#

The proxy handles that

#

You just blindly fire request to the proxy, doesnt matter what it is

compact spade
#

lets assume REST is an interface with get(), post() and put(). i implement it to send http requests to my proxy server, which then does the actual ratelimit handling.

tacit crypt
#

No, crawl, what Stitch wants is to forward all requests done through -next's rest to some local endpoint, to a global "proxy" (in go or otherwise) that handles the rest

wild flax
#

I understand that

tacit crypt
#

which, as I said, is already doable

wild flax
#

But thats unneeded

compact spade
#

that way i can use message.channel.send() and it works with my proxy 😉

tacit crypt
#

...

wild flax
#

Yeaaaaah, thats exactly what I meant with shoehorning a solution

tacit crypt
#

Y'all ignoring me intentionally? Stitch, just override the api endpoint. That's all. If your proxy runs and it catches all requests and works PROPERLY (do note its your responsibility if you fuck ratelimits up and get banned), d.js will just pass requests on

#

or rather, not d.js but -next's rest

#

IN FACT, the EXACT SAME is possible right now in v12

#

Same logic

compact spade
#

yes vlad i gotcha, i did mention you said it was already possible

tacit crypt
#

override the api base

wild flax
#

You talk about microservices but say stuff like message.channel.send when you wouldnt even theoretically have a cache like that in mutliple services because its not feasible

compact spade
#

that wasn't the point, the point is i can continue using discord.js' REST functions with my own proxy and handler

tacit crypt
#

you

#

dont

#

touch

#

the fucking

#

handler

#

You strictly change 1 option

compact spade
#

yes vlad i get it, don't worry

wild flax
#

If it wasn't the point don't bring up the points of microservices

compact spade
#

now you're just taking whatever i say out of context, im not continuing this, especially since the "issue" is already resolved anyway

wild flax
#

If you continue to dismiss the claims that proper microservices do not rely on a monolithic library like d.js to properly function and just try to have it your way, go ahead.

#

You brought examples into this with skyra that have no foundation because its simply not how you would even design such a system

solemn oyster
#

My opinion on this is that we shouldn't go too much overboard, -next is still very young and while many things are in the design stage, most aren't even in that step, naturally, -next will evolve with time just like any other OSS project

compact spade
#

¯_(ツ)_/¯

#

if you say so, sure

solemn oyster
#

I personally think that having a functional REST package that works for the typical case, is priority

#

We can enhance it later with things we might need in the future, but as for now, we aren't very sure about everything

wild flax
#

Nah, its easy, just do it

#

Don't worry.

solemn oyster
#

I'd say give the new REST a shot once it's released, and if you can find a solution (Crawl and Vlad proposed some) that works for you, then you don't have anything else to worry about

tacit crypt
#

Kinda like what I want, a handler that spreads out requests evenly in the reset window (so if we can do 4 requests a second it'd spread them out as evenly as possible), but thats offtopic

real compass
#

^ conversation relating to the ts recode and exporting interfaces

solemn oyster
#

There are chances that we might keep Structures, mostly because it's the polar opposite of DI

#

The only feasible alternative a friend of mine (who's well versed in DI and many other programming patterns) was the transformer pattern, which is basically the same as Structures but less optimised and bulkier

snow crypt
#

@tacit crypt @unique axle my point with the PR is to e n f o r c e the consistency, not just change all cases into using the same bracket type (have no idea how to word this).
With the PR in place it means: route building is property access, requests are function calls, no way for it to be confusing like that.
Currently (even with the PR), you can still use .guilds(id), so if the PR gets merged and turns out I missed some changes, the library won't break.
When we're sure that all places use the correct way, we can make it error - e n f o r c ing the new style.

solemn oyster
#

To be honest, I'm unsure if we'll go for that path, we can very well replace REST with -next's once Smugen's PR lands (since we need the other changes)

tacit crypt
#

^

wild flax
#

I'm more in favor of x.y.z.post(), x.y(id).z.post() or x.y(id).z(member).post() than any other variation

snow crypt
#

unenforced consistency

wild flax
#

I think thats a pretty consistent pattern

clever crypt
#

Would it be a good idea to add some sort of error throw on GuildMemberManager#fetch if the correct intent isn't enabled?

ornate topaz
#

wouldn't that be done on api side already

clever crypt
#

it'll just timeout

snow crypt
#

Could Message#authorID be added for partial messages?

clever crypt
#

hmm .. i mentioned something similar yesterday to be used with Guild#owner, suggesting it should be GuildMember | PartialGuildMember. the only 'valid' property of PartialGuildMember is id and it has a fetch function similar to GuildMember#fetch

perhaps the same thing could be applied here? PartialUser where only id is valid with a fetch function?

unique axle
#

what's the reasoning for Message? seeing that both user and member data are included in the message payload?

real compass
#

does running .fetch add to its respective cache?

tacit crypt
#

yes

#

unless you pass false to the cache parameter

snow crypt
#

@unique axle author.id is known in MESSAGE_UPDATE, so I can save an api call by using it on the partial structure and not fetch it.

#

all i need there is message.id, message.authorID, message.content

#

which I should not have to fetch

oak quail
#

so whenever djs has a message it should have an author object

#

so authorID would never be present if author.id isnt

copper laurel
ornate topaz
#

Huh

#

Oh, hm

snow crypt
#

@oak quail but it is not

#

when message.partial is true, message.author.id throws cannot access id of undefined

oak quail
#

well that means that djs is constructing a message object from just an id

snow crypt
#

why though

solemn oyster
#

Try enabling user partials, @snow crypt

snow crypt
#

Still (node:7640) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'id' of null when message.partial

copper garden
snow crypt
#

yes, no

#

I have edited the literally oldest message I could find in a server (which is only 1.5 years ago, but still not very new), and MESSAGE_UPDATE (using client.ws) emitted with author

copper garden
#

Was it a webhook message?

snow crypt
#

no

copper garden
#

Can you show the data object?

snow crypt
#

well i can redo it in a moment

wild flax
oak quail
#

well in that case you... still wouldn't have the author id

#

nothing djs can do about that

snow crypt
#

yeah fml

#

see above the raw event with author.id and then messageUpdate where newMessage.author is non-existent

#

@copper garden

copper garden
#

Looks like the method assumes that it'll either get a message object like { message: Message {...} } or just { id: "...", channel_id: "...", guild_id: "..." } (partial message)

copper laurel
#

We looked into this the other day, getMessage doesnt pass the author to getPayload

snow crypt
#

and.. is that intentional?

copper laurel
#

Probably not 😄

wild flax
#

@ruby terrace I don’t see how that our job to maintain lol

#

And as I read that, several other libs decided against too

ruby terrace
#

It's not something that the library should handle automatically, it should be a developer choice, but the developer currently can't make that choice

wild flax
#

Huh

#

I would argue it’s discord’s fault and that’s it

#

We do exactly as it should work

#

It just so happens that discord fucks up

ruby terrace
#

I can't think of one of the top of my head, but there is a point to be made for a use case where you do want to change solely allowed_mentions.
I know this is a stretch, but maybe mentioning a user, but disabling the highlight afterwards.

wild flax
#

That seems very cosmetic at most

#

Which would fall into the argument of a noop IMO

ruby terrace
#

Editing with allowed_mentions is always going to be cosmetic, even with content come to think of it

wild flax
#

I’m not against a fix, but I sure wouldnt put any high priority on it

ruby terrace
#

I wonder if it affects the inbox