#archive-library-discussion

25085 messages · Page 5 of 26

frank turret

I don't see this as being a breaking change, so I don't see why it wouldn't come to v12

empty viper

Oh I understand, breaking changes comes into new releases ?

unique axle
frank turret

:oops:

hadn't realized interactions were v8

unique axle
empty viper

Mh I see, I was thinking that you have to change the MAJOR version when there is a big feature that comes

copper laurel

Standard reminder that -next is not the next major release either way

wide sand

so there is no option for INTERACTION_CREATE

ruby terrace

That's actively being developed, please check the pull requests (you can also get the raw event)

wide sand

i see

wide sand
ruby terrace
raven juniper
oak quail

@raven juniper client.ws.on works and is public

raven juniper

i see nothing about it on the client.ws docs but i'll take your word for it, i don't mess too much with that stuff, i just know it's a pain and people looking for support aren't likely gonna get any because not many people know it

tacit crypt

we don't document what events we emit on client.ws for..obv reasons

But, any packet that has a t will get emitted

unique axle
snow crypt

Is .tern-profect still needed?

kindred haven

Should Client#token really be instantiating to process.env.DISCORD_TOKEN if it exists?
The behavior seems really odd

ornate topaz

what's wrong with it?

kindred haven

Nothing in particular
just wondering why it was added
it seems like it would only be useful for env vars but nothing really else

ornate topaz

this way you don't have to "store" token in your code

merry python

it's also used by the sharding manager to pass the token to the shards

ornate topaz

and considering that there are million other ways to populate process.env than dotenv, you can do it from outside of your bot code or even node

kindred haven

ah k

makes sense

frank turret
unique axle

yes, search GH for PR, if none: make one

frank turret

Make one, got it

unique axle

haven't done step 1 yet, so that might need to happen before WonkBOYE

frank turret

02facepalm there is one nevermind

real jetty

is there support for the new /command system?

oh thanks

dawn lodge

Nico beat me to it 👀

dawn lodge
frank turret
dawn lodge

Oh nice, already a PR

I assume this will arrive in v13 as well?

frank turret

I don't see why it wouldn't come in v12, it's not a breaking change and the PR doesn't specify whether only the v8 api would have it

quaint bronze

I'm not sure but I think it makes more sense to have an event for that maybe GuildMemberAccept it emit when the member accepts the rules so we can do something like sending a welcome message or giving a role

old kelp

we cannot really implement something custom if the API does not support natively (which makes it a way unreliable), so that is upto them, then surely.

ruby terrace
wild flax

This is correct and it needs API v8 so it’s a v13 only

ornate topaz

How does it require v8 though?

oak quail

it doesnt

wild flax

I have not touched v6 in a long time but gus mentioned the v8 PR in his

Do registering commands / the gateway event exist in v6?

ruby terrace

oh, we were talking about the guildMember#pending property. As far as v6 for interactions, I've heard they work just fine, but we should be on v8 at that point anyways

wild flax

Right. I was looking at the wrong PR then.

Either way, very slim chance this will be released in the 12 cycle, as semver:major prs already landed in master

frank turret
unique axle

why? should be the question, rather than "why not"

it emits guildMemberUpdate, you check #pending on old and new

if the end user wants to make that its own event they are free to use the node vent API to emit it, but i don't see any benefit in doing it on the library level

ruby terrace

There's also been a lot of push to make that event a native API event, but as long as we get a difference in pending state, then it shouldn't matter, that's an easy check to make.

unique axle

and that push is just as nonsensical from discords perspective, it emits an event, that single check should not warrant it being its own event

  • nvm, intents have happened, people want to operate without cache

ofc, if it becomes its own gateway event it will get its own Client event in discord.js - as it is GUILD_MEMBER_UPDATE currently however at the present time that makes no sense on the library level

ruby terrace

I doubt it will become its own event at this point, supposedly we only get the pending parameter (set false) on the GUILD_MEMBER_UPDATE event after someone finishes screening. That should be enough for people without cache too.

