#archive-library-discussion

25217 messages · Page 10 of 26

neat bolt
#

same thing theCutest

tacit crypt
#

Catch me up please, I'm all over the place

neat bolt
#

see screenshot for why we do any for extends checking

tacit crypt
#

Ok, for those, but for the rest?

neat bolt
#

crawl wants any for listener callbacks so that people can just assume the types

tacit crypt
#

Huh... ok

neat bolt
#

the setInterval stuff is the same

#

not that anyone uses that anyways

tacit crypt
#

Fair fair

neat bolt
#

pr probably needs a better name tbh

outer raven
#

what should it be i didnt know what to call it tbh

neat bolt
#

revert incorrect unknown type changes or something like that

outer raven
#

done

zenith oracle
#

Mmh it makes sense since we know if a structure is a partial with the partial property and we know that only id is in that structure if it's a partial... I think it would be really useful

tired valve
#

The thread update event exists and is documented

outer raven
wild flax
#

eh it doesnt matter really

outer raven
#

aight ill just commit the other change then

neat bolt
#

@remote wasp could you elaborate on that

remote wasp
#

sure

neat bolt
#

i think you're looking for 41 btw

loud jayBOT
remote wasp
#

oh yeah, that seems to be what I was looking for. Collection<Snowflake, TextChannel | ThreadChannel>. The ThreadChannel has isThread, so <col>.filter(e => e.isThread()) should give Collection<Snowflake, ThreadChannel>

neat bolt
#

yea its been fixed for a bit

remote wasp
#

👍

copper laurel
#

djs might not be using that version though?

neat bolt
#

maybe not

copper laurel
#

Not sure if/when we updated djs deps

neat bolt
#

you're in support a lot mon

#

do people 1. extend Collection, 2. rely on toJSON

ornate topaz
#

there wasn't even a collection release with that

#

might be worth to do a release 👀

#

though not sure if removal of array methods wouldn't clash with d.js

#

since that would also be in new release

copper laurel
ornate topaz
#

extend Collection? comp, most people just blindly do .array() no matter what they do or want

copper laurel
#

I think you're forgetting the best class, DataStore

ornate topaz
#

that was extension in the lib, not by people though

neat bolt
#

there still are extensions in the lib

#

mainly the toJSON collection and the limitedcollection

copper laurel
#

Yeah djs Collection is already an extension

#

So does this mean you break LimitedCollection#toJSON() if we dont add typings for that?

neat bolt
#

no

remote wasp
#

just wanna say that some libs that use @discordjs/Collection might/do extend it 👀

neat bolt
#

CollectionDJS#toJSON exists

#

but if you did colldjs.filter() then it wont

copper laurel
#

right okay

neat bolt
#

you need to do one of the alternatives

copper laurel
#

So if you did like, messages.cache.filter(one author).toJSON()

neat bolt
#

itd coincidentally work in JS, but wont typecheck in TS

copper laurel
#

it wont work

#

yeah

raven cloak
#

Hi guys, I'd love to help with development.
Where could I start? Learn TS 😅 ? Is there somewhere I can see a list of things that are currently being done/need doing just to have a browse

copper laurel
#

There isnt anything specific, pretty much everything for v13 is already done

#

The only place things that need doing are logged is really the github issues

remote wasp
slate nacelle
remote wasp
#

righhhht, that's why I didn't find it, cuz I was looking it in that _add PR. Now why is it still omitted tho? 🤔

slate nacelle
#

Oversight

remote wasp
#

I'll leave a comment 👍

zenith oracle
#

Mmh I was trying to apply some changes for structures when they're partials like isPartial(): this is PartialUser but the User is reduced to never for some unknown reason and TS says that there are conflicting types in some constituents...

#

Also are the PartialX types actually correct for partial structures or are they only for parameters passed in events?

wild flax
#

"is this actually correct" is an interesting question, considering we wouldn't know if nothings wrong

zenith oracle
#

Lol

outer raven
#

@solemn oyster why Opcodes and not OpCodes?

solemn oyster
#

It's a single word

outer raven
#

doesn't it stand for something?

oak quail
solemn oyster
#

I have no idea where we discussed opcodes before, but we spend a full day in internal channels deciding which one to use

outer raven
#

lmao

oak quail
#

its short for "operation code" but "Opcode"/"opcode" is the standard

#

nobody says OpCode

outer raven
#

ah I see

vivid field
#

We did that for the dapi-types enums before

solemn oyster
#

Oh, dapi-types, yeah

remote wasp
#

I still don't get why enums in dapi-types are PascalCase when the api has it in SCREAMING_SNAKE_CASE 🤔

wild flax
#

because enums are pascal cased

remote wasp
#

I meant this:

export const enum GuildVerificationLevel {
  None,
  Low,
  Medium,
  ...
}

The api has it like: NONE, LOW, ...

wild flax
#

what api

remote wasp
#

the docs

wild flax
#

yeah those are values

#

what you showed is enum keys

#

either way it doesnt matter much

#

we do not base our code style off of discord lol

#

You don't see use using snake_case do you

#

instead of camelcase

remote wasp
#

no... 😔

outer raven
#

only internally tho i think

#

oh and my PR oruded

wild flax
#

yeah all of that needs to be removed

outer raven
#

ohwait

wild flax
#

we should only use enums internally

#

no strings

outer raven
#

but like these are fine right

wild flax
#

Users using it is fine

#

But us internally not so much

outer raven
#

idk whats used internally, but that is what the users see at least

tacit crypt
remote wasp
#

yeah, I get it now. I got confused because of the enums we create in djs for them

solemn oyster
#

If you insist, we can make all enums in d.js PascalCased

zenith oracle
#

Why is this always false?

#

Same for other possible partials structures

#

Mmh I see that in events a partial dm channel can be passed too and ig that's why

outer raven
#

How do you know that’s a DMChannel type for sure?

zenith oracle
#

wdym?

outer raven
#

like if it's a DMChannel and not a PartialDMChannel then obviously partial will be false

#

my question is how do you know for sure that's a DMChannel type and not a PartialDMChannel

zenith oracle
#

That's what i've asked lol

#

Iiuc it's always a non-partial dm channel and it can be a partial one only in some events properties

ornate topaz
#

afair, on v8 discord doesn't emit channelCreate for DM messages. that's why you need CHANNEL partial to be able to receive DMs at all

zenith oracle
#

Ye

quiet viper
#

I was thinking to add a Client Option, which fires the event, even when the Structure is uncached?
Should I do a pull request or are they any concerns?

solemn oyster
#

@zenith oracle can you test all partial and non-partial structures, please?

#

Also put them at the end, they're grouped in chunks testing 1 thing

solemn oyster
#

Simply make it so the relevant stores use a custom one that emits events in your client

neat bolt
#

oh god pls dont do it like that

quiet viper
solemn oyster
#

declare const messageReaction: MessageReaction; and declare const partialMessageReaction: PartialMessageReaction;

#

Among others

#

Then you use the utilities to assert the types

#

Wait

#

I mixed the two of you, sorry

#

@quiet viper ^

quiet viper
solemn oyster
#

np

neat bolt
#

kyra wait NotLikeAlice

solemn oyster
#

Just curious, @quiet viper, what do you want that for?

neat bolt
#

thats to change caching not to change when events are emitted beeproll

solemn oyster
#

Oh

quiet viper
neat bolt
#

obviously beeproll

quiet viper
#

and the Dm Create Problem (Discord gives you the rawddata)

#

For advanced Devs, there should be a option for getting the Data, which Discord gives....

#

Idk if you understand what I mean

solemn oyster
#

I thought you meant emitting an event when a structure is uncached

#

aka removed from the collection

quiet viper
wild flax
#

just use the raw event then lol

#

since you cant use any of our structures anyway

#

idgi

zenith oracle
zenith oracle
#

Mmh when is Member#joinedAt null? Is it only when is partial?

solemn oyster
#

Presences don't give joinedAt, but they give all others (even roles)

wild flax
#

Also lurkers

zenith oracle
#

👍

real jetty
#

@wild flax srry for mention,
i found a typing error

wild flax
#

fix it

loud jayBOT
zenith oracle
#

@real jetty ^^

wild flax
#

You have to fix the tests in the index.ts too

#

silly you

zenith oracle
#

Done lol

#

Totally forgot about tests

#

Sorry

real jetty
#

@zenith oracle thx for submit a pull for this, but don't pull requests without at least checking that everything is OK

zenith oracle
#

So sorry about it. I always run the npm test command before making a pr but this time I didn't. I updated tests and everything should be ok now. However lmk if something should be changed via comment or review 👍

copper laurel
steady salmon
#

what else is needed to do for v13 release? looking to help out

copper laurel
#

Discord to release features

#

Otherwise feel free to check the Github Issues and fix anything you think you can

steady salmon
copper laurel
#

Mainly threads and stickers actually, banners and avatars are not important features we would delay for

steady salmon
#

makes sense, i was just looking at the pull requests

oak quail
#

why not make each typings "region" a different file?

#

its kinda messy rn, as one huge file

wild flax
#

can't

sacred river
#

add timeout back to message.delete() function ?

#

that would be nice

copper laurel
#

[3:35 PM] KinectTheDots: It didn’t allow for proper error handling

sacred river
#

welp

#

ggs

raven cloak
#

