#archive-library-discussion

25085 messages · Page 10 of 26

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

Sure, I'll do! 👍

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

Isn't this exactly what partials are

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

Thonk

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

monkaBless

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

iara_shrug

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

meguFace

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

and then return everything but the id

because its always there on a partial

lol

oak quail

so on the banner properties should I add a "The user must be force fetched" note?

tacit crypt

Well, force isn't really needed cuz it defaults to true anyways

But the main fun factor is how you'll differentiate from no banner and no fetched banner

oak quail

cuz it defaults to true anyways
no it doesnt?

afaik its always been default false

tacit crypt

Not in the manager function, in the user function

oak quail

false there too

wait wtf is the point then

tacit crypt

I mean it should probably default to true, because partial users

@wild flax @solemn oyster ^ thoughts

wild flax

yeah imo on instances it should default to fetch(true)

but on managers it should be fetch(false)

oak quail

should i change that in the fix pr too

wild flax

Sure

oak quail

done

oak quail

made the banner pr

raven cloak

Why did presence get moved to member, rather than user

Even on Discord API side

What made that change

wild flax

A presence is never on a user

raven cloak

It was in v12

wild flax

That was us doing it

unique axle

it was always just a getter that tried to retrieve the presence from the member on the first guild it happened to stumble across
however there were edge cases that resulted in this not working and a surplus of the suggestion "use message.member.presence instead of message.author.presence"
presence data is inherently guild based data and we probably should've never started to provide it to the user

wild flax

you also get presences when being friends, but since thats not a thing with bots

clever spoke

is there any chance djs would add undocumented features?

raven juniper

no - see stuff like mobile indicator

clever spoke

understandable

real jetty
gusty carbon

Would it make sense for the library to add a Message#removeComponents method, similarly to how there is Message#removeAttachments?

copper laurel

tbh I thought that was kinda a silly method since all it does is message.edit({ attachments: [] });

rough glacier

Yeah, it doesn't really make sense for only this to exist. It's the same thing with removing components. Either we create one for everything (components, embeds, attachments) or none

gusty carbon

yea thats what i thought too
but if we keep it we might as well add the components method

clever spoke

what if we removed Message#removeAttachments

I guess maybe it doesn't make sense because its not causing any harm

real jetty

or a singular method taking arguments as to what to remove?

gusty carbon

would probably be the best decision, it doesn't really provide much value

gusty carbon
rough glacier

Then again, lib enforces using methods rather than constructor objects in guides. In embeds for ex, it's proposed to use emb.addDescription().addField() rather than passing values to embed constructor. It could make sense for a similar approach msg.removeEmbeds().removeComponents()

clever spoke

what about something like message.remove(["embeds", "components"]);
like Mega said

gusty carbon

that seems kinda janky honestly, imo i would either add both methods or remove removeAttachments

real jetty
copper laurel

No

Just confirmed - the reason that exists is because you can remove the attachments from other people's messages if you have MANAGE_MESSAGES

Much like supressEmbeds also exists

We aren't going to add helpers for every other edit of your own messages

gusty carbon
outer raven

@oak quail did u change anything in the icon function?

oak quail

dynamic wasnt in the typings before

that change was copied from the old banner pr

zenith oracle
outer raven

alright but can you remove 'default' then? I have no clue what that's supposed to be but it doesn't work

outer raven

btw @bitter peak about your command options PR, can you please make all functions return undefined instead of null just like collection.get does?

copper laurel

yeah should the default format be webp or something

null is more correct for those imo

outer raven
wild flax

they arent a collection

why would it matter

copper laurel

they are methods, not a hashmap

outer raven

because undefined is more accurate than null

copper laurel

no, i dont think it is

wild flax

based on what

outer raven

because these options were not specified by the user who ran the command

it's not like they were specified as null

wild flax

imo null is always more accurate

we only use undefined for collections because map does

to not completely break the map interface

copper laurel

undefined is a dumb type that doesnt exist in a lot of other languages

outer raven

but it's way easier to work with undefined types because if you have an optional parameter in a function you can't assign null to that

wild flax

?

solemn oyster

?

We use undefined only when exclusively needed

bitter peak

Null is more of an explicit empty type whereas undefined is an implicit one. It's pretty much always better to use null for things like this unless it breaks something else

solemn oyster

Such as skipping fields from JSON.stringify

tacit crypt

Why are subCommand/subCommandGroups not... properties meguFace

copper laurel

they are. private properties lol

tacit crypt

Not what I asked

They shouldn't be glorified getters

copper laurel

oh I know

wild flax

I dont see why not

copper laurel

I dont know why theyre private and not just properties

outer raven
wild flax

thats more a you problem

null can be a default value for optional params

outer raven

yeah but why do this

function example(param: string | null) {}

when you can just do this

function example(param?: string) {}
wild flax

bruh

copper laurel

we.... dont

outer raven

that doesn't even make sense because you want the param to be optional, not null

outer raven
copper laurel

dont write shit functions

dont pass variables you havent checked to functions

wild flax

function example(param?: string | null) {}

is the best of both worlds

and youd need to do it anyway

since not everything returns undefined

outer raven

yeah but the | null there is redundant

wild flax

no it isnt

if you ever pass a null value the ? wont allow it

outer raven
outer raven
wild flax

it doesnt mean the same thing

undefined is not null

outer raven

then what's the difference

wild flax

learn js thats the difference

outer raven

I don't get why you guys like the null type so much it's just annoying to work with

copper laurel

undefined is not the lack of a value. Its the lack of any definition at all, a property not existing in an object, etc
null is a specific, defined return value denoting that everything was successful, but theres no value found. A function should not be returning undefined, it should return null when it has no value to return because it deliberately, knowingly has no value

Because its correct

If JS wasn't such a loose language, undefined wouldnt exist. Accessing undefined things would throw errors in a good language like C#

outer raven
copper laurel

But this is a command option resolver

A resolver function should return null

accessing a property that isnt defined yes, should return undefined but thats not what youre doing

You dont have to use the resolver if you want the map-like behaviour

You're drawing a false equivalency to an option being null, and an option resolver method returning null. They arent the same

outer raven

But Discord will never allow you to pass null to any type of option, it only lets you not pass it at all and the API doesn't pass anything as null either, so why change that behavior?
You could simply try to find the option in the array and return that regardless of the outcome (meaning that you wouldn't be explicitly returning undefined, it's just what the find function returns if that makes sense)

wild flax

What has that to do with sending something to discord when we are transforming received data?

We strife to write good and maintainable code, not some mess.

undefined is a mess that should not exist in the first place, the only reason we use it is because of quirks in the current language

outer raven

aight if typescript itself is doing that then sure there's no point to change to undefined

wild flax

lol

It's almost like we know what we are talking about

bitter peak

@outer raven The reason I have methods for getSubCommand and getSubCommandGroup is to perform validation as all the other methods do. e.g. getSubCommand() asserts that there actually is a subcommand, moving the burden of checking away from end-user code.

cc @tacit crypt bc you asked too

tacit crypt

??

outer raven

but if you wanna check if there is a subcommand or a group just check if they're null ?

bitter peak

The same could be said for all the get* methods

But the rest of the PR follows that assertion logic