old kelp

agreeing Souji and ckohen on that, I guess must be fully enough to make a (single) check against the .pending-property (as GUILD_MEMBER_UPATE already emits for that specific change), and except if you're messing with partials.

vivid field
vernal atlas
vivid field

oops, didn't notice that integration applications have their own part in the docs

thanks

hollow salmon

Can I ask here if the Collections utility is still maintained or would that be off-topic?

vivid field

it is maintained

proper dock

I was asking some questions in the Discord Development server, and someone mentioned that if discord.js hits a rate limit, it waits until the limit expires and then tries the call again (#697489244649816084 message). Is that accurate? I feel that when I call a method that might be rate limited, I should be able to pass in options to tell it whether I want it to retry and/or whether I want it to throw an error. At the very least, it would help in debugging. But ideally if the bot can't do something right now, I'd like to be able to act on that. My bot could say something like, channel.send('Sorry! I've been doing that too fast, and I'm not allowed to right now. Please try again later.');

slate nacelle

The behavior you are describing is correct.
Something like an option to limit retries that are longer than x or the like have been brought up, but nothing really has been done in that regard so far.

proper dock

Oh, interesting. I missed that. So, something like setTopic - I'm limited to 2 every 10 minutes. Does this fire when I've done the second one, or when I attempt the third one?

So discord.js will still make that attempt later, right?

Is there any way to stop it from re-trying that request?

slate nacelle

You would get a debug log there, rate limit only emits when the request is preemptively throttled. (That is indeed a bit weird)

No, there is currently no way to cancel a request.

proper dock

Are there any ongoing discussions about this topic? I'd be willing to take a stab at something and make a PR, but I'd totally welcome some guidance onto what the best way to handle all of this is.

Option 1: A map of known rate limits. We could fire a rateLimitWarning based on a map of known values: If you try that again in the next X seconds, it will be rate limited.
Option 2: Some hook on the current rateLimit event that does the current behavior by default, but the developer can do something with the event to cancel the request if required.
Option 3: Pass options into anything that might get rate limited to tell it what to do.

Out of these 3, it seems Option 2 is best to me. But maybe there are other options I haven't thought of.

slate nacelle

I am not aware of any discussions currently (or recently) about that. (I am pretty sure there was something about that, but I can't find anything at the moment.)

1 is a no, since Discord might change limits at any point, and is generally not happy about hard coded limits.
3 is possible, but feels like it could become a hassle since everything is subject to be rate limited.

2 indeed looks like something that could be done, although I'm not too sure how it would work exactly.

Some possible naive solution could be to add a function to the object that, when called, cancels the request (causing the returned promise to reject with some error).

My initial idea was to be able to configure a static limit for rate limits as client options, but that might be a bit too broad, while yours would allow better control about when to (not) wait.

proper dock

So - I haven't FULLY read the code yet. Just given it a quick glance. Requests go into a queue, right? If we attached a UID to each request, we could pass back the UID in the rateLimit event. Then we could provide a method to remove an event in the queue by passing in a UID.

slate nacelle

They go into a queue, one per route.

proper dock

Okay. So if we have a method called something like cancelRequest(route, uid), that might work.

Find the queue for the route and in there, find the request with uid...

And although a bit confusing given the rateLimitInfo.method there, it might also be interesting to pass back the name of the method that was called. For example, rateLimitInfo.callingMethod: 'channel.setTopic' or something like that.

Unless that's information that's in the route already. I'm not familiar with how the routes are named in the API.

I guess my only remaining question is - if I tackle something like this, how do I ensure I'm doing it in a way that will be accepted in a PR? I don't want to do the work, and then have someone say, "no, we're not implementing this." That's the only reason for all of this discussion/questions. So what do I need to do/to whom do I need to speak, to make sure that the spec work I'm doing describes work that will be accepted?

slate nacelle

Somewhat, I don't think there are too many, if any, not-helper functions sharing the same route.
What public-facing-method caused the request is currently not known to the request handler.

That is indeed a good and reasonable question, which I do not really have an answer to jynEHEH

I'd be +1 on this feature, but think it could be troublesome to actually cancel a request that currently waits for a rate limit to expire (as that's just a promisified setTimeout, and polling some cancel flag does not feel too right?).
But I do not really have another idea about how that would work.

proper dock

This'll be my last post. Please @ me if you reply so I can check it later. If we put the setTimeouts into an object with that UID I was talking about as the key, then we could do clearTimeout(queue[uid]). Maybe. Anyway, I'm off. Merry Christmas. Thanks for the chat. Is it correct that I'll need to make an Issue describing my spec ideas and wait for a +1 from you and/or someone else to know it will be accepted? any clarification on how I can only do work after knowing it'll be accepted is appreciated 🙂

vernal atlas

im refactoring usages of GuildChannel#permissionsFor to handle there being no permissions returned, but in the getters should i just return null here or default to false?

null feels like better option but i just want to be sure

loud moat

There's a difference between false and "we don't know", right?

vernal atlas

yeah

i ask also because

GuildChannel#viewable just returns false

if there are no permissions

but all the other getters just error

old kelp

why not also take GuildChannel#viewable under consideration to be removed (relative to all other removals for v13)? I think it has been discussed once

<#archive-library-discussion message>

I believe what nico is referring to is removing stuff like GuildChannel#viewable, the basic permission checks that the user can very easily implement
agreed there, users must be able to do basic logical checks 🤔

real jetty

hey uh guys, i heard you guys are removing the like <message>.delete({timeout:500}) functionality in v13, and im a bit confused as to why?

i dont see any downsides personally to keeping it, and it really helps keep code clean

rather than having a lot of settimeouts which makes code kinda messy

if someone has a moment can they please like gimme the reasoning behind this? thanks!

unique axle

it should never have existed in the first place.
no other structure has a native delay on delete, why should message have one? this is nothing the API actually supports but just a timeout wrapper, which the dev can do themselves to the same effect through Client#setTimeout

ruby terrace

it also forces you to think about how to handle deleting messages after a time (such as checking if it is actually still deletable)

real jetty

yeah, personally I understand how like no other wrapper has a native delay for deleting and stuff, but since it's already like there, and it doesn't exactly have any downsides, I'm not getting like why you guys are removing it
(sorry if i seem pushy, im just genuinely kinda confused)

unique axle
vernal atlas

if you miss the functionality you can always add it back by extending the structure yourself

real jetty

oh, oof, i guess that kinda makes sense, Personally I still feel like the amount of code a lot of devs will have to re-write isnt worth like removing functionality, since theres other options if you dont wanna use it, but I guess that like reasoning kinda makes sense, so thanks!

unique axle

we generally are moving forward with removing inconsistent and unnecessary "convenience" methods, since they have and still do cause more pain than they relieve

real jetty
real jetty
unique axle

amount of code a lot of devs will have to re-write isnt worth like removing functionality,
this is not a consideration we make for major version updates.
we are moving forward with what's the more consistent, coherent and ultimately hopefully semi-straightforward API

real jetty

ohhh kk gotchu, ty!! 👍

carmine socket

Hey there. I don't think my question is 100% related to Djs, but I'm wondering how events work. (ex. client.on('event', callback) in d.js). I first thought that the Discord servers were sending a request to the local machine to trigger an event, but it seems very unlikely. How is the message event triggered for example? Is it some kind of infinite loop that keeps asking for updates over HTTP? (please ping to answer)

vivid field

updates aren't sent over http, your client opens a websocket connection to the discord gateway where it receives events on client.ws and forwards them to the user

carmine socket
real jetty

Could the member who deleted /created a Channel be added to channelDelete and channelCreate? It could be pretty useful to a lot of things.

unique axle

we can only provide information discord provides, so that's a "nope"

"who" did something is always accessible through audit logs, not the general event that emits when it happens

real jetty

Sad that discord does not provide that directly in the event

solemn oyster
real jetty
latent crag

Should i pull request directly to the master branch or PR a new one? If creating a new one, how do i name it?

wild flax

You should never commit directly to master

And whatever seems most logical to you

latent crag

Do i name branch using my github nick and PR to djs repo then?

ornate topaz

it doesn't really matter. you're going to create a branch on your fork, which would just get merged into one of branches at the upstream (or closed)

grizzled rover

Not sure if this is the right channel but what is message.nonce even useful for? I was just reading docs and couldn't really see a use for it other than maybe checking if you already fetched the message?

frank turret
solemn oyster

It's something you send that you can use to verify when you get the response back, e.g. if you send a message with nonce foo bar, you'll get a response that will be a message with nonce foo bar.

However, all other clients (and even you when fetching the message) won't get said nonce again. Think of it as an integrity check. @grizzled rover

ruby terrace

I guess this got purged on accident:
The current behavior of the USER partial only affects reaction events and typingStart. Since userUpdate is an internally emitted event through GUILD_MEMBER_UPDATE in most instances, it makes me question whether userUpdate should be emitted for an uncached user. Right now it does not, and other Partial types behavior seem to suggest that it would. Does anyone have any insight into whether this was just an oversight or if its intended.
I definitely see how it could be intended since most GUILD_MEMBER_UPDATE events have nothing to do with a user update, and this would always emit userUpdate if the member was uncached, even when only member parameters have changed.

unique axle

i'm currently questioning if userUpdate should be tweaked at all, or if it should just be emitted for current user changes, as the API emits it

ruby terrace

Yeah, I was highly considering suggesting just to remove the external handles into userUpdate completely

old kelp

i'd personally suggest that, i don't think so there is any use to emit it artificially other than convenience, which rather makes it inconsistent and harder to manage on the library level 🤔

ruby terrace

And to further that point, the event is probably significantly more confusing for users now that at least one of the privileged intents are required to get the event. I think this would go hand in hand with many of the other changes for v13 that aim to emulate the API more directly.

empty viper

I have a question, why on GitHub can't we see the versions prior to v5, and where can I find these ?

vernal atlas

npm install discord.js@version

all versions before v11 are deprecated and probably dont work / are very broken

empty viper

I know

but just for seeing the code

oak quail

why aren't the v11 versions marked as deprecated in npm?

solemn oyster

Probably an oversight

But yeah, we should eventually mark v11 versions as deprecated, as they'll stop working next year

frank turret

Would we prefer having separate prs for removing all the unnecessary getters (like viewable and other dumb permission checks) or is it alright to have one collective pr for the removal of all unnecessary getters
and hearts to @old kelp for the reminder abt this blobReachReverse

undone ravine

don't understand why anybody would want to get rid of those

old kelp
unique axle

it's not rest, it's not gateway, i thiiiink caching can be interpreted as "methods that don't do any calls outside and don't interact with discord in any way"
could be that my interpretation of that label is wrong tho shibaSmile

nvm, it's utility, i'm a doofus

old kelp

ah, right, thanks catThumbsUp

frank turret
undone ravine
frank turret

Some of them are simple permission checks that the user can do on their own, unneccessary for the lib to provide methods for something they can do on their own imo. It also feels like a bit of pollution

copper laurel

This has been a debated topic for a while but people generally lean towards removal, for a cleaner library.

Having said that, I think the checks we're removing should be "ported" over to a guide page - there are other ways to support good practices.

frank turret

Agreed. One other thing I'd like to ask, if we have other methods that depend on the getter, should we leave it?

undone ravine

I don't think there's any good reason to remove the permission getters, specifically.

  1. They add almost no additional overhead to the library in terms of maintenance
  2. The tiny bit of overhead they do add (making sure the permission checks change to match up with Discord's behavioural changes) aids in making the users' lives much easier, ensuring they don't have to go through and make the same updates themselves
  3. They are substantially cleaner for users of the library vs. the alternative
  4. They give a simple, descriptive name for what a bunch of otherwise arbitrary permission checks are accomplishing
  5. They often build upon each other (see GuildMember#manageable -> kickable/bannable)

Seems foolish to remove these widely-used getters that aid in producing much cleaner code on the users' end in the pursuit of a "cleaner" library (they're not even messy, inconsistent, or obtrusive in any way that I can think of internally)

All you would be accomplishing by removing them is making an extremely common task much less convenient/clean, there's not even a downside to them existing afaik

lofty birch

what could perhaps be better is making them more consistent, as the above complains about them not being so

undone ravine

Don't think they were called inconsistent. I believe that was directed at the weird message delete timeout functionality that nothing else shared

frank turret

In my PR I outline that getters that are used internally (see point 5 of gawds message) wouldn’t be touched

undone ravine

Sounds like you'd be introducing more inconsistency by removing the ones that aren't, then

Because then you have some convenient permission getters with much smaller coverage vs. them generally being available on everything where relevant

In fact, I think these permission-related helpers should be expanded further. Any object -> action relationship should be covered, ideally. Additionally, the comparison should be able to made between any two arbitrary objects, not just with the client user/member (i.e. a GuildMember#canBan(GuildMember) method or GuildMember#canView(GuildChannel))

unique axle

just to clarify: you are suggesting we should essentially have a permission getter per flag on the member as well as a getter per overwritable flag per channel type
additionally relationship methods as well as - possibly the reverse GuildMember#canView(GuildChannel) - GuildChannel#canBeViewedBy(GuildMember)

undone ravine

the reverse, no, not really

and not every flag, just actionable items

undone ravine
copper garden

So ur saying we'd add fail-guards for permission errors to methods like TextChannel#send?

undone ravine

no

raven juniper
frank turret

Thonk should it really be the responsibility of the library to do simple permission checks

undone ravine

permission checks are rarely simple

copper garden

Why is Role.comparePositions static method still there? I thought the raw position moved to Role#rawPosition?

Or at least why is it still using Role#position

ruby terrace
undone ravine

Agreed. I think it is absolutely the responsibility of the library to abstract the [often obscure to those not intimately familiar with discord/its API] requirements behind whether you can do something to an object or not

frank turret

Hm, then should the PR be reduced to only getters like message#editable?

undone ravine

Personally, I don't really see any reason to remove that one. Just because it's much simpler than the others doesn't mean it can't change in the future. Plus, that would only bring inconsistency, IMO

That one is kind of unique in that it's not tied directly to a permission currently

ruby terrace

I feel like that may be necessary for an edge case that is coming soon (but this particular one puts a lot extra on the library): interaction responses are from the same user id, but aren't guaranteed to be editable. E.g. once the token expires, and those must be edited via webhook, not the same general edit method

frank turret

some of them like voicechannel#speakable and the likes are just wrappers for permissionsFor, which would fall under my meaning

ruby terrace

I think that wrapper methods like that were the original aim. This becomes a question of whether those actually want to be removed, as that encompasses all of the basic *.edit() wrappers as well. IIRC, that is a desired end goal, but needed further discussion.

oak quail

I agree with gawd here, I don't really see a benefit in removing these, it just adds more work for bot devs

I do support changing things which would have an actual improvement, like, iirc someone said changing channel/message type to numbers saves a lot of ram? although they were numbers in early v12 dev iirc; that has an improvement and doesn't really have a disadvantage other than having to change code

but removing these methods seems to have disadvantages with no advantages other than them just not existing

ruby terrace

I support those changes as well, those type from string to numbers changes would also align with the removal of strings for permission flags

solemn oyster

Probably before, I want to evaluate the impact the refactor will have to other PRs (merge conflicts mostly)

#5022 definitely needs to be merged before I can do that

Just pending review from Space

frank turret

So then is the decision to keep all the getters including ones like message#editable and voicechannel#speakable?

vernal atlas

is the reply-prefix branch gonna be deleted? mmLol its just been sitting there for months lol

snow crypt

the native replies are going to replies discord.js's reply, making reply-prefix useless

ebon radish

Would it be out of scope for the library to include a few handy regexes, like the elusive invite link regex

snow crypt

that specifically you can grab from resolveInviteCode

?docs DataResolver.resolveInviteCode()

rich iglooBOT
ebon radish

@snow crypt You can apply this to a whole message and get an invite code?

snow crypt

yes

you can also make that regexp global and match many

but that is outside the scope of this channel

dawn lodge

Has the library been updated to allow for the "pending" param upon join to support the screening feature?

frank turret

There is a pr for it currently, not merged yet

dawn lodge

Okay, thanks!

I assume there is no ETA on it quite yet?

frank turret

no, it's probably gonna be included with the v13 release since there aren't any more planned v12 releases

dawn lodge

Will v13 have breaking changes like v12 had?

frank turret

Major versions generally do have breaking changes

dawn lodge

Well dang..

raven juniper

Not nearly as many as v12, though

long mason

Why does <Guild>#equals() even exist

Along with <User>#equals()

real jetty

Another like basically useless method is <MessageEmbed>#addFields() which I doubt anyone uses, which just adds more like clutter to the library

nah i think addfields should stay. people do use it and it makes code easier to read (ie you don’t have to have .addField 8 times in a row in your code)

old kelp
real jetty

oops i meant normalizefields @real jetty i copypasted the wrong thing

i meant to write <MessageEmbed>.normalizeFields(), idk why i wrote the wrong thing haha

sry

snow crypt

addFields imo is just... meh ```js
// addFields
embed.addFields(
{
name: "Global",
value: ${stats.globalWins} wins; ${stats.globalLosses} losses;,
inline: true,
},
{
name: "Per-country",
value: stats.countries.map((s) => ${s.name}: ${s.wins} wins; ${s.losses} losses;).join("\n"),
inline: true,
}
);
// addField
embed
.addField("Global", ${stats.globalWins} wins; ${stats.globalLosses} losses;, true)
.addField(
"Per-country",
stats.countries.map((s) => ${s.name}: ${s.wins} wins; ${s.losses} losses;).join("\n"),
true
);

wild flax

Yeah for composition I prefer the former

copper laurel
real jetty

ohhh kk gotchu , ty!!

lofty beacon

If they are statically used internally, in es2021 are they going to be private methods?

unique axle
snow crypt `addFields` imo is just... meh ```js // addFields embed.addFields( { name:...

The initial idea was to only have #addFields since it's much more verbose, doesn't bear the danger of providing arguments in the wrong order (which has often occurred in support) and takes a variable amount of arguments (field objects) which allows workflows like spreading or providing arrays of data objects.
However due to "feedback" from the community (read as: help channels being full of people calling the person applying the change (me) idiot, the library staff dumb and being very verbose about how much they hate the change) the method was re-introduced shortly after removal, after some proficient+ also expressed doubts in that approach. I might re-visit that thought in -next though, since a full on rewrite allows for structural changes like these to be perceived much less emotionally than in an already existing API.

snow crypt

Good point about the spreading, didn't think about that

Yet I still believe addField is better for stuff like my example

unique axle

i don't but that's besides the point and nothing to be discussed here

unique axle
tacit crypt

Kinda hard to male them hard privates and then use them externally actually..

lofty beacon

What? NormalizeFields is being used inside the MessageEmbed class, right?

If it's being used else where, then it can't be private

unique axle

do we use that anywhere else potatodetective
no we don't

old kelp

do we use TextBasedChannel as a typedef?

tired valve
tacit crypt

If it gets set in .ws then it's fine..if it isn't then it's a problem

tired valve

it doesn't hence when you run the pr it throws 4013 invalid intents

vernal atlas

resolved

junior pumice

who are the libary devs

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

KoishiThonk

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

shibaLol

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