Takes little bit extra code and allows for both a timeout and proper error handling

quiet viper
loud jayBOT
stray delta
#

You can also probably listen to websocket events Thonk

quiet viper
#

why the Event does not get fired...

wild flax
#

@oak quail PR technically done?

oak quail
#

yeah

#

except for the docs link thing sugden? asked for

wild flax
#

yeah

#

Ill wait anyway until discord merges the PR

#

in case they have a fever dream over their break

#

and want to break more

oak quail
#

yeah I'll try to get it merged next week

bitter peak
#

btw does this need to be allowed for my PR?

wild flax
#

Yes, first time contribs need to be approved for a workflow run first

bitter peak
#

Hey @copper laurel, in regards to the conditional typing pr, what do you think about making Client#token readonly? That would prevent arbitrary API calls without logging in first, in a way that is rather unintrusive.

copper laurel
#

I'm not really in favour of restricting the way the library can be used

#

But Im also not responsible for the direction of the lib - my main concern is simply that the types weren't correct. Not all of the time.
I really do like Client#isReady() and TypeScript users should be expecting to typeguard their stuff. I have no sympathy for TS users complaining about having to typeguard - don't use TS then.

bitter peak
#

Alright. It seems to me that the refactor of Base is outside of the scope of the PR then. I'll revert the commits that changed Base.

copper laurel
#

Dont do it on my say so

bitter peak
#

Nah, it seems to be a much bigger issue than I initially assumed it to be

copper laurel
#

Hope i didn't come across too negative, I like the concept you're going for

#

Im just not sure asserting it as true is the right approach, its not truly conditional if there isnt a condition being evaluated

bitter peak
zenith oracle
#

Since Events became a const enum we can't use index access with them.... I mean, doing Constants.Events[eventName], where eventName is a function parameter typed as keyof Events, will now throw the error A const enum member can only be accessed using a string literal. Is this somehow intentional? Why?

#

I use an event handler and obviously this break a lot of things for me...

wild flax
#

Why do you need to access it like that

#

Oh, I see

#

Hmmm

#

That might have been an oversight if anything

#

We didn’t intend to have such a breaking change here

zenith oracle
#

Mmh yeah

oak quail
#

hmm should the library reject this

copper laurel
#

Why, the API doesnt

tacit crypt
#

Course it doesn't, because its a valid route. Now if we should handle it...idk.

#

Definitiely unexpected

solemn oyster
#

That's user error if anything, so no

#

Just like passing a non-snowflake to members.fetch can result on a WS restart and a timeout, sanitise your input and you're gold

bitter peak
#

Typings already use ${bigint} to guard against that right?

solemn oyster
#

Mhmm

shrewd kelp
#

When you provide invalid data to the api and get a DiscordAPIError the stack trace is lost, which can make debugging difficult. Could the APIRequest class capture the stack trace when it is created then emit it on error?

I tried to do this myself but I ended up getting a stack trace to TextBasedChannel.js (for a channel.send), the goal would be to get a stack trace back to the users code where they did the actual channel.send call

copper laurel
#

It should work fine on Node v15 or 16, forget which, with async stack traces

shrewd kelp
#

Im on 15.6.0, didn't realize v16 added that ill have to check it out

copper laurel
#

Hmm they should already be in place

#

Yeah it was actually 14

shrewd kelp
#

I don't think its just because its async, its because it gets pushed into a que and then the event is fired later

copper laurel
#

Right, gotcha

shrewd kelp
#

Im currently trying to figure out how to get a stack trace from here, to where the error is created... but I am very lost 😄

wild flax
#

Or do you sometimes just return or smth

shrewd kelp
#

I normally await, and the line that is throwing the error is await'ed

#

Actually, the stack trace may be broken due to how the route builder works

#

Or... its just javascript being funny

#

Put a trace right here and yet the callstack is broken

#

Javascript must be high or something, if I write the code like this, then my stack trace is logged correctly

#

However like this it fails

#

or... maybe not?

#

I have no idea, ill take another look at it tomarrow

warped crater
# solemn oyster Just like passing a non-snowflake to members.fetch can result on a WS restart an...

bit late but I'm not sure if I agree with this - input should indeed be sanitized on the user's end but the return value of such a function call does not match documentation nor typings at all - it is practically UB - which shouldn't happen.

I don't think the library should handle the snowflake passed in and try to validate it, but it should at the very least validate that the data returned from the API is valid to construct a User instance from

zenith oracle
#

I agree with this ^^
I think it can just check if id is typeof string, and, if isn't, throw an error

#

Can prevent unexpected behaviour like that where the returned structure doesn't match any documented one

warped crater
#

uh, no, that's not what this is about.

warped crater
#

i.e. somestring causes the library to do /users/somestring which is an actual 404, the library does throw since it could not find the user

#

however some/string makes it hit an entirely different route than intended (/users/some/string)

zenith oracle
#

So what should the library do to prevent this?

warped crater
#

if the route happens to exist (like @me/guilds) then U.B happens

zenith oracle
#

Ohh I see what you mean

warped crater
#

there's 2 approaches:

  • make sure the string cannot include / (or other HTTP unfriendly characters) <- not a fan of this
  • check the API response so one of those weird User instances cannot be constructed - and instead an error is thrown
#

the 2nd approach makes much more sense as it doesn't do input validation for the user

#

it simply prevents completely unpredictable behavior

#

a User instance should simply never have an undefined id, that's just horrible on all fronts

zenith oracle
#

Mmh do you mean this should be checked by the User constructor?

warped crater
#

sure, it could be done in each structures' constructors, I suppose

#

it could also be done before one is called, it depends how wide spread such checks should become (if at all)

zenith oracle
#

That's really interesting, but how should djs check if the data provided in the constructor is ok?

warped crater
#

it doesnt

#

lol

zenith oracle
#

I meant, how it should 😂

warped crater
#

just how you typically do runtime checks in js. ¯\_(ツ)_/¯

#

typeof - in keyword etc

#

similar to how client options and everything else is validated

zenith oracle
#

Well that's what djs does (using if "something" in data), but it also handle partial Structures so it cannot throw an error if, for example, a property is missing...

warped crater
#

Those checks are there for different reasons and are there for mutable properties and happen in _patch typically.

#

its like

#

for users, yeah I get it

#

a partial user doesnt have much other than its ID as raw data

#

but this is mostly about data coming from GET api calls which is typically complete

#

hence why I was hasitant to say "do it in the constructor"

#

its a very tricky problem to approach, yes

wild flax
#

Lol

#

I don’t think we need to do anything

#

We document very clearly what you can do

#

If you go strictly against that, dont expect anything fancy to happen

warped crater
#

I dunno, I don't think an undefined id on a "valid" User instance is desireable

wild flax
#

Introducing random runtime checks on every construct now seems ridiculous

#

The error is already not passing an id

#

Not the construct you get back

#

Feel free to PR a good solution to this, because I can’t think of one that wouldn’t either impact perf or convolute the code

warped crater
#

👍

real jetty
#

I just saw that interaction includes an "inGuild" function. I think that this could be great if we can add a similar function on Message and maybe on Channel

sharp wigeon
#

what would the inGuild method do?

real jetty
#

i pressume itd check whether the message was sent in a guild or a channel belongs to a guild

sharp wigeon
#

Don't think that would be necessary as the GuildChannel class does exist

#

but I may be wrong

unique axle
#

should be able to guard that with a return on !message.guild

sharp wigeon
#

Yuh

zenith oracle
#

Well it's the same as !interaction.guild_id. Not sure why this was added effectively Thonk

wild flax
#

Does that actually type guard

zenith oracle
#

The inGuild? No, I don't think so

wild flax
#

I mean the others

zenith oracle
#

Others what?

tired valve
#

Jeez, the other structures, message.guild, channel.guild

real jetty
zenith oracle
zenith oracle
real jetty
#

I thought it worked similar to "isText()" to indicate that "guild" is not null and that "channel" is of type GuildChannel

ornate topaz
#

the question wasn't "what should they guard from" but "do they guard"

zenith oracle
ornate topaz
#

does doing !whatever.guild have the same effect as using !inGuild()

zenith oracle
#

No, since the first at least ensure that the .guild exists, while the second just returns a Boolean

wild flax
#

Do you actually look at the typings

#

Hence why it’s called a type guard

#

Returning a bool is how type guards work

zenith oracle
#

Well what I mean is that returning boolean is different from returning this is TextChannel

tired valve
zenith oracle
#

So this returns to my question 😄
The inGuild, what should typeguard?

ornate topaz
#

that the guild exists...

#

and it's notis but in

#

probably also why it cannot be this is whatever

tired valve
zenith oracle
# ornate topaz that the guild exists...

Okok I think I wasn't really clear but I think I'll look into it better tomorrow 😂
However what I meant is that if I use if (interaction.inGuild()), for example, am I sure that .guild or .guild_id (or was it guildId, I don't remember) are defined? That's what I mean for typeguard lol, maybe I did not say it clear enough, sry

ornate topaz
#

did you look at what it actually checks?

#

There's no GuildInteraction you could typeguard with isGuild(): this is GuildInteraction guard that would check if there are guild and member properties.
There's just Interaction, so you have nothing to is against.

#

this typeguard doesn't act as a type narrower

#

but more of a check

zenith oracle
real jetty
#

I think I understood what he meant, he expected to have a this is ...... ||too late XD||

Nvm we come back to my question, why not add this principle to the Message object? Both functions would have the same principle and I think it would homogenize the operation by having this same function in the Message class.

zenith oracle
#

And that's the main question

real jetty
#

Honestly, I thought, like D Trombett, that with this function there was a new class like GuildInteraction that was created and that overloaded some properties like guild by indicating that it was not nullable anymore

zenith oracle
#

If there is a typeguard (which doesn't change the type with an is) that just check for the existence of guild_id, is it there for a specific reason? (And it's not in other structures like message or channel for another?)

ornate topaz
#

why would there be a separate class that has exactly same properties

zenith oracle
vivid field
#

Tbh if there's no GuildInteraction interface that is used for the inGuild() function, I don't see why it was added in the first place

#

The sole reason why I added isText() was to have a type guard

real jetty
real jetty
zenith oracle
#

Yeah!!! That's what I said 😂
Why is it there without a type check... But actually not in any other place (in that case we can say it's for consistency...).

Actually replying to Vaporox but forgot to add the reply woops

wild flax
#

@copper laurel maybe knows 🤔

copper laurel
#

I forget who added it but yeah, it just typeguards a couple properties to rule out it being in a DM. I think I proposed "inGuild" rather than "isGuild"

ornate topaz
#

is it actually supposed to work in this context?

vivid field
#

I would expect it to, but since it's not a type guard it doesn't

real jetty
#

I thought it worked like a type guard to remove the nullable status but it doesn't. So I don't see the point of this function nor the point of adding it as I proposed to the Message and Channel class...

So I have a new question, why not make this function a real typeguard to update interaction typing?

ornate topaz
#

as it was already said, it would require to have a separate interface just to have any way to narrow the type. except such GuildInteraction would be identical in properties to Interaction merely changing one or two property types. Having it would also mean that you would have to have every other interaction class extending Interaction in version that extends GuildInteraction

#

it works like that on channel types because TextChannel extends GuildChannel, adding things related to text channels that are not found on other types of GuildChannels. Similarly, GuildChannel extends Channel adding things that are not on the base Channel. Or even ButtonInteraction adding button related stuff not valid to be placed on the base Interaction class.

rough glacier
#

It's just a fancier way to identify guild interactions than checking guildId. Nothing more really..

#

If it seems like a typeguard because of its name, perhaps we could rename it to fromGuild or something

copper laurel
#

If it doesn't typeguard the guild properties it's useless

ornate topaz
#

Is there a reason why Options.cacheWithLimits({}) takes Record<string, number> instead of being keyed with manager names?

#

Right now i can search up for the PR that adds it since it shows an example, or open v13 changes showing exactly same examples, but to my knowledge there isn't a list of managers to pick from

#

maybe apart from opening the source code since managers are in their own directory

zenith oracle
#

They're just the names of all class that extends another class called like CachedManager (totally unsure about this name). They're really a lot but yeah, a type that includes all of these name would be great

ornate topaz
#

yeah, but if i am a new user, i have to click (or at least scroll) through every single class on the sidebar to see all managers. "and extend? what is that supposed to mean, how is extending relevant"

zenith oracle
copper laurel
#

I don't see why we couldn't make a typedef/constants like ClientEvents

ornate topaz
# zenith oracle <:Thonk:654873234809946132>

I personally, know where to look for managers, or how to tell which managers can be manipulated with the option.
But i was talking from perspective of a new user somewhere down the line. They might not have any idea what they can put there at all, and when they look at the guide (assuming it will land somewhere else than just v12 -> v13 page which might not even apply to them if they start with v13 directly) they might see only those 2 managers shown on the examples. then they can either assume only those 2 can have options, or that they can pass more stuff.
But what other stuff. Every manager? then they need to scroll through every single class on the docs sidebar, since there is no single list of all managers, like for a resolvable or flag list. And while doing so, they will be completely clueless that not every manager extends CachedManager or even have 0 clue what that means

#

having a typedef for it not only makes it appear in the docs on a single page, but also can be made to appear in your IDE as you're typing the option

real jetty
#

I'm sorry I'm pushing a little bit for the inGuild function but I'm convinced that we can improve the typing by making a real typeGuard
I searched for several hours to find a way to make a real typeGuard that can be set up very easily. Be careful it's typescript but I think that transcribing it into javascript should be no problem:

type PropertiesNotNull<T, TProp> = { [P in keyof T]: P extends TProp ? NonNullable<T[P]> : T[P] };
function propertiesAreDefined<T, TKey extends keyof T>(object: T, ...properties: TKey[]): object is PropertiesNotNull<T, TKey> {
    return properties.every( property => object[property] != null);
}
function wasSentInGuild(interaction: Interaction): interaction is PropertiesNotNull<Interaction, 'guild' | 'guildId'> {
    return propertiesAreDefined(interaction, 'guild', 'guildId');
}```
#

||Do not pay attention to the names of the functions they were written in the night XD||

real jetty
strange igloo
#

Shouldn't this just assert whether or not Interaction#guildId is not null?
Let's say we do not have the GUILDS intent for some reasons, Interaction#guild will be null no matter if it came from a guild or not.

ornate topaz
#

that is what the check does right now, along also checking for member

real jetty
#

I shouldn't work so late at night because beyond that interactions can be sent even if the bot is not in the guild...
Maybe this system can be improved but actually right now it would lead to an error...

wild flax
#

No runtime checks apart from inGuild should be needed which is already in place

#

all that is missing are the proper types for it in the index.d.ts

vagrant holly
wild flax
#

There’s a pr for that

vagrant holly
#

Ah sweet

#

Hm, I'm getting a load of PermissionOverwrites#type being an integer on master babyyoda_sad_devcord

wild flax
#

Huh

vagrant holly
#

Should be a string, role or member, but getting a lot of errors thrown that its 0, which is what the API returns directly. Will dig through d.js source in a bit to find where its not being converted

wild flax
#

Uh I can’t link because of mobile

#

But maybe the permission manager PR is a hint

#

Since that changed a bit in that regard

#

Could double check the files it modified

#

Permission Overwrite Manager sorry

vagrant holly
#

nod that would be my suspect as well given that was in my version bump where it broke

zenith oracle
# ornate topaz *I* personally, know where to look for managers, or how to tell which managers c...

I think the same... It's a really good idea to add these cache options, but effectively not really everyone knows all structures (eligibles for this cache limit) or know where to find.

The other day I saw that a property in my clientOptipns (regarding the max count of messages in the cache) was removed and I found this new property called "makeCache", which accepts a function. Sincerely, I totally didn't find anything about what's this and the function it accepts what should do so I tried to check the docs, but they were really generic: Function to create a cache. (-1 or Infinity for unlimited - don't do this without message sweeping, otherwise memory usage will climb indefinitely), which includes a part from the old description (which is not really useful now) and nothing that refers to Options... Nothing neither in the reference of CacheFactory, that actually only confused me more... I found Options.cacheWithLimits for a pure case, because a user asked support about it in #djs-help-v14... So I tried to use it, but saw just a record of string and numbers, so I was totally confused. After checking the pr where it was added I found more informations about it and I went to check all of these managers.

Now, I don't think that everyone can do this, and types and docs are there for a reason, so the first thing I think can be done is to improve the description of this option (replacing the old text with a word about Options) and then adding to types all of the structures that can be used in the Options.cacheWithLimits. Probably, it's an hard work 😂, but is necessary for all people who have no idea about these options and their potential, so, I totally agree with you and I can help with docs or types for this but lemme know What y'all think about this...

I wrote too much, don't read everything because it's useless probably 😂

strange igloo
#

That's some niche feature anyway, I don't think begineers would use it (I hope so at least, scared about what I could see in #djs-help-v14 )
Also except for Message and Presence managers I'm not sure where is the practical use case of restricting guild manager cache (for example)

#

Even if some typedef about accepted inputs should definitely be a thing, of course

zenith oracle
#

I'm limiting the user and members managers iirc

#

To avoid having a lot of users in the cache

#

And Message obviously

vagrant holly
#

Was mentioning this in #archive-offtopic but thought I would surface here -- strings seem to be the worst offender for mem usage (a shock to no one), but it seems d.js currently prefers to store a lot of stuff as strings internally (most enums, etc.). I'm wondering what folks would think of switching things like this to store as numbers, and have getters to retain returning strings when accessed (or just moving to numbers completely and having the user resolve the enum)?

wild flax
#

it's just so much more annoying to use for people ugh

#

Like I fully agree and we should do it

vagrant holly
#

If you were to add getters the end user wouldn't see much impact, but maintainer/contributor experience would suck a bit more

wild flax
#

but people are gonna give us an earful again for having to do ChannelTypes.GUILD_TEXT and why they cant do "GUILD_TEXT"

vagrant holly
#

Could just resolve that

#

But I'm also a fan of just moving folks to using constants like that anyway, hard-coded strings are bad

wild flax
#

Very hard with such a close-minded userbase that mostly learned JS from bad express tutorials

#

Sure you can just force it onto people, but still

vagrant holly
#

It could be for now that its done in a completely non-breaking way, so there are getters so accessing still returns a string, and resolvers for anywhere a user-provided value is ingested

#

And made more breaking with numbers-only later on

left moat
#

getters use memory too tho cryingmansad

wild flax
#

"Later on" will probably just be the rewrite anyway

#

but yeah, it will be hella annoying for people to do channel.type === ChannelTypes.GUILD_TEXT

#

most of our userbase is js beginners OMEGALUL

#

Honestly, you are right though

#

I should just not care

vagrant holly
#

tbf v13 is so incredibly breaking that a change like this would fit in

wild flax
#

But I do to a certain degree, I don't like death threats from 14yo

vagrant holly
#

i struggle to keep up with master when I update once a week LUL

wild flax
#

vlad wanted to do something like that

#

we still have approx a month or so until threads hit general availability

#

which is when v13 is supposed to hit anyway

vagrant holly
#

I'd like to think this'd have a pretty significant impact on d.js mem usage so would be worth it

#

But I don't actually have anything concrete to confirm that

wild flax
#

Its pretty clear that 700k "GUILD_TEXT" or "GUILD_VOICE" uses a lot more than 1 or 2

#

But not sure if it makes a huge difference

#

id rather be interested in extending cacheable props

#

so you can disable certain props on structs and theyd just return null or smth

vagrant holly
#

'GUILD_TEXT'.length * 2 * 700_000 = 14_000_000, channel type is currently 14mb

#

A number in JS is 8 bytes iirc, so 8 * 700_000 = 5_600_000 5.6mb

#

Seems like a decent saving on a single prop, so across all enums would be p good imo

wild flax
#

but what if you could just do

# 
makeCache() {
  return {
    ChannelCache: {
       // amount: 200 -- Defaults to Infinity
       topic: false,
    }
  }
}
vagrant holly
#

Yeah that'd be nice

#

You can just update the prototypes for channel to add a null'ed getter & setter for topic though, so thats less of an issue

wild flax
#

Not as userfriendly honestly

#

but I guess yeah

vagrant holly
#

nod Yeah, for a ux perspective, being able to just disable the prop would be nice

#

Doesn't fix things like type though, cause a lot of folks need those, but they're still strings Sadge

#

Gonna test this on Res for a bit and see if I notice any diff in mem usage:

    // Store channel type as an integer
    Reflect.defineProperty(Channel.prototype, 'type', {
        get: function () { return Constants.ChannelTypes[this._type]; },
        set: function (value) { this._type = Constants.ChannelTypes[value]; },
    });

    // Store overwrites type as an integer
    Reflect.defineProperty(PermissionOverwrites.prototype, 'type', {
        get: function () { return Constants.OverwriteTypes[this._type]; },
        set: function (value) { this._type = Constants.OverwriteTypes[value]; },
    });
wild flax
oak quail
#

@vagrant holly i brought that up recently, Kyra said changing all types to numbers didn't impact memory usage

vagrant holly
#

Dropped about 100mb for me

#

Maybe something about what Discord was sending me happened to change as I rolled out the above, but it would appear to me that it does help

solemn oyster
solemn oyster
#

It takes 10 bytes for text, 8 for length

#

Every reference to it is 8 bytes

#

Numbers inside objects are boxed, to prevent memory issues as well as preventing memory expansions in the middle of it, which means they're always doubles, and they always take 8 bytes

#

So the overhead of having this as a string, is a mere 18 bytes

vagrant holly
#

This is assuming that v8 decides to optimise the strings?

#

Aren't strings in most cases stored directly, not optimised in a map with pointers?

#

Oh, hm, the enum they come from is a constant object, so maybe v8 does optimise them, assuming type is always set from those objects

drifting knot
#

what about making snowflakes a bunch of opaque types

#

so that the type of a user ID is distinct from the type of a channel ID

remote wasp
#

on what basis, because they are the same thing

solemn oyster
#

@vagrant holly there's an internal hashmap for strings

#

Even if you don't use the constant, it'll be assigned to the same one, although there's also the one in the .data field from the compiled assembly, which ofc is a constant value

#

This is also the same behaviour in C# and several other languages, specially managed ones that have immutable strings

solemn oyster
drifting knot
#

?

solemn oyster
#

Simply, no

#

If this were C++ or Rust, sure, because RTTI is optional

#

But JS is RTTI heavy, and as such, the overhead of boxing primitives is enormous

drifting knot
#

uh

#

what are you talking about

#

opaque types

#

build time

#

typescript

solemn oyster
#

RTTI = RunTime Type Information

#

aka information that tells what each thing is instance of

#

Each value in JS has a header which tells the type followed by the data of the values

#

Primitives are specially optimised, because they can skip some headers, but they don't work when boxed due to alignment issues that can arise from mutating them with other types

drifting knot
#

are you reading my messages at all

solemn oyster
#

Yes

drifting knot
solemn oyster
#

And it makes no sense

drifting knot
#

okay 👍 👍

solemn oyster
#

So I'm trying to make sense of it

#

You can't just make 2 types resolving to the same one and expect them to be opaque

#

Simply because type is called type alias in TS

#

The only way to make them properly opaque is by adding type-only fields that don't exist, I think those are called brand fields

#

But it can be troublesome, you see

#

In all servers, the everyone role id is the same as the guild's

drifting knot
#

BUILD TIME

#

ONLY TYPESCRIPT

#

I DO NOT CARE ABOUT JS USERS

solemn oyster
#

As well as in old ones, their created-by-default channel also has them

#

Dude are you even reading me?

#

What do you think "type-only fields" mean?

drifting knot
#

that's the only way to do opaque types

#

that is how everyone does it

#

it is a feature of the language

solemn oyster
#

Yes

#

And I'm telling you

drifting knot
# 
type Snowflake = `${bigint}`;

type Opaque<T, N extends string> = T & { __opaque: N };

type UserSnowflake = Opaque<Snowflake, "UserSnowflake">;
type ChannelSnowflake = Opaque<Snowflake, "ChannelSnowflake">;

const user = "123" as UserSnowflake;
const channel = "123" as ChannelSnowflake;

console.log(user === user);
console.log(channel === channel);
// @ts-expect-error
console.log(user === channel);

behold, 0 cost typesafety

solemn oyster
#

It's baseless

#

IDs are shared in many places

#

A message ID can be a thread's ID

#

A role ID can be a channel's ID as well as the guild's ID

#

You'd have to typecast it all the time and it's annoying

#

I already know what branded types are, I use them in my bot to define i18next types

drifting knot
#

tell me more about your Discordjs Bot

solemn oyster
#

Outside this channel, probably in DMs

rough arrow
#

I see that discord.js-modules is using tabs. Is there a reason for this switch? Can I read about discussion anywhere?

wild flax
#

Tabs are simply superior

#

If you want a "real" reason read this.

#

But either way, point still stands. Superior.

rough arrow
#

I see, I can't imagine this was the reason that discord.js switched though. Is it just preference among the core developers and this is something that they've wanted to do but can't because it would ruin the history?

copper laurel
#

I mean youre talking to a maintainer of discord.js who just gave you the reason

#

and you think its not the reason because...?

solemn oyster
#

In the topic of memory usage (that IPv4 has brought over), should we look into removing TextBasedChannel#_typings? Its removal has a pretty large memory impact

#

Alternatively, we can do a global WeakMap<ChannelId, Set<UserId>>, which has near to no memory effect for those not using typing intents, if we need it

#

Or a Map and remove it in CHANNEL_DELETE/GUILD_DELETE just like we do with the global channel cache

#

The structure would ideally self-sweep if an entry removal ends up to 0 user IDs, so we don't memory leak

wild flax
#

@outer raven no we don't know, thats exactly why

outer raven
#

I don't see any reason why they would do that though? The row doesn't even have any special properties it's just a place to put components in

solemn oyster
#

It's Discord, they can implement new features without notice, say, padded columns within a row with a different type

#

And we'd have to support 2D arrays as well as different row types, but the former always resolving to the first row type

outer raven
#

Well I assume the nested arrays would always only contain components and not those columns, and Action Rows would always be the default when a nested array is passed

solemn oyster
#

Rows having a type identifier suggests they're going to add new ones in the future

outer raven
#

They have a type identifier just like buttons and select menus, but, unlike channel types, there is no gap between the action row number type and the others

copper laurel
#

having a default to only one type of row is exactly the sort of misleading interface we wanted to remove

#

like the send overloads

outer raven
#

But why remove things that make people's lives easier? I have a system rn that I genuinely don't think I can use action rows for and the nested arrays were the perfect solution

copper laurel
#

For all of the reasons listed above

wild flax
#

Why are people like this

copper laurel
#

Because Discord might introduce new types of rows
Because we might have to support 2D arrays and cant if theyre being interpreted as rows
Because we don't want two different interfaces for different types of rows

wild flax
#

And yes, you can easily have your system work with non-nested arrays

#

Because you can literally just replicate what we do when you pass a nested array

copper laurel
#

a raw object is barely more code

outer raven
copper laurel
#

because we cant make breaking changes later

#

dont want to have to do a v14 just for one row type

outer raven
#

can't you just do a 13.x?

copper laurel
#

not to remove something

#

do you understand semver?

solemn oyster
#

Removing features = semver major = v13 -> v14

amber basalt
#

What is semver?

solemn oyster
outer raven
#

Is v14 gonna be the TS rewrite or is it still gonna be on js

solemn oyster
outer raven
#

Alright then

copper laurel
# 
- components: [[button]]
+ components: [{ type: 1, components: [button] }]`
outer raven
#

I have an array of arrays of buttons though, and I can't possibly predict how many buttons I have in total
Can you at least provide an alternative in the PR description?

vagrant holly
copper laurel
#

if you want me to copy and paste that, sure

outer raven
copper laurel
#

why dont you show what youre doing now, in buttons channel, and I'll give alternative

outer raven
#

sure

copper laurel
#

The alternative is literally just doing what the library does now internally with a nested array really, which is mapping it to components

remote wasp
#

What was the reason for not removing interaction event instead of deprecating it. We had no reason to keep support for something that wasn't even released yet 🤔

ornate topaz
#

pretty sure it wasn't just a niche no one knew about. we literally made channels for v13 support

remote wasp
#

Yes a lot of people would have been affected from it but interaction were added on master and were changed on master, so there is like no need to keep the bloat around when we already are near a major release and have already broken some really big parts of the lib

ornate topaz
#

and the "bloat" is singular boolean variable?

remote wasp
#

it's not how small or big it is, if we can break stuff that has been here for a long time then why keep support for something that had been just added to the lib.

copper laurel
#

just because of the scale we expect its been used at

#

and there isnt much harm in that existing

remote wasp
#

I understand where you're coming from but this all seems like a problem we are pushing over to the future. We have a chance to rename both the events as people will likely rewrite their bot for v13.

copper laurel
#

it isnt a problem though

#

it wasnt a problem for 12 versions and it isnt now

solemn oyster
#

True, @dev releases have around 5000 downloads a week

remote wasp
#

It will be a problem if we decide to remove the deprecated events somewhere in future. I'm just saying that we have a chance of making breaking changes then why not break this too. It's not like users can get anymore angry, given that we already broke a lot

copper laurel
#

we'd remove them in v14

#

thats what deprecations are

remote wasp
#

If that's what the majority decision is then okay 👍

wild flax
#

It will be a problem if we decide to remove the deprecated events somewhere in future.
I don't understand lol

#

How

tacit crypt
#

Right now you get a warning if you listen to the deprecated events, just like you get when you used a deprecated function in v11 that was removed or replaced in v12. In the future, they'll probably (I'd assume so) be removed, so you have a gap of time for people to update their code

remote wasp
#

I was curious about the decision so I asked, I'm okay with the change now because it seems like everyone supports it and it wasn't something that just made it's way into the master (for eg inGuild typeguard). I would like to make myself clear tho, I was thinking that we deprecate things when we introduce a replacement without a major release and then remove it on the next major release. I get it now.

For crawl's question, I meant it would be a problem if we remove it in the next version and someone who still uses the deprecated events will have to make change but that's their problem. Yeah that's it.

wild flax
#

Yeah but considering they get a warning in the console even

rough arrow
# copper laurel and you think its not the reason because...?

Don't get me wrong, using tabs to accommodate for the visually impaired is a pretty good reason for one to use tabs. But was that one of the arguments brought up when this was discussed and ultimately decided?

I really just want to know what lead up to this decision so that I can weigh the benefits myself and make better decisions for my own projects.

ornate topaz
#

the reason that was given doesn't really have any counter argument

#

or maybe you have some arguments against tabs

#

and by the way, yes. that reddit post was brought up few times before. it's not like it was random and crawl just now searched up something to give you.

rough arrow
rough arrow
#

Thank you!

outer raven
#

I think it would be quite beneficial to a lot of users if the messageReactionAdd, messageReactionRemove and typingStart events sent a GuildMember object along with the user object that is already sent so that they can more easily manipulate the member. Of course this would be nullable since the member may not be cached or the event may not be triggered in a guild. What do you guys think?

tired valve
#

If you want a member grab it from a guild

outer raven
#

The current event is already getting the user from a member most of the times so why not just send the member as well?

ornate topaz
#

"along user"? sounds pointless when you can do member.user

#

and then you realize that all those events can be received in DMs

outer raven
ornate topaz
#

does the library even emit any empty data for current state in any place?

#

it would seem weird that you sometimes do get member, sometimes you don't.
with current way you are going to get an error at message.guild if you are going to carelessly assume it was in a guild and get/resolve anyway

outer raven
ornate topaz
#

what

strange igloo
#

I think they meant to have that kind of signature for typingStart handler
(TextChannel, User, ?GuildMember)
Or
(TextChannel, (User | GuildMember))

ornate topaz
#

the latter is just going to be confusing, since you have a parameter that can have 2 completely different from each other objects

#

the former i think falls onto what i already asked

#

does the library at any place states that it can emit data that can be missing (and explicitly stating that it's not old state in *Update, which is cache dependant)

strange igloo
#

I don't really agree with that at all tbh
That job could be done by the end user with a single line

const member = channel.guild?.members.resolve(user.id) ?? null```
wild flax
#

resolve already defaults to null upsidemmlol

#

if the concern is cache though, you want to fetch anyway

strange igloo
#

If guild not here it would set it to undefined (?)

ornate topaz
#

regardless, !member

#

but in your own variable, obtained from objects that are always going to be given

oak quail
#

since the member may not be cached
well this isnt an issue for message reaction add or typing start

#

discord never sends a user object in those, it only sends user_id and the member if it's in a guild

#

djs not providing the member object is inconsistent with the gw, and the fact that it caches the member when those events are received is hidden behavior

copper laurel
#

This has been discussed before and decided that we didn't want either of these interfaces
client.on("messageReactionAdd", (reaction, user, member or null) =>
client.on("messageReactionAdd", (reaction, user or member) =>

#

discord.js caches the member when data is received, so reaction.message.guild.members.cache.get(user.id) should always work if sent in a guild, intents etc

boreal vapor
#

The UserFlags "EARLY_VERIFIED_BOT_DEVELOPER" reports to the Djs as "EARLY_VERIFIED_DEVELOPER"

I tested with two users with this flag (The second command isn't there because I used Edit), maybe this is better suited to the #archive-site-discussion, srry if is

oak quail
#

thats because youre on v12

#

its EARLY_VERIFIED_BOT_DEVELOPER in v13

boreal vapor
oak quail
#

well v12 is done, it wont be changed

real jetty
#

Why was sending a MessageEmbed by itself removed from .send in favor of using an array in messageoptions? Seems like a weird choice considering more likely then not you're only sending one embed at a time.

wild flax
real jetty
#

interesting, thanks for the read

rough arrow
#

I am not sure what to do with my PR
https://github.com/discordjs/discord.js/pull/6085

Looking into the source of MessageManager, the resolve method looks like this ```js
resolve(idOrInstance) {
if (idOrInstance instanceof this.holds) return idOrInstance;
if (typeof idOrInstance === 'string') return this.cache.get(idOrInstance) ?? null;
return null;
}

There's no way to detect an API message, because it is simply an object (not an ES6 class). The only way to know is to see if it has an `id` property and then try to use that, but that has side-effects of accepting anything with an id.

So I guess I'll just add move it to this union (instead of inside MessageResolvable)? ```js
deleteMessage(message: MessageResolvable| APIMessage | '@original'): Promise<void>;
remote wasp
#

You already know the answer, do what you suggested in your last comment on the PR i.e. change the typings for deleteMessages and add APIMessage to it.

Changing MessageResolvable will create ripple effect for just a niche change.

#

The Webhook#send returns APIMessage along with MessageResolvable, you let deleteMessage take APIMessage along with the same

bitter peak
#

Used like so: (/command single <name?: string>)

fallow crater
#

maybe it could be implemented in @discordjs/builders?

bitter peak
#

@tacit crypt what do you think about this ^?

tacit crypt
#

Dunno... builders are more... take user-friendly input and emit raw API data. But I'm not opposed to utilities like that. Thoughts @wild flax @solemn oyster?

wild flax
#

hmm

#

I don't mind transformers

#

the only requirements would be:

  • Takes interaction.options and returns a completely new collection (no mutation)
  • Using ow for validation, since we already do that in builders
#

The first point might be irrelevant anyway, since all it does, as I see it, is parsing it

bitter peak
#

Yeah, the way I have it currently, it just provides methods for parsing the options into native data, and subcommands into another resolver class

#

Is that or a transformer of some kind preferable?

wild flax
#

Nah, I mean thatd be fine

#

I just wouldnt know what to call it or where to put it

bitter peak
#

oops sorry for ping person

#

discord.js@dev*

wild flax
#

djs types are required?

bitter peak
#

CommandInteractionOption

#

and User etc.

wild flax
#

hmm

bitter peak
#

Maybe this would belong in the main lib instead?

wild flax
#

probably so yeah

bitter peak
#

Should it replace CommandInteraction#options?

#

Where the current raw collection would be something like rawOptions

wild flax
#

Neither are raw

bitter peak
#

Hmm, maybe optionsCollection or something then?

#

Or make it a property of the resolver

wild flax
#

I think it should be opt-in if anything

bitter peak
#

alright, should it be like, manually constructed or should it be a property?

wild flax
#

I don't necessarily wanna decide this alone so uh

#

cc @copper laurel @vivid field

copper laurel
#

Does Discord always resolve the data that reliably to be throwing if its not there?

#

This wrapper is essentially just allowing us to get a properly typed value from the options collections right?

#

So intead of .get("name").value: string | number | boolean etc, we'd use .channel("name"): GuildChannel | null

bitter peak
#

correct

#

Also has support for subcommands and subcommand groups

copper laurel
#

So, for the most part I think I like this, but I also don't really like the fact that it's a Collection at all currently, I'm not sure what benefit changing it to that in the first place provided other than .get("name") being ever so slightly easier than .find(o => o.name === "name")

bitter peak
#

It also doesn't really make sense to me to be a collection

copper laurel
#

So for me personally, I think this is a far better implementation than the options collection, except for the fact that it doesnt provide access to things like the raw values that I can see (unless you went via resolver.options.get?)

wild flax
#

yeah I wouldn't like being forced the validation on me

#

I have a transformer for the current collection that just .get() values, I don't care if they are nulled

copper laurel
#

My other concern is the method names, while this does seem like the simplest naming convention I'm pretty hesitant to have methods that are name the same thing as properties all over the rest of the lib, like .user(), .channel() etc

#

We got rid of guild.member() and people used to use that incorrectly all the time

#

Definitely support the concept though

bitter peak
#

My idea would be to have
CommandInteraction#options: CommandOptionResolver
CommandOptionResolver#data: Collection<string, CommandInteractionOption> -- (or CommandInteractionOption[])

So for most users, they could use interaction.options.channel("target").
And for users that need the raw collection/array, use interaction.options.data.

Also, how about get*() instead? getUser(), getSubCommand(), getString(), etc.?

copper laurel
#

probably yeah

wild flax
#

I'd still like to keep a generic get()

copper laurel
#

Its a little wordier but it makes them clear that theyre methods and not props

wild flax
#

that does no validation

copper laurel
#

Wouldnt that be interaction.options.data.get

bitter peak
#

^

copper laurel
#

or do you want to promote it up

wild flax
#

I wouldn't want to expose raw data at all

#

we don't do that anywhere in the lib rn

copper laurel
#

Well that "raw" data is still the parsed collection

bitter peak
#

So have CommandOptionResolver#get(name: string): CommandInteractionOption | null?

copper laurel
#

actually

#

Doesnt this already do that

wild flax
#

yah

bitter peak
#

oh yeah

copper laurel
#

cool

bitter peak
#

forgot I added that lmao

wild flax
#

id like to keep that

copper laurel
#

as long as required defaults to false

vivid field
#

I really like the concept too, but I don't really get the purpose of validating the data again. Shouldn't you as the bot owner know which option has which type?

wild flax
#

yeah but TS

#

people would (like me) have to write their own type parser or smth

#

or constantly cast

#

which is really a sub-par experience lol

vivid field
#

Sure we can keep the methods, but e.g. the required parameter seems unnecessary to me (or maybe as a type parameter)

copper laurel
#

People like me would have to steal Crawl's type parser

bitter peak
#

A lot of command options are required, so having to check in code that it isn't null gets annoying fast

#

It's a lot easier to just add a , true) to when you get the option

#

imo at least

#

It can also help devs catch bugs where their command code doesn't match their command option data

vivid field
#

Yea agreed, but I think a type param would be better suited here

bitter peak
#

Instead of just getting null, they would get an error

#

hmm

#

I've run into the issue a few times where I update my command's ApplicationCommandData, but forget to change something in my actual execution code. The errors are usually really annoying to track down because I would just get back null when I tried to access an invalid option. With the required parameter, it would help catch those errors better, in addition to the inferred types without adding another generic.

#

I'd argue that trying to get an invalid option should be an error, and it's better for the library to handle that error than for the user to. There's no use case at all for fetching an invalid option.

vivid field
#

But if you get cannot read property "user" of null or whatever, shouldn't you know what's going on 🤔

#

Well anyway, I'd say you can implement it and then it can still be changed afterwards

bitter peak
#

(just added the property parameter to _getTypedOption, it would be added to all the calls as well as "value", "channel", "options" etc.)

wild flax
#

maybe im too tired but

  get(name, required = false) {
    const option = this.options.find(opt => opt.name === name);
    if (option) {
      return option.value;
    } else if (required) {
      throw new Error(`Missing required option "${name}"`);
    } else {
      return null;
    }
  }

get(null, true) will throw?

bitter peak
#

yes

wild flax
#

ah right, makes sense

#

I was reading that in a very dumb way OMEGALUL

#

yeah the private method is ok

#

reduces code duplication

vivid field
#

Just a few minor nits but the concept LGTM

#

Actually couldn't this extend BaseManager instead of being its own structure?

#

Or are those meant to be for api methods only

copper laurel
wild flax
#

ah yeah

#

it should probably do that

copper laurel
#

I really dont see how this aligns with a Manager

#

Otherwise yeah I think so

loud jayBOT
#

pr_draft #6107 in discordjs/discord.js by shinotheshino created 56 seconds ago (review required)
refactor(CommandInteraction): create CommandInteractionOptionResolver
📥 npm i shinotheshino/discord.js#commandoptionresolver

bitter peak
#

lmk what changes are needed :]

#

@wild flax @vivid field @copper laurel

vivid field
#

I'll look at it tomorrow 👍

bitter peak
wild flax
#

@bitter peak can you fixup the spacing in the typings file

#

we don't usually space out props/methods like that

bitter peak
#

prettier did that sorry

wild flax
#

prettier surely didn't add newlines

#

:P

bitter peak
#

op lemme check

bitter peak
wild flax
#

don't really, usually, need to separate privates from publics like that

#

and they are usually at the very top in other typings

#

its a bit inconsistent, but thats better anyway

bitter peak
#

so... this?

wild flax
#

yep yep

bitter peak
#

kk

sacred river
#

possibly add a way to add permissions on slash command creation if possible

zenith oracle
#

Iirc the API doesn't support this yet

copper laurel
#

Yeah, Discord has it as separate APIs, because it doesn't really make sense as one. At least not for globals

short crescent
#

Hello everyone. I thought for a long time about the fact that many of my favorite structures were removed. I had an idea of ​​how this could be altered normally so that it would be convenient and understandable to use the extension of structures. Inside the library there are managers for each available data type. But the problem is that the GuildManager does not accept the holds parameter and is substituted inside. If we had access to replace it, then we could create classes inside the cache with the required fields and methods. Also, adding your executable method inside _add will allow you to make a query to the database for further data query. Maybe I'm wrong, but I would like to express my thought here

# 
// https://github.com/discordjs/discord.js/blob/master/src/managers/GuildManager.js

// example
class MyGuild extends Guild {
  cool = true;
}

class MyGuildManager extends GuildManager {
  constructor(client, holds, iterable) {
    super(client, holds, iterable)
  }
}

class MyClient extends Client {
  constructor() {
    super(options)
    this.guilds = new MyGuildManager(this, MyGuild)
  }
}
neat bolt
#

no dont

#

i have so many things i want to say

#

this runs into the same problems and more as the original problem

#

and please separate your database code from your discord code why do you want these things to be coupled so badly i dont understand

#

its like you intentionally want to write unmaintainable code

short crescent
#

But on the other hand, why do we need to separate the data that logically relate to the structure and move it into a separate entity, or do you have options for how to do it more correctly and better?

neat bolt
#
  1. because it makes your code more maintainable
  2. it makes your code more readable
  3. it makes everyone's code more predictable
#

as for how, literally just put that code somewhere else; in a separate class, as separate functions, whatever

#

these stuff are basic software design principles

short crescent
#

That is, create many separate functions in which, as an argument, always pass the required structure by reference? I don’t think this is the best thing to think of

neat bolt
#

it is

#

people who use Structures before and wish to continue it are too attached to the idea that just because its related it has to follow with a dot like foo.bar

#

that . is not magic

#

it doesnt make your code better just because its a dot

short crescent
#

Okay, I agree with that

neat bolt
#

please look up these terms to understand where you're going wrong: coupling, single responsibility, open-closed principle
and look up these terms for what you can do: dependency inversion & injection, repository pattern, functions

short crescent
#

Yep, i know

#

The extension approach itself is not good practice in general

#

Do you have examples of how someone has moved away from structures to another approach that is more correct in your opinion in order to study

neat bolt
#

see yuudachi/v3 @rich igloo

#

crawl never used structures in the first place

#

also its agpl so follow the license if youre gonna use its code

short crescent
#

Thanks for the answer. Yes, definitely

outer raven
#

Can someone tag shinotheshino please

#

Wanna ask them a question about their comment on their pr

zenith oracle
#

@bitter peak ↑↓

outer raven
copper laurel
#

Yeah, I dont think that's the same thing

#

Theres no reason for required but there's also kinda no reason for name either
I dont really think this resolver is quite the right approach for sub-commands, at least not the one #5880 was going for

#

interaction.options.getSubCommand() should return the name of the sub-command used

#

Or do what #5880 was going to do (I really shouldnt have shut that one down) and have that already parsed

outer raven
#

Yeah it shouldn’t have any parameters at all and find the subcommand by the type

bitter peak
#

Hmmm, @outer raven @copper laurel I feel like the way it is makes for a much better control flow when being used (see the example). Maybe name could be optional, to allow for both ways? If name is set, behave as it currently does; if it isn't set, get the current subcommand (required). Thoughts?

copper laurel
#

I don't quite understand what flow you'd have where you'd want to try to get each subcommand by name vs just... getting the one selected. It's more like string choices.

#

Can you explain a bit more?

#

In fairness, I think Discords implementation of it is kinda shit, and I think the fact we left it in options is dumb. #5880 was actually good, I misread it originally

bitter peak
outer raven
bitter peak
#

I think it's just preference tbh; although handling groups seems like it's nicer with the way it currently is

copper laurel
#

etc

#

Especially for anything more than two sub commands

#

Or in this case since it is only two, you dont need to get both, you can if/else on either one of them

bitter peak
#

Well, first off you'd have to wrap the cases in braces if you want to have block-scoped variables, so switch isn't really a great option

copper laurel
#

sure

#

i guess

ornate topaz
#

that seems like a unrelated reason

copper laurel
#

youd be putting the ifs in braces too though

#

So that just seems like a style preference, nothing wrong with braced cases

ruby terrace
#

You don’t have to use a switch, some people might break those out into completely separate functions even

copper laurel
#

Im just really struggling to see why anyone would want to have to get every possible subcommand and then check which one is defined

#

Sorry 😄

#

I feel like I must be missing something good in that design

ruby terrace
#

Especially since that requires a call to the api if you don’t already have the command in cache

bitter peak
#

Well in that case would we pull the options up to the top level, or would we return a second resolver from getSubCommand()

copper laurel
#

cause i dont see it

#

Im okay with either - #5880 was top level

#

and I was kinda an ass about it and I was wrong

bitter peak
#

Top-level seems a bit more useful to me. Although, can't you have multiple layers of subcommand groups?

copper laurel
#

nope

#

You can have top level subcommands, or subcommand groups
If you have top level subcommand groups, second level is subcommands

#

Max 3 levels, group > subcommand > options

#

I have no idea what the use case for groups is tbh

ruby terrace
#

Stuff like this

copper laurel
#

Kinda off-topic for this channel though

outer raven
ruby terrace
#

Wow it’d help if I sent the screenshot dead

copper laurel
#

eh, that could also be new remove-embed

#

i guess though

#

or whatever passes validation

ruby terrace
#

There’s a lot more options, just can’t show cause mobile

copper laurel
#

Off-topic, lets stop here

bitter peak
#

Before I push, does this seem good?

copper laurel
#

Im not sure why we need the getters

bitter peak
#

To align with the rest of the class and provide the error message

copper laurel
#

ahh you abstracted away options too

#

Yeah I think so

bitter peak
#

minus the typo on line 43 oops dogekek

copper laurel
bitter peak
#

yes I think so

copper laurel
#

and so... no options either?

ruby terrace
#

No, it’s a subcommand

copper laurel
#

you can have a toplevel subcommand with a toplevel group?

ruby terrace
#

Can’t shave a group without a subcommand, won’t even show up

#

Yes, you can mix and match

copper laurel
#

god damnit discord

ruby terrace
#

People always get confused when I say that lmao

copper laurel
#

Okay so.... _subcommand could be defined and _group not

#

But if _group is defined then _subcommand has to be defined, and in that group

ruby terrace
#

Yeah

copper laurel
#

/command bar foo
/command foo
These are both foo subcommand?

bitter peak
#

Hmm, that makes throwing an error in getSubCommandGroup kinda risky

copper laurel
#

but one has null group

ruby terrace
#

Correct

copper laurel
#

ughghufgubfhdjgk yeah that needs required then

ruby terrace
#

You cannot have a subcommand with the same name as a group IIRC

copper laurel
#

to control the throw

#

Fuck this design is terrible, why Discord

ruby terrace
#

*at the same level

bitter peak
#

Alright so that becomes:

outer raven
bitter peak
#

Yes

outer raven
#

hm i think it would make sense for those to be getters, you can easily remove the required param from the group function too

vivid field
#

I'd even argue that you can just make subCommand and group normal properties instead of using methods/getters

wild flax
#

Switches make for way better control flow

copper laurel
#

Yeah looks like we did, current debate seems to be it should still be a method at all

wild flax
#

Compared to?

#

a getter?

copper laurel
#

Properties

#

Why getters at all

wild flax
#

I don't know im just asking

#

Why properties

bitter peak
#

the rest of the class is methods, so it doesn't seem right to make this one a property

outer raven
#

the rest of the class works for multiple options, you can only have 1 group and 1 subcommand

zenith oracle
#

Why is APIInteractionGuildMember#permissions a Snowflake instead of Readonly<Permissions> like a normal GuildMember? Can I use new Permissions(<APIInteractionGuildMember>.permissions) to get the correct Permissions object?

vivid field
#

Because discord only sends the bitfield, discord.js then constructs a Permissions object out of that

zenith oracle
#

Well, yes about the first part, but if we have an interaction (let's assume a slash command), we may want to check the permissions of the member of the command using interaction.member.permissions.has() for example, but we can't since the .permissions can also returns a Snowflake...

vivid field
#

Then you can construct a Permissions object manually, sure

tacit crypt
#

Anything prefixed with API is usually a type from discord-api-types, which represents raw Discord data

zenith oracle
zenith oracle
outer raven
#

@tacit crypt idk if you read the convo but the subcommand method isn’t the same because you don’t need either of the params

#

It should even be a property imo

tacit crypt
#

I mean from what I saw in the code, right now they're glorified getters

outer raven
#

Yeah, that’s what it should be

tacit crypt
#

neither of them should be functions nor getters

outer raven
#

A getter/property

tacit crypt
#

no getters??

#

why would you make it a getter

outer raven
#

Idrk

#

Idk what the advantages of getters are tbh so I won’t comment on that

tacit crypt
# 
declare const options: APIInteractionDataOptionsIDoNotRememberTheNameOkSueMe;

if (options[0].type === 'SUB_COMMAND_GROUP') {
  this.subCommandGroup = options[0].name;
  const [subCommand] = options[0].options;
  this.subcommand = subCommand.name;
  this._options = subCommand.options;
} else if (options[0].type === 'SUB_COMMAND') {
  this.subCommand = options[0].name;
  this._options = options[0].options;
}
wild flax
void rivet
#

Hey, I'm new here so this might not be the channel for it but I wanted to talk about a certain feature implemented on discord.js (cache check)
So' this is regarding intents, the cache system and fetching
Now, with intents being a thing and discord.js enforcing the idea of specifically adding the ones you need, we can limit which events the client receives, which discord.js' cache depends on to stay up to date, and fetching also checks the cache first I believe
my example would be guild.members.fetch, which (unless explicitly told to skip the cache check ) checks the cache

  async _fetchSingle({ user, cache, force = false }) {
    if (!force) {
      const existing = this.cache.get(user);
      if (existing && !existing.partial) return existing;
    }
```keep this in mind
now a scenario, let's say a client has GUILDS, GUILD_MESSAGES and GUILD_MESSAGE_REACTIONS intents enabled
the (relevant) events they'll receive would be
`messageReactionAdd`, which discord.js will use to cache the GuildMember, at this point in the cache, there is a GuildMember instance for member x (our reactee)
 now note that since there isn't GUILD_MEMBERS, the client won't be sent events like guildMemberUpdate, so if i were to add a role (role1) on x, that wouldn't trigger the event, discord.js wouldn't be able to internally update the cache, right?
now, the fetch, what the fetch does is that it `const existing = this.cache.get(user);` just checks if the cache has the instance, that's enough to make it not call the api, it doesn't check if the data is up to date by my understanding
so in this scenario, I am fetching a member, however, it's not giving me the up to date data , so my question is and what I wanted to talk about was if this cache check feature is useless now or if i'm misunderstanding it of course
zenith oracle
#

Well, there's a force option for a reason. You (usually) don't need to fetch a user if you have it already in the cache, but if scenarios, like the one you showed, occurs, we can use the force option to ignore the cache...

copper laurel
#

Avoiding API calls you dont need to make is never useless, but yes, if you've chosen not to receive events that maintain the state of the cache, then you also need to make sure you're not relying on cache where appropriate

ruby terrace
#

Another option is with custom caching if you don't set GUILD_MEMBERS intent for example you can set GuildMemberManager: () => new LimitedCollection() that way it never caches. I think that would probably be the intended use, not 100% sure though.

void rivet
#

I know the force exists and I have mentioned it before, however,I don't think the users were made aware of this, that even the fetch which's point (i always thought) was to give you the up to date data
in this case, a possible solution may be tweaking cache check to check if the client had the necessary intents passed on
like for this, we need GUILD_MEMBERS, since we want the guildMembersUpdate event, (theoretically that should keep the member cache up to date)
so checking Client.options.intents, if it has the GUILDS and GUILD_MEMBERS intents should do it?

  async _fetchSingle({ user, cache, force = false }) {
    if (!force) {
      const existing = this.cache.get(user);
      if (existing && !existing.partial && this.client.options.intents.has(["GUILDS", "GUILD_MEMBERS"])) return existing;
    }
``` is this a possible workaround to it?
copper garden
#

Pretty sure intents doesn’t apply to fetching a single member

void rivet
void rivet
# void rivet Hey, I'm new here so this might not be the channel for it but I wanted to talk a...

@copper garden the original issue was that with a few certain intents disabled, the cache stores an entry however it's no longer updated (since the cache depends on certain events to stay up to date and not having intents means the client won't get said events)
now the "cache check" only checked for const existing = this.cache.get(user); an entry
and if it found one regardless of if it's outdated, it would return that instead of an api call, yes I am aware you can force and I mentioned that above too
Yes intents may not affect calling the api, but not having them could result in discord.js returning the outdated instance even before reaching to call the api

ruby terrace
#

I get what you're saying about checking intents btw, just not sure if its the right design decision

void rivet
#

that would completely discard caching in this case, right?

ruby terrace
#

yes

void rivet
#

honestly don't know if it's better but what do you think is wrong with checking the intents?

ruby terrace
#

hmm, actually I have an idea, default force to !client.options.intents.has(['GUILDS', 'OTHER_RELEVANT_INTENT']), that way the dev can still pass false or true if they want. The issue I have is the way that things are intertwined, like you need guilds for pretty much everything for example. Figuring out what depends on what isn't really something each manager should have to worry about

void rivet
#

I get that you are trying to say, if we disable the cache, it won't store the data, since at this point if the intents aren't there, this specific cache seems useless

ruby terrace
#

although to be fair, you won't have a guild member manager without a guild, so you don't necessarily need the guilds intent check, plus you could've fetched the guild

void rivet
#

i feel like, this may have unexpected side effects
intents checking there would be safer i feel like

void rivet
wild flax
#

You never get outdated data

#

Data just happens to become outdated without intents

#

If you do not turn on specific intents, it’s your job to make sure to constantly fetch a member IMO

#

I don’t think the library should hardcode intents everywhere now to circumvent that

ruby terrace
wild flax
#

Yeah that seems the most sensible approach

void rivet
#

Yeah I agree that intents shouldn't be hardcoded and that was never recommended

#

I am still relatively new to this library when it comes to internal stuff
but judging by what I'm seeing so far, disabling this particular cache completely may be a workaround
However, there is a specific thing I used to use which I feel like is now more complicated
As you know messageReactionAdd only gives you the MessageReaction and the User, however, the API's event actually gives the client a member object( of the reactee)

client.on("messageReactionAdd", (reaction, user) => {
  const member = reaction.message.guild?.members.cache.get(user.id)
})
``` I would've been pretty confident that it's cached since I know the api gives you the member
however here, I need to make the fetch
so here, I am required to make an api request I know could've been avoided
also I would like to ask why this event doesn't have an optional third parameter or something to pass on the member
wild flax
#

Yeah this is what I don't get about this explaination

#

Doesn't this indicate the member gets always freshly cached?

#

So you would only be hitting the problem if you try to use the members cache OUTSIDE of the event

void rivet
#

I am talking about the suggestion that says the lack of required intents would result in the cache being disabled completely

wild flax
#

Well I don't see this as a needed solution

#

I am talking about your original concern

#

If you use

client.on("messageReactionAdd", (reaction, user) => {
  const member = reaction.message.guild?.members.cache.get(user.id)
})

the member will always be up2date in this event. No matter what.

#

If you however start using it in different places, without:
const member = await message.guild?.members.fetch(id, { force: true })

void rivet
wild flax
#

Yeah youll run into issues

#

But I am asking myself, why would you knowingly use the cache (.cache) if you know you won't get GUILD_MEMBER_UPDATEs

void rivet
#

in this case because I know the api is giving me the up to date member in this event

wild flax
#

Yes, but the member in the "event" will always be up2date

#

Since we update the cache when we get a new member from this event

#

That doesn't mean other events wouldnt update it, and since you knowingly didnt enable them..

oak quail
wild flax
#

Yeah just do

void rivet
#

I'm a bit lost here, could you give me an example of what you mean?

wild flax
#

I did above

void rivet
#

I am aware that outside of this event, it's outdated

wild flax
#

Yeah, and inside of it it will always be the latest up2date member

#

and the cache will be updated

#

Since even if it exists, we patch the new data on top of it

void rivet
#

exactly, I know that
but i was talking about the cache being disabled completely, and said that this would be a side effect
if you disable the cache completely, (limit it to 0 entries) this member won't be cached

client.on("messageReactionAdd", (reaction, user) => {
  const member = reaction.message.guild?.members.cache.get(user.id)
})
``` and here, member will be undefined
wild flax
#

Well yes

#

But that is a deliberate decision of yours

#

No?

#

Why would you access a cached instance if you know you disabled it

#

I feel, if anything, we should do more educating on that behaviour, but not change code because of it

void rivet
#

I wouldn't, that was an example, saying that here I am avoiding to call the api since it's definitely not needed
Be right back

wild flax
#

But you just disabled cache?

#

So your decision is to prefer API calls

#

messageReactionAdd is also kind of an odd child here because its the only event that actually gives you a member from the API, but we don't directly expose it

#

But thats a rather interesting design decision

#

If the problem exists anywhere else, I'd happy to discuss this further, but if its only this event I would rather fix the event by possibly returning (reaction, user, member?) =>

#

Even if its a very awkward interface lol

oak quail
ruby terrace
wild flax
#

But sure, preferably we should fix all of them

copper laurel
#

But there's a valid point about not receiving it or caching it at all I guess

wild flax
#

Might be worth to revisit

#

Esp with disabling cache

copper laurel
#

Fetch is always an option anyway

wild flax
#

Yeah but its kinda pointless to fetch if you technically get the data lol

#

We just don't expose it to you

copper laurel
#

I definitely prefer a third sometimes-there param to inconsistent types on the second

wild flax
#

Yeah, I wouldnt wanna do user/member

void rivet
void rivet
# wild flax Even if its a very awkward interface lol

I am new to typescript/javascript in general I guess so I don't quite get what you mean by an awkward interface
sorry if this is dumb but I would like to know what you mean by that
to me "GuildMember | null" seems fine when compared to presenceUpdate where the first parameter is Presence or null

wild flax
#

The “User | GuildMember | null” would be a very awkward one

#

Since it can’t be a member in dms

void rivet
#

Yeah that would be awkward, I thought you were talking about the one with the third parameter

oak quail
#

for user banner color discord returns a hex string ("#123456"), which is inconsistent with embed colors. should djs just return that or also have a getter for the color as a number?

wild flax
#

You could check on the roles

#

We have a displayColor thing there I think

#

And also a hex prop

zenith oracle
#

Roles have a color property in base 10 and hexColor getter in hexadecimal

oak quail
#

roles do the same thing embeds do ^

#

bc discord returns a number for role colors too

wild flax
#

Then we should copy that behavior IMO

oak quail
#

but banner colors are inconsistent for some reason

wild flax
#

And just transform discord’s stuff

#

Or maybe ask in the docs PR

#

Fucking dumb

#

(╯°□°）╯︵ ┻━┻

oak quail
#

so would hexColor be the property and color be the getter, or should it store the converted color and add a hexColor getter to un-convert it

#

i'll also comment on the docs pr

void rivet
#

I feel like I made this conversation drag on for a long time, sorry, it's 5 am here I can't think but I'll end it with what i came out with from this
fetch should check client's intents, if it seems like the data might be outdated, skip the cache check, by my understanding, users would expect the latest data, the reason why I am not okay with completely disabling this cache is because i feel like it'll lead to unnecessary api calls like with the event I mentioned
Lastly, the users should be told that the intents don't just disable some events, it also causes cache issues internally. They should know what they're doing and what to expect.
Thanks a lot for this conversation, I explored and learned a lot thanks to all of you, goodnight

wild flax
#

Probably no transform for the raw data

#

and transform into an int via the getter in this case

#

Actually annoying how backwards it is

#

Literally ass

oak quail
#

oh

#

user#fetch(true) is broken lol

#

ig i'll fix that in a different pr so it can get merged faster

raven cloak
#

Have channel type names been made uppercase?

oak quail
#

yes

raven cloak
#

Why so?

#

Consistency?

wild flax
oak quail
raven cloak
oak quail
#

since u have to fetch the user to get user banner & banner color (its not always on the user object)

#

should I add fetchBanner, fetchBannerURL, fetchBannerColor, and fetchHexBannerColor methods, like the current fetchFlags?

wild flax
#

Wait what

#

Why not just fetch banner that populates all of those

oak quail
#

what should it return tho

#

just the banner hash?

wild flax
#

void mmLol

oak quail
#

isnt that just fetch(true) tho

wild flax
#

I dunno, the patched user?

oak quail
#

i mean

wild flax
#

I don't even know why we don't do that everywhere

oak quail
#

fetchFlags is essentially just fetch(true) that returns .flags

#

i assume theres some situation where user.flags exists but isnt sent

wild flax
#

If we need to fetch something, best case scenario we should just return the patched user

#

cc @tacit crypt @solemn oyster

tacit crypt
#

So fetchBanner() = this.fetch()?

wild flax
#

yeah

#

essentially

#

lol

#

I know its awkward but I just don't like arbitrarily returning data based on the fetch

tacit crypt
#

I mean at that point we would drop fetchFlags and just tell users to call user.fetch if null

wild flax
#

yeah thats even better

tacit crypt
#

What do with emojis/stickers then

oak quail
#

what about them

#

how are they relevant 🤔

tacit crypt
#

fetchGuild

wild flax
#

well for all of those

oak quail
#

oh no not this again

wild flax
#

we should really just return the whole instance

#

it feels so awkward and dumb to have 5 methods just to return 5 strings from them

#

we don't do tell people to do .fetchFull() either when its a partial right