#archive-library-discussion | discord.js - imagine an app | Page 11

wild flax Jul 10, 2021, 7:45 PM

#

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

#

Like I fully agree and we should do it

vagrant holly Jul 10, 2021, 7:46 PM

#

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 Jul 10, 2021, 7:46 PM

#

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 Jul 10, 2021, 7:46 PM

#

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 Jul 10, 2021, 7:48 PM

#

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 Jul 10, 2021, 7:49 PM

#

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 Jul 10, 2021, 7:49 PM

#

getters use memory too tho cryingmansad

wild flax Jul 10, 2021, 7:49 PM

#

"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 Jul 10, 2021, 7:51 PM

#

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

wild flax Jul 10, 2021, 7:51 PM

#

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

vagrant holly Jul 10, 2021, 7:51 PM

#

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

wild flax Jul 10, 2021, 7:53 PM

#

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 Jul 10, 2021, 7:54 PM

#

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 Jul 10, 2021, 7:55 PM

#

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 Jul 10, 2021, 7:56 PM

#

'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 Jul 10, 2021, 7:58 PM

#

but what if you could just do

#

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

vagrant holly Jul 10, 2021, 8:00 PM

#

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 Jul 10, 2021, 8:01 PM

#

Not as userfriendly honestly

#

but I guess yeah

vagrant holly Jul 10, 2021, 8:01 PM

#

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 Jul 10, 2021, 8:15 PM

#

monkaBless

oak quail Jul 10, 2021, 9:45 PM

#

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

#

#archive-offtopic message
#archive-offtopic message

vagrant holly Jul 10, 2021, 9:51 PM

#

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 Jul 10, 2021, 9:54 PM

#

left moat getters use memory too tho <a:cryingmansad:746511853169016853>

Prove it

solemn oyster Jul 10, 2021, 9:54 PM

#

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

0 effect

#

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 Jul 10, 2021, 9:58 PM

#

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 Jul 11, 2021, 3:03 AM

#

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 Jul 11, 2021, 3:08 AM

#

on what basis, because they are the same thing

solemn oyster Jul 11, 2021, 4:28 AM

#

@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 Jul 11, 2021, 4:38 AM

#

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

Memory usage goes to space

drifting knot Jul 11, 2021, 4:38 AM

#

?

solemn oyster Jul 11, 2021, 4:38 AM

#

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 Jul 11, 2021, 4:39 AM

#

uh

#

what are you talking about

#

opaque types

#

build time

#

typescript

solemn oyster Jul 11, 2021, 4:39 AM

#

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 Jul 11, 2021, 4:42 AM

#

are you reading my messages at all

solemn oyster Jul 11, 2021, 4:42 AM

#

Yes

drifting knot Jul 11, 2021, 4:42 AM

#

solemn oyster Jul 11, 2021, 4:42 AM

#

And it makes no sense

drifting knot Jul 11, 2021, 4:42 AM

#

okay 👍 👍

solemn oyster Jul 11, 2021, 4:42 AM

#

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 Jul 11, 2021, 4:46 AM

#

BUILD TIME

#

ONLY TYPESCRIPT

#

I DO NOT CARE ABOUT JS USERS

solemn oyster Jul 11, 2021, 4:46 AM

#

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 Jul 11, 2021, 4:47 AM

#

that's the only way to do opaque types

#

that is how everyone does it

#

it is a feature of the language

solemn oyster Jul 11, 2021, 4:47 AM

#

Yes

#

And I'm telling you

drifting knot Jul 11, 2021, 4:47 AM

#

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 Jul 11, 2021, 4:47 AM

#

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 Jul 11, 2021, 4:50 AM

#

tell me more about your Discordjs Bot

solemn oyster Jul 11, 2021, 4:50 AM

#

Outside this channel, probably in DMs

rough arrow Jul 11, 2021, 10:28 AM

#

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

wild flax Jul 11, 2021, 10:28 AM

#

Tabs are simply superior

#

https://www.reddit.com/r/javascript/comments/c8drjo/nobody_talks_about_the_real_reason_to_use_tabs/

#

If you want a "real" reason read this.

#

But either way, point still stands. Superior.

rough arrow Jul 11, 2021, 11:16 AM

#

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 Jul 11, 2021, 11:16 AM

#

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 Jul 11, 2021, 11:25 AM

#

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 Jul 11, 2021, 11:30 AM

#

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

outer raven Jul 11, 2021, 11:31 AM

#

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 Jul 11, 2021, 11:31 AM

#

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 Jul 11, 2021, 11:33 AM

#

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 Jul 11, 2021, 11:33 AM

#

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

outer raven Jul 11, 2021, 11:33 AM

#

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 Jul 11, 2021, 11:34 AM

#

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 Jul 11, 2021, 11:35 AM

#

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 Jul 11, 2021, 11:35 AM

#

For all of the reasons listed above

wild flax Jul 11, 2021, 11:35 AM

#

Why are people like this

copper laurel Jul 11, 2021, 11:36 AM

#

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 Jul 11, 2021, 11:36 AM

#

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 Jul 11, 2021, 11:37 AM

#

a raw object is barely more code

outer raven Jul 11, 2021, 11:38 AM

#

copper laurel Because Discord might introduce new types of rows Because we might have to suppo...

I'm just saying that at the moment there are no signs that Discord would ever do that and if they do I can't see that happening in the near future
If they do add those columns or whatever it might be then this can be removed in the PR that adds support for them, but why now?

copper laurel Jul 11, 2021, 11:39 AM

#

because we cant make breaking changes later

#

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

outer raven Jul 11, 2021, 11:39 AM

#

can't you just do a 13.x?

copper laurel Jul 11, 2021, 11:41 AM

#

not to remove something

#

do you understand semver?

solemn oyster Jul 11, 2021, 11:41 AM

#

Removing features = semver major = v13 -> v14

amber basalt Jul 11, 2021, 11:41 AM

#

What is semver?

solemn oyster Jul 11, 2021, 11:42 AM

#

https://semver.org

outer raven Jul 11, 2021, 11:43 AM

#

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

solemn oyster Jul 11, 2021, 11:43 AM

#

iara_shrug

outer raven Jul 11, 2021, 11:44 AM

#

Alright then

copper laurel Jul 11, 2021, 11:44 AM

#

- components: [[button]]
+ components: [{ type: 1, components: [button] }]`

outer raven Jul 11, 2021, 11:45 AM

#

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 Jul 11, 2021, 11:46 AM

#

solemn oyster In the topic of memory usage (that IPv4 has brought over), should we look into r...

Taking a quick glance at source, this seems to power two things right? It backs all the methods for seeing who is typing in channels, but also backs being able to set the client to be typing somewhere?

copper laurel Jul 11, 2021, 11:46 AM

#

if you want me to copy and paste that, sure

outer raven Jul 11, 2021, 11:46 AM

#

copper laurel if you want me to copy and paste that, sure

but that only works for 1 button, not for multiple buttons across multiple rows

copper laurel Jul 11, 2021, 11:47 AM

#

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

outer raven Jul 11, 2021, 11:47 AM

#

sure

copper laurel Jul 11, 2021, 11:48 AM

#

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 Jul 11, 2021, 11:50 AM

#

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 Jul 11, 2021, 11:53 AM

#

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

remote wasp Jul 11, 2021, 11:56 AM

#

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 Jul 11, 2021, 11:57 AM

#

and the "bloat" is singular boolean variable?

remote wasp Jul 11, 2021, 11:59 AM

#

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 Jul 11, 2021, 11:59 AM

#

just because of the scale we expect its been used at

#

and there isnt much harm in that existing

remote wasp Jul 11, 2021, 12:04 PM

#

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 Jul 11, 2021, 12:04 PM

#

it isnt a problem though

#

it wasnt a problem for 12 versions and it isnt now

solemn oyster Jul 11, 2021, 12:05 PM

#

True, @dev releases have around 5000 downloads a week

remote wasp Jul 11, 2021, 12:07 PM

#

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 Jul 11, 2021, 12:07 PM

#

we'd remove them in v14

#

thats what deprecations are

remote wasp Jul 11, 2021, 12:09 PM

#

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

wild flax Jul 11, 2021, 12:12 PM

#

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

#

How

tacit crypt Jul 11, 2021, 12:13 PM

#

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 Jul 11, 2021, 12:26 PM

#

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 Jul 11, 2021, 12:28 PM

#

Yeah but considering they get a warning in the console even

rough arrow Jul 11, 2021, 12:50 PM

#

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 Jul 11, 2021, 12:51 PM

#

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 Jul 11, 2021, 12:54 PM

#

ornate topaz or maybe you have some arguments against tabs

Not really no, I have no issue with the library using tabs.

rough arrow Jul 11, 2021, 12:54 PM

#

ornate topaz and by the way, yes. that reddit post was brought up few times before. it's not ...

I see, that was the impression that I got.

#

Thank you!

outer raven Jul 11, 2021, 3:23 PM

#

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 Jul 11, 2021, 3:27 PM

#

If you want a member grab it from a guild

outer raven Jul 11, 2021, 3:32 PM

#

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 Jul 11, 2021, 3:37 PM

#

"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 Jul 11, 2021, 3:43 PM

#

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

That’s exactly why I suggested for it to be sent along the user cuz it can sometimes be null

ornate topaz Jul 11, 2021, 3:45 PM

#

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 Jul 11, 2021, 4:01 PM

#

ornate topaz it would seem weird that you sometimes do get member, sometimes you don't. with ...

I never said you would assume anything, I just said the member object would be sent when it makes sense for it to be sent, that is when the message is in a guild

ornate topaz Jul 11, 2021, 4:16 PM

#

what

strange igloo Jul 11, 2021, 4:32 PM

#

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

ornate topaz Jul 11, 2021, 4:45 PM

#

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 Jul 11, 2021, 4:46 PM

#

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 Jul 11, 2021, 4:47 PM

#

resolve already defaults to null upsidemmlol

#

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

strange igloo Jul 11, 2021, 4:47 PM

#

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

ornate topaz Jul 11, 2021, 4:47 PM

#

regardless, !member

#

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

oak quail Jul 11, 2021, 11:31 PM

#

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 Jul 11, 2021, 11:41 PM

#

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 Jul 12, 2021, 12:09 AM

#

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 Jul 12, 2021, 12:15 AM

#

thats because youre on v12

#

its EARLY_VERIFIED_BOT_DEVELOPER in v13

boreal vapor Jul 12, 2021, 12:20 AM

#

oak quail its `EARLY_VERIFIED_BOT_DEVELOPER` in v13

I just tested in a v13 bot, you are right, but the v12 docs are as EARLY_VERIFIED_BOT_DEVELOPER, but this is better suited for the other channel

oak quail Jul 12, 2021, 12:58 AM

#

well v12 is done, it wont be changed

real jetty Jul 12, 2021, 7:51 AM

#

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 Jul 12, 2021, 7:53 AM

#

https://github.com/discordjs/discord.js/issues/5771

real jetty Jul 12, 2021, 7:57 AM

#

interesting, thanks for the read

rough arrow Jul 12, 2021, 8:46 AM

#

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 Jul 12, 2021, 8:55 AM

#

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 Jul 12, 2021, 7:16 PM

#

Would a CommandOptionResolver class like https://git.io/JCkad be a good addition?

#

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

fallow crater Jul 12, 2021, 7:27 PM

#

maybe it could be implemented in @discordjs/builders?

bitter peak Jul 12, 2021, 9:19 PM

#

@tacit crypt what do you think about this ^?

tacit crypt Jul 12, 2021, 9:22 PM

#

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 Jul 12, 2021, 9:25 PM

#

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 Jul 12, 2021, 9:28 PM

#

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 Jul 12, 2021, 9:42 PM

#

Nah, I mean thatd be fine

#

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

bitter peak Jul 12, 2021, 10:08 PM

#

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

How about src/interactions/slashCommands/resolvers.ts, export class SlashCommandOptionResolver?
Also, for the types, both discord.js@amber obsidian and discord-api-types@next are required. Should these be peerDependencies? Or how should that work?

#

oops sorry for ping person

#

discord.js@dev*

wild flax Jul 12, 2021, 10:09 PM

#

djs types are required?

bitter peak Jul 12, 2021, 10:09 PM

#

CommandInteractionOption

#

and User etc.

wild flax Jul 12, 2021, 10:10 PM

#

hmm

bitter peak Jul 12, 2021, 10:10 PM

#

Maybe this would belong in the main lib instead?

wild flax Jul 12, 2021, 10:10 PM

#

probably so yeah

bitter peak Jul 12, 2021, 10:11 PM

#

Should it replace CommandInteraction#options?

#

Where the current raw collection would be something like rawOptions

wild flax Jul 12, 2021, 10:11 PM

#

Neither are raw

bitter peak Jul 12, 2021, 10:11 PM

#

Hmm, maybe optionsCollection or something then?

#

Or make it a property of the resolver

wild flax Jul 12, 2021, 10:11 PM

#

I think it should be opt-in if anything

bitter peak Jul 12, 2021, 10:12 PM

#

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

wild flax Jul 12, 2021, 10:12 PM

#

I don't necessarily wanna decide this alone so uh

#

cc @copper laurel @vivid field

#

#archive-library-discussion message

copper laurel Jul 12, 2021, 10:15 PM

#

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 Jul 12, 2021, 10:18 PM

#

correct

#

Also has support for subcommands and subcommand groups

copper laurel Jul 12, 2021, 10:19 PM

#

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 Jul 12, 2021, 10:20 PM

#

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

copper laurel Jul 12, 2021, 10:20 PM

#

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 Jul 12, 2021, 10:21 PM

#

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 Jul 12, 2021, 10:23 PM

#

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 Jul 12, 2021, 10:24 PM

#

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 Jul 12, 2021, 10:25 PM

#

probably yeah

wild flax Jul 12, 2021, 10:25 PM

#

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

copper laurel Jul 12, 2021, 10:25 PM

#

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

wild flax Jul 12, 2021, 10:25 PM

#

that does no validation

copper laurel Jul 12, 2021, 10:25 PM

#

Wouldnt that be interaction.options.data.get

bitter peak Jul 12, 2021, 10:25 PM

#

^

copper laurel Jul 12, 2021, 10:25 PM

#

or do you want to promote it up

wild flax Jul 12, 2021, 10:25 PM

#

I wouldn't want to expose raw data at all

#

we don't do that anywhere in the lib rn

copper laurel Jul 12, 2021, 10:25 PM

#

Well that "raw" data is still the parsed collection

bitter peak Jul 12, 2021, 10:26 PM

#

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

copper laurel Jul 12, 2021, 10:26 PM

#

actually

#

https://github.com/remoji-bot/remoji-bot/blob/discordjs-13/src/core/base/CommandOptionResolver.ts#L43

#

Doesnt this already do that

wild flax Jul 12, 2021, 10:26 PM

#

yah

bitter peak Jul 12, 2021, 10:26 PM

#

oh yeah

copper laurel Jul 12, 2021, 10:26 PM

#

cool

bitter peak Jul 12, 2021, 10:26 PM

#

forgot I added that lmao

wild flax Jul 12, 2021, 10:26 PM

#

id like to keep that

copper laurel Jul 12, 2021, 10:26 PM

#

as long as required defaults to false

vivid field Jul 12, 2021, 10:27 PM

#

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 Jul 12, 2021, 10:27 PM

#

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 Jul 12, 2021, 10:28 PM

#

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

copper laurel Jul 12, 2021, 10:28 PM

#

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

bitter peak Jul 12, 2021, 10:28 PM

#

vivid field Sure we can keep the methods, but e.g. the `required` parameter seems unnecessar...

It allows for more type inference (string | null for non-required strings, just string for required ones)

#

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 Jul 12, 2021, 10:30 PM

#

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

bitter peak Jul 12, 2021, 10:30 PM

#

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 Jul 12, 2021, 10:34 PM

#

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 Jul 12, 2021, 11:09 PM

#

Before I go too far in the wrong direction, does this look alright? @copper laurel @vivid field @wild flax
https://gist.github.com/shinotheshino/845cd2fb41ca2ae2d71762c3d36fb573

#

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

wild flax Jul 12, 2021, 11:10 PM

#

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 Jul 12, 2021, 11:10 PM

#

yes

wild flax Jul 12, 2021, 11:10 PM

#

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 Jul 12, 2021, 11:12 PM

#

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 Jul 12, 2021, 11:25 PM

#

wild flax maybe im too tired but ```ts get(name, required = false) { const option = ...

why only return the value and not the whole option

wild flax Jul 12, 2021, 11:26 PM

#

ah yeah

#

it should probably do that

copper laurel Jul 12, 2021, 11:27 PM

#

I really dont see how this aligns with a Manager

#

Otherwise yeah I think so

loud jayBOT Jul 13, 2021, 12:13 AM

#

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 Jul 13, 2021, 12:13 AM

#

lmk what changes are needed :]

#

@wild flax @vivid field @copper laurel

vivid field Jul 13, 2021, 12:15 AM

#

I'll look at it tomorrow 👍

bitter peak Jul 13, 2021, 12:27 AM

#

copper laurel why only return the value and not the whole option

yeah caught that

wild flax Jul 13, 2021, 8:09 AM

#

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

#

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

bitter peak Jul 13, 2021, 8:10 AM

#

prettier did that sorry

wild flax Jul 13, 2021, 8:10 AM

#

prettier surely didn't add newlines

#

:P

bitter peak Jul 13, 2021, 8:10 AM

#

op lemme check

bitter peak Jul 13, 2021, 8:13 AM

#

wild flax <@!321944279373185027> can you fixup the spacing in the typings file

how's that?

wild flax Jul 13, 2021, 8:14 AM

#

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 Jul 13, 2021, 8:15 AM

#

so... this?

wild flax Jul 13, 2021, 8:15 AM

#

yep yep

bitter peak Jul 13, 2021, 8:15 AM

#

kk

sacred river Jul 13, 2021, 9:20 AM

#

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

zenith oracle Jul 13, 2021, 9:34 AM

#

Iirc the API doesn't support this yet

copper laurel Jul 13, 2021, 10:56 AM

#

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

short crescent Jul 14, 2021, 3:59 AM

#

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 Jul 14, 2021, 4:09 AM

#

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 Jul 14, 2021, 4:14 AM

#

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 Jul 14, 2021, 4:19 AM

#

because it makes your code more maintainable
it makes your code more readable
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 Jul 14, 2021, 4:21 AM

#

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 Jul 14, 2021, 4:22 AM

#

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 Jul 14, 2021, 4:23 AM

#

Okay, I agree with that

neat bolt Jul 14, 2021, 4:26 AM

#

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 Jul 14, 2021, 4:26 AM

#

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 Jul 14, 2021, 4:27 AM

#

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 Jul 14, 2021, 4:30 AM

#

Thanks for the answer. Yes, definitely

outer raven Jul 14, 2021, 8:04 AM

#

Can someone tag shinotheshino please

#

Wanna ask them a question about their comment on their pr

zenith oracle Jul 14, 2021, 8:09 AM

#

@bitter peak ↑↓

outer raven Jul 14, 2021, 8:10 AM

#

Aight thanks
So what exactly is this supposed to mean? https://github.com/discordjs/discord.js/pull/6107#issuecomment-879504907
Seems like you’re agreeing with me but I’m not really sure cuz the wording is a bit weird

copper laurel Jul 14, 2021, 8:21 AM

#

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 Jul 14, 2021, 8:28 AM

#

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

bitter peak Jul 14, 2021, 11:05 AM

#

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 Jul 14, 2021, 11:07 AM

#

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 Jul 14, 2021, 11:08 AM

#

copper laurel Can you explain a bit more?

That'd be something like https://github.com/remoji-bot/remoji-bot/blob/discordjs-13/src/commands/emotes/copy.command.ts (sorry for the messy code)

outer raven Jul 14, 2021, 11:12 AM

#

bitter peak Hmmm, <@240875059953139714> <@122157285790187530> I feel like the way it is make...

You can make it optional if you really wanna support that way but I don't see why anyone would want to do that. If you can only have one subcommand at a time and you're sure there is a subcommand then why check for all of them? What advantages would that give vs just grabbing the option with the subcommand type?

bitter peak Jul 14, 2021, 11:14 AM

#

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

copper laurel Jul 14, 2021, 11:15 AM

#

bitter peak That'd be something like https://github.com/remoji-bot/remoji-bot/blob/discordjs...

I dont see why you wouldnt rather do

const subcommand = interaction.options.getSubCommand();
switch(subcommand) {
  case "single":
    break;
  case "double":
    break
}```

#

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 Jul 14, 2021, 11:16 AM

#

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 Jul 14, 2021, 11:16 AM

#

sure

#

i guess

ornate topaz Jul 14, 2021, 11:16 AM

#

that seems like a unrelated reason

copper laurel Jul 14, 2021, 11:16 AM

#

youd be putting the ifs in braces too though

#

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

ruby terrace Jul 14, 2021, 11:17 AM

#

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

copper laurel Jul 14, 2021, 11:17 AM

#

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 Jul 14, 2021, 11:18 AM

#

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

bitter peak Jul 14, 2021, 11:18 AM

#

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 Jul 14, 2021, 11:18 AM

#

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 Jul 14, 2021, 11:20 AM

#

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

copper laurel Jul 14, 2021, 11:21 AM

#

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 Jul 14, 2021, 11:22 AM

#

Stuff like this

copper laurel Jul 14, 2021, 11:23 AM

#

Kinda off-topic for this channel though

outer raven Jul 14, 2021, 11:23 AM

#

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

a /tic tac toe command oruded

ruby terrace Jul 14, 2021, 11:24 AM

#

Wow it’d help if I sent the screenshot dead

copper laurel Jul 14, 2021, 11:24 AM

#

eh, that could also be new remove-embed

#

i guess though

#

or whatever passes validation

ruby terrace Jul 14, 2021, 11:25 AM

#

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

copper laurel Jul 14, 2021, 11:25 AM

#

Off-topic, lets stop here

bitter peak Jul 14, 2021, 11:39 AM

#

Before I push, does this seem good?

#

copper laurel Jul 14, 2021, 11:41 AM

#

Im not sure why we need the getters

bitter peak Jul 14, 2021, 11:41 AM

#

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

copper laurel Jul 14, 2021, 11:42 AM

#

ahh you abstracted away options too

#

Yeah I think so

bitter peak Jul 14, 2021, 11:42 AM

#

minus the typo on line 43 oops dogekek

copper laurel Jul 14, 2021, 11:42 AM

#

ruby terrace Wow it’d help if I sent the screenshot <:dead:851908623503851550>

So coming back to this, is /buttonroles new a group with no subcommands?

bitter peak Jul 14, 2021, 11:43 AM

#

yes I think so

copper laurel Jul 14, 2021, 11:43 AM

#

and so... no options either?

ruby terrace Jul 14, 2021, 11:43 AM

#

No, it’s a subcommand

copper laurel Jul 14, 2021, 11:43 AM

#

you can have a toplevel subcommand with a toplevel group?

ruby terrace Jul 14, 2021, 11:43 AM

#

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

#

Yes, you can mix and match

copper laurel Jul 14, 2021, 11:43 AM

#

god damnit discord

ruby terrace Jul 14, 2021, 11:43 AM

#

People always get confused when I say that lmao

copper laurel Jul 14, 2021, 11:44 AM

#

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 Jul 14, 2021, 11:44 AM

#

Yeah

copper laurel Jul 14, 2021, 11:45 AM

#

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

bitter peak Jul 14, 2021, 11:45 AM

#

Hmm, that makes throwing an error in getSubCommandGroup kinda risky

copper laurel Jul 14, 2021, 11:45 AM

#

but one has null group

ruby terrace Jul 14, 2021, 11:45 AM

#

Correct

copper laurel Jul 14, 2021, 11:45 AM

#

ughghufgubfhdjgk yeah that needs required then

ruby terrace Jul 14, 2021, 11:45 AM

#

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

copper laurel Jul 14, 2021, 11:45 AM

#

to control the throw

#

Fuck this design is terrible, why Discord

ruby terrace Jul 14, 2021, 11:46 AM

#

*at the same level

bitter peak Jul 14, 2021, 11:48 AM

#

Alright so that becomes:

#

https://github.com/discordjs/discord.js/pull/6107/commits/6d4f5ad11bc8c59321c946e87af8f74ec5beca90

outer raven Jul 14, 2021, 12:02 PM

#

bitter peak https://github.com/discordjs/discord.js/pull/6107/commits/6d4f5ad11bc8c59321c946...

btw why are these functions instead of regular getters? Is it for consistency with the others?

bitter peak Jul 14, 2021, 12:02 PM

#

Yes

outer raven Jul 14, 2021, 12:03 PM

#

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 Jul 14, 2021, 12:08 PM

#

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

wild flax Jul 14, 2021, 12:42 PM

#

copper laurel I dont see why you wouldnt rather do ```js const subcommand = interaction.option...

Did we get a resolution on this?

#

https://github.com/remoji-bot/remoji-bot/blob/discordjs-13/src/commands/emotes/copy.command.ts this is terrible btw

#

Switches make for way better control flow

copper laurel Jul 14, 2021, 12:44 PM

#

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

wild flax Jul 14, 2021, 12:45 PM

#

Compared to?

#

a getter?

copper laurel Jul 14, 2021, 12:46 PM

#

Properties

#

Why getters at all

wild flax Jul 14, 2021, 12:46 PM

#

I don't know im just asking

#

Why properties

bitter peak Jul 14, 2021, 12:51 PM

#

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

outer raven Jul 14, 2021, 1:43 PM

#

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

zenith oracle Jul 14, 2021, 2:17 PM

#

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 Jul 14, 2021, 2:21 PM

#

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

zenith oracle Jul 14, 2021, 2:25 PM

#

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 Jul 14, 2021, 2:26 PM

#

Then you can construct a Permissions object manually, sure

tacit crypt Jul 14, 2021, 2:30 PM

#

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

zenith oracle Jul 14, 2021, 2:33 PM

#

vivid field Then you can construct a Permissions object manually, sure

Ok so I'll use that, thanks!

zenith oracle Jul 14, 2021, 2:33 PM

#

tacit crypt Anything prefixed with `API` is usually a type from `discord-api-types`, which r...

Yeah, I saw

outer raven Jul 14, 2021, 2:54 PM

#

@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 Jul 14, 2021, 2:54 PM

#

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

outer raven Jul 14, 2021, 2:55 PM

#

Yeah, that’s what it should be

tacit crypt Jul 14, 2021, 2:55 PM

#

neither of them should be functions nor getters

outer raven Jul 14, 2021, 2:55 PM

#

A getter/property

tacit crypt Jul 14, 2021, 2:55 PM

#

no getters??

#

why would you make it a getter

outer raven Jul 14, 2021, 2:55 PM

#

Idrk

#

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

tacit crypt Jul 14, 2021, 2:57 PM

#

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 Jul 14, 2021, 3:00 PM

#

meguFace

void rivet Jul 14, 2021, 8:37 PM

#

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 Jul 14, 2021, 9:33 PM

#

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 Jul 14, 2021, 10:15 PM

#

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 Jul 15, 2021, 12:06 AM

#

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 Jul 15, 2021, 6:37 AM

#

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 Jul 15, 2021, 6:49 AM

#

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

void rivet Jul 15, 2021, 6:49 AM

#

ruby terrace Another option is with custom caching if you don't set GUILD_MEMBERS intent for ...

I will say that I don't quite understand what you're saying here 😅
I thought the whole point of limitedCollection was that it only stores a limited amount of entries, I don't think a limitedcollection would just stop caching or keep outdated instances, which was the original issue, sorry but I would need a bit more explanation on this to understand what's going on here

void rivet Jul 15, 2021, 6:51 AM

#

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 Jul 15, 2021, 7:17 AM

#

void rivet I will say that I don't quite understand what you're saying here 😅 I thought t...

If you set the maximum of entries to 0, then it doesn't cache anything, therefore it has no outdated instances (0 also happens to be the default)

#

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

void rivet Jul 15, 2021, 7:18 AM

#

that would completely discard caching in this case, right?

ruby terrace Jul 15, 2021, 7:18 AM

#

yes

void rivet Jul 15, 2021, 7:19 AM

#

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

ruby terrace Jul 15, 2021, 7:22 AM

#

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 Jul 15, 2021, 7:22 AM

#

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 Jul 15, 2021, 7:23 AM

#

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 Jul 15, 2021, 7:26 AM

#

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

void rivet Jul 15, 2021, 7:42 AM

#

ruby terrace hmm, actually I have an idea, default force to !client.options.intents.has(['GUI...

that is sort of what's going on with the intents check further, just that this would skip the check we know wouldn't work and would allow the user to check the api
however this brings up the question why, why would a user want to get outdated data?

wild flax Jul 15, 2021, 7:44 AM

#

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 Jul 15, 2021, 7:45 AM

#

ruby terrace Another option is with custom caching if you don't set GUILD_MEMBERS intent for ...

yeah, hence this

wild flax Jul 15, 2021, 7:45 AM

#

Yeah that seems the most sensible approach

void rivet Jul 15, 2021, 7:45 AM

#

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 Jul 15, 2021, 7:53 AM

#

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

#

How could you be confident, maybe I'm just reading the code wrong, its early:
https://github.com/discordjs/discord.js/blob/master/src/client/actions/MessageReactionAdd.js#L21

#

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

#

https://github.com/discordjs/discord.js/blob/4206e35b2316431c1a009664636dcda85d39fff8/src/client/actions/Action.js#L95-L96

#

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

void rivet Jul 15, 2021, 7:56 AM

#

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

wild flax Jul 15, 2021, 7:56 AM

#

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 Jul 15, 2021, 7:58 AM

#

ruby terrace Another option is with custom caching if you don't set GUILD_MEMBERS intent for ...

yes exactly, that's what i said, and never caching would break this code
I was noting this as a side effect to this suggestion

wild flax Jul 15, 2021, 7:58 AM

#

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 Jul 15, 2021, 7:59 AM

#

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

wild flax Jul 15, 2021, 8:00 AM

#

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 Jul 15, 2021, 8:00 AM

#

re #archive-github-djs message should I copy the branch and make a new pr fixing the conflicts and adding banner color?

wild flax Jul 15, 2021, 8:00 AM

#

Yeah just do

void rivet Jul 15, 2021, 8:01 AM

#

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

wild flax Jul 15, 2021, 8:01 AM

#

I did above

#

Starting from: #archive-library-discussion message

void rivet Jul 15, 2021, 8:02 AM

#

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

wild flax Jul 15, 2021, 8:03 AM

#

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

#

and the cache will be updated

#

https://github.com/discordjs/discord.js/blob/4206e35b2316431c1a009664636dcda85d39fff8/src/managers/CachedManager.js#L33-L34

#

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

void rivet Jul 15, 2021, 8:04 AM

#

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 Jul 15, 2021, 8:05 AM

#

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 Jul 15, 2021, 8:06 AM

#

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 Jul 15, 2021, 8:06 AM

#

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 Jul 15, 2021, 8:10 AM

#

wild flax messageReactionAdd is also kind of an odd child here because its the only event ...

typing start does too iirc

ruby terrace Jul 15, 2021, 8:10 AM

#

copper laurel This has been discussed before and decided that we didn't want either of these i...

this conversation has happened before btw

wild flax Jul 15, 2021, 8:11 AM

#

oak quail typing start does too iirc

Well what I meant was rather if the problem exists even in events where we expose all the data 😅

#

But sure, preferably we should fix all of them

copper laurel Jul 15, 2021, 8:48 AM

#

wild flax If the problem exists anywhere else, I'd happy to discuss this further, but if i...

Yeah we've had this exact discussion and decided that was a crap interface

#

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

wild flax Jul 15, 2021, 8:49 AM

#

Might be worth to revisit

#

Esp with disabling cache

copper laurel Jul 15, 2021, 8:49 AM

#

Fetch is always an option anyway

wild flax Jul 15, 2021, 8:50 AM

#

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

#

We just don't expose it to you

copper laurel Jul 15, 2021, 8:50 AM

#

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

wild flax Jul 15, 2021, 8:51 AM

#

Yeah, I wouldnt wanna do user/member

void rivet Jul 15, 2021, 9:02 AM

#

wild flax I feel, if anything, we should do more educating on that behaviour, but not chan...

This is something I completely agree with, the users should know what they're doing, they should know how intents are affecting everything
stating that when you don't have the required intents, the data is outdated in the cache and you have to force if you need the latest data in this case would help

void rivet Jul 15, 2021, 9:10 AM

#

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 Jul 15, 2021, 9:11 AM

#

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

#

Since it can’t be a member in dms

void rivet Jul 15, 2021, 9:13 AM

#

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

oak quail Jul 15, 2021, 9:30 AM

#

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 Jul 15, 2021, 9:31 AM

#

You could check on the roles

#

We have a displayColor thing there I think

#

And also a hex prop

zenith oracle Jul 15, 2021, 9:32 AM

#

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

oak quail Jul 15, 2021, 9:32 AM

#

roles do the same thing embeds do ^

#

bc discord returns a number for role colors too

wild flax Jul 15, 2021, 9:32 AM

#

Then we should copy that behavior IMO

oak quail Jul 15, 2021, 9:32 AM

#

but banner colors are inconsistent for some reason

wild flax Jul 15, 2021, 9:32 AM

#

And just transform discord’s stuff

#

Or maybe ask in the docs PR

#

Fucking dumb

#

(╯°□°）╯︵ ┻━┻

oak quail Jul 15, 2021, 9:33 AM

#

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 Jul 15, 2021, 9:47 AM

#

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 Jul 15, 2021, 10:03 AM

#

oak quail so would hexColor be the property and color be the getter, or should it store th...

Hmmm

#

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 Jul 15, 2021, 10:15 AM

#

oh

#

user#fetch(true) is broken lol

#

it doesnt send an options object https://advaith.is-in-hyper.space/5f70359b41.png

#

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

raven cloak Jul 15, 2021, 10:16 AM

#

Have channel type names been made uppercase?

oak quail Jul 15, 2021, 10:16 AM

#

yes

raven cloak Jul 15, 2021, 10:16 AM

#

Why so?

#

Consistency?

wild flax Jul 15, 2021, 10:18 AM

#

https://discord.com/developers/docs/resources/channel#channel-object-channel-types

oak quail Jul 15, 2021, 10:18 AM

#

https://github.com/discordjs/discord.js/pull/6035

raven cloak Jul 15, 2021, 10:19 AM

#

wild flax <https://discord.com/developers/docs/resources/channel#channel-object-channel-ty...

Makes sense

oak quail Jul 15, 2021, 10:26 AM

#

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 Jul 15, 2021, 10:27 AM

#

Wait what

#

Why not just fetch banner that populates all of those

oak quail Jul 15, 2021, 10:28 AM

#

what should it return tho

#

just the banner hash?

wild flax Jul 15, 2021, 10:28 AM

#

void mmLol

oak quail Jul 15, 2021, 10:28 AM

#

isnt that just fetch(true) tho

wild flax Jul 15, 2021, 10:28 AM

#

I dunno, the patched user?

oak quail Jul 15, 2021, 10:28 AM

#

i mean

wild flax Jul 15, 2021, 10:29 AM

#

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

oak quail Jul 15, 2021, 10:29 AM

#

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

#

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

wild flax Jul 15, 2021, 10:29 AM

#

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

#

cc @tacit crypt @solemn oyster

tacit crypt Jul 15, 2021, 10:30 AM

#

So fetchBanner() = this.fetch()?

wild flax Jul 15, 2021, 10:30 AM

#

yeah

#

essentially

#

lol

#

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

tacit crypt Jul 15, 2021, 10:31 AM

#

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

wild flax Jul 15, 2021, 10:31 AM

#

yeah thats even better

tacit crypt Jul 15, 2021, 10:31 AM

#

What do with emojis/stickers then

oak quail Jul 15, 2021, 10:32 AM

#

what about them

#

how are they relevant 🤔

tacit crypt Jul 15, 2021, 10:32 AM

#

fetchGuild

#

wild flax Jul 15, 2021, 10:32 AM

#

well for all of those

oak quail Jul 15, 2021, 10:32 AM

#

oh no not this again

wild flax Jul 15, 2021, 10:32 AM

#

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 Jul 15, 2021, 10:38 AM

#

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

tacit crypt Jul 15, 2021, 10:53 AM

#

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 Jul 15, 2021, 10:54 AM

#

cuz it defaults to true anyways
no it doesnt?

#

afaik its always been default false

tacit crypt Jul 15, 2021, 11:00 AM

#

Not in the manager function, in the user function

oak quail Jul 15, 2021, 11:01 AM

#

false there too

#

https://advaith.is-in-hyper.space/b63d25f588.png

#

wait wtf is the point then

tacit crypt Jul 15, 2021, 11:01 AM

#

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

#

@wild flax @solemn oyster ^ thoughts

wild flax Jul 15, 2021, 11:02 AM

#

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

#

but on managers it should be fetch(false)

oak quail Jul 15, 2021, 11:03 AM

#

should i change that in the fix pr too

wild flax Jul 15, 2021, 11:07 AM

#

Sure

oak quail Jul 15, 2021, 11:10 AM

#

done

oak quail Jul 15, 2021, 11:26 AM

#

made the banner pr

raven cloak Jul 15, 2021, 1:44 PM

#

Why did presence get moved to member, rather than user

#

Even on Discord API side

#

What made that change

wild flax Jul 15, 2021, 1:48 PM

#

A presence is never on a user

raven cloak Jul 15, 2021, 1:54 PM

#

It was in v12

wild flax Jul 15, 2021, 1:55 PM

#

That was us doing it

unique axle Jul 15, 2021, 1:56 PM

#

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 Jul 15, 2021, 1:57 PM

#

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

clever spoke Jul 16, 2021, 1:00 AM

#

is there any chance djs would add undocumented features?

raven juniper Jul 16, 2021, 1:09 AM

#

no - see stuff like mobile indicator

clever spoke Jul 16, 2021, 1:23 AM

#

understandable

real jetty Jul 16, 2021, 8:04 AM

#

clever spoke is there any chance djs would add undocumented features?

From memory, pull requests are created before documentation but merge on the branch only when documented.
So I think DJs release doesn't support undocumented features

gusty carbon Jul 16, 2021, 8:07 AM

#

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

copper laurel Jul 16, 2021, 8:26 AM

#

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

rough glacier Jul 16, 2021, 8:28 AM

#

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 Jul 16, 2021, 8:29 AM

#

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

clever spoke Jul 16, 2021, 8:30 AM

#

what if we removed Message#removeAttachments

#

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

real jetty Jul 16, 2021, 8:31 AM

#

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

gusty carbon Jul 16, 2021, 8:31 AM

#

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

gusty carbon Jul 16, 2021, 8:32 AM

#

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

that just seems like more work than doing edit({components: [] })

rough glacier Jul 16, 2021, 8:37 AM

#

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 Jul 16, 2021, 8:39 AM

#

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

gusty carbon Jul 16, 2021, 8:44 AM

#

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

real jetty Jul 16, 2021, 9:02 AM

#

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

msg.removeEmbeds().removeComponents() seems better tbh

copper laurel Jul 16, 2021, 9:10 AM

#

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 Jul 16, 2021, 9:13 AM

#

copper laurel Just confirmed - the reason that exists is because you can remove the attachment...

oh well that makes sense, forgot about that

outer raven Jul 16, 2021, 10:18 AM

#

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

#

oak quail Jul 16, 2021, 10:18 AM

#

dynamic wasnt in the typings before

#

that change was copied from the old banner pr

zenith oracle Jul 16, 2021, 10:18 AM

#

Yeah, I remember someone said to add missing Typings for it too
Found this comment: https://github.com/discordjs/discord.js/pull/5930#discussion_r659213364

outer raven Jul 16, 2021, 10:19 AM

#

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

outer raven Jul 16, 2021, 11:15 AM

#

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 Jul 16, 2021, 12:13 PM

#

yeah should the default format be webp or something

#

null is more correct for those imo

outer raven Jul 16, 2021, 12:27 PM

#

copper laurel `null` is more correct for those imo

why?

wild flax Jul 16, 2021, 12:28 PM

#

they arent a collection

#

why would it matter

copper laurel Jul 16, 2021, 12:28 PM

#

they are methods, not a hashmap

outer raven Jul 16, 2021, 12:28 PM

#

because undefined is more accurate than null

copper laurel Jul 16, 2021, 12:28 PM

#

no, i dont think it is

wild flax Jul 16, 2021, 12:28 PM

#

based on what

outer raven Jul 16, 2021, 12:28 PM

#

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

#

it's not like they were specified as null

wild flax Jul 16, 2021, 12:28 PM

#

imo null is always more accurate

#

we only use undefined for collections because map does

#

to not completely break the map interface

copper laurel Jul 16, 2021, 12:29 PM

#

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

outer raven Jul 16, 2021, 12:29 PM

#

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 Jul 16, 2021, 12:32 PM

#

?

solemn oyster Jul 16, 2021, 12:36 PM

#

?

#

We use undefined only when exclusively needed

bitter peak Jul 16, 2021, 12:37 PM

#

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 Jul 16, 2021, 12:37 PM

#

Such as skipping fields from JSON.stringify

tacit crypt Jul 16, 2021, 12:42 PM

#

Why are subCommand/subCommandGroups not... properties meguFace

copper laurel Jul 16, 2021, 12:43 PM

#

they are. private properties lol

tacit crypt Jul 16, 2021, 12:43 PM

#

Not what I asked

#

They shouldn't be glorified getters

copper laurel Jul 16, 2021, 12:44 PM

#

oh I know

wild flax Jul 16, 2021, 12:44 PM

#

I dont see why not

copper laurel Jul 16, 2021, 12:44 PM

#

I dont know why theyre private and not just properties

outer raven Jul 16, 2021, 1:01 PM

#

bitter peak Null is more of an explicit empty type whereas undefined is an implicit one. It'...

ok so imagine you have a function with an optional parameter and you assign one of the command's options to that parameter
if the option is not required it will throw an error because you can't assign null to whatever type your parameter has
whereas with the previous .get you could and both work perfectly fine, it's just a typescript error

wild flax Jul 16, 2021, 1:02 PM

#

thats more a you problem

#

null can be a default value for optional params

outer raven Jul 16, 2021, 1:04 PM

#

yeah but why do this

function example(param: string | null) {}

when you can just do this

function example(param?: string) {}

wild flax Jul 16, 2021, 1:04 PM

#

bruh

copper laurel Jul 16, 2021, 1:04 PM

#

we.... dont

outer raven Jul 16, 2021, 1:04 PM

#

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

outer raven Jul 16, 2021, 1:05 PM

#

copper laurel we.... dont

wdym?

copper laurel Jul 16, 2021, 1:05 PM

#

dont write shit functions

#

dont pass variables you havent checked to functions

wild flax Jul 16, 2021, 1:05 PM

#

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 Jul 16, 2021, 1:06 PM

#

yeah but the | null there is redundant

wild flax Jul 16, 2021, 1:06 PM

#

no it isnt

#

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

outer raven Jul 16, 2021, 1:06 PM

#

wild flax since not everything returns undefined

well if you're only passing command options it should, and that's what happens atm

outer raven Jul 16, 2021, 1:06 PM

#

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

I know but allowing the null type or adding a ? essentially means the same thing but for different types

wild flax Jul 16, 2021, 1:07 PM

#

it doesnt mean the same thing

#

undefined is not null

outer raven Jul 16, 2021, 1:07 PM

#

then what's the difference

wild flax Jul 16, 2021, 1:07 PM

#

learn js thats the difference

outer raven Jul 16, 2021, 1:07 PM

#

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

copper laurel Jul 16, 2021, 1:07 PM

#

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 Jul 16, 2021, 1:09 PM

#

copper laurel `undefined` is not the lack of a value. Its the lack of any definition at all, a...

well but when you leave an optional parameter out wouldn't that mean it's not defined? Why are you giving an option a value that it doesn't even have? You can even try to get an option that doesn't exist on your command and it will return null which doesn't make sense to me because the option isn't there to begin with

copper laurel Jul 16, 2021, 1:09 PM

#

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 Jul 16, 2021, 1:12 PM

#

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 Jul 16, 2021, 1:13 PM

#

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

#

Also this is a nice read for you too: https://devblogs.microsoft.com/typescript/announcing-typescript-4-4-beta/#exact-optional-property-types

outer raven Jul 16, 2021, 1:16 PM

#

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

wild flax Jul 16, 2021, 1:18 PM

#

lol

#

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

bitter peak Jul 16, 2021, 2:12 PM

#

@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 Jul 16, 2021, 2:12 PM

#

??

outer raven Jul 16, 2021, 2:12 PM

#

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

bitter peak Jul 16, 2021, 2:13 PM

#

The same could be said for all the get* methods

#

But the rest of the PR follows that assertion logic

outer raven Jul 16, 2021, 2:13 PM

#

not really cuz there are multiple options and only one subcommand and group

tacit crypt Jul 16, 2021, 2:13 PM

#

How is that moving the burden of checking from the user, when it now makes it twice as annoying because it throws an error

bitter peak Jul 16, 2021, 2:13 PM

#

It still serves to validate the user's command data against their runtime code

#

If anything, I think it should be a getter

#

If not a function

outer raven Jul 16, 2021, 2:14 PM

#

i don't see any advantage in that

tacit crypt Jul 16, 2021, 2:14 PM

#

it should be a property

#

and let users check themselves

#

there's no reason for it to be a glorified getter

bitter peak Jul 16, 2021, 2:15 PM

#

But none of the other methods do that, for the sole reason of ensuring the user doesn't have mismatched command definitions and code

tacit crypt Jul 16, 2021, 2:15 PM

#

the other options are specifically of a type (roles, users, channels, etc)

#

subcommands and subcommand groups are strings and nothing more

#

All you do right now is throw an INCONSISTENT error when someone gets the subcommand / subcommand group

wild flax Jul 16, 2021, 2:16 PM

#

I think you guys make this out to be a way bigger deal than it is

#

lol

tacit crypt Jul 16, 2021, 2:16 PM

#

??

wild flax Jul 16, 2021, 2:17 PM

#

the whole point of us transforming the options is so the user doesnt need to check at all

#

so I agree with shino there

#

otherwise you can just use the get() method

tacit crypt Jul 16, 2021, 2:17 PM

#

I am strongly disagreeing with glorified getters for sub command (group) but shrug

wild flax Jul 16, 2021, 2:17 PM

#

which does none of this

#

Yeah I don't think its too big of a deal

tacit crypt Jul 16, 2021, 2:19 PM

#

I don't see how that's better than just having users check if interaction.options.subCommand === 'name'

#

Especially since I bet you there will be someone doing if/else chains of getSubCommand() instead of caching the variable

wild flax Jul 16, 2021, 2:20 PM

#

I don't think we ever tried to pre-optimize what users end up doing

#

So that is out of scope for this and rather belongs into the guide or a programming guide in general

#

Calling a function that returns the same value over and over, and how that isnt optimal, is not something we need to teach people.

gaunt lotus Jul 16, 2021, 9:52 PM

#

Not sure if this is where I should be asking this but I can't seem to import SlashCommandBuilder, I have:
import { SlashCommandBuilder } from '@discordjs/builders';

Says Module '"@discordjs/builders"' has no exported member 'SlashCommandBuilder'. I have @discordjs/builders@0.2.0 installed

wild flax Jul 16, 2021, 9:57 PM

#

Its not yet published

#

Ill let you know when it is

gaunt lotus Jul 16, 2021, 9:59 PM

#

Oh ok my bad, thank you

oak quail Jul 17, 2021, 2:41 AM

#

hm

#

for my stickers pr should i actually make it work for both stickers and sticker_items

#

so it works for old messages and new messages

#

stickers is technically deprecated but its the only way of seeing what stickers old msgs have

oak quail Jul 17, 2021, 10:44 AM

#

also, couldnt the Base class implement createdAt and createdTimestamp? since it says its for data models that have snowflakes (although looks like some non-snowflake objects incorrectly extend it)

warped crater Jul 17, 2021, 10:52 AM

#

yeah thats primarily the reason why it doesnt annoyingly enough

#

i had a look at it once but its fairly obnoxious to refactor

slate nacelle Jul 17, 2021, 10:53 AM

#

We could also update the docstring to reflect what it is.

wild flax Jul 17, 2021, 11:34 AM

#

oak quail `stickers` is technically deprecated but its the only way of seeing what sticker...

not sure if worth

#

is it deprecated in v9 still?

bitter peak Jul 17, 2021, 4:45 PM

#

Maybe Base can be refactored into Base and SnowflakeBase extends Base?

zenith oracle Jul 17, 2021, 5:17 PM

#

Why?

tacit crypt Jul 17, 2021, 5:38 PM

#

@wild flax @bitter peak why don't we make getSubCommandGroup do the exact same as getSubCommand? (aka throw if missing, return otherwise)?

bitter peak Jul 17, 2021, 6:31 PM

#

I guess yeah that does make sense. What do you think, Crawl?

bitter peak Jul 17, 2021, 8:11 PM

#

@wild flax

wild flax Jul 17, 2021, 8:13 PM

#

sure

#

next time make sure to ping me directly instead of waiting :P

bitter peak Jul 17, 2021, 8:43 PM

#

tacit crypt <@!81440962496172032> <@!321944279373185027> why don't we make getSubCommandGrou...

changed :)

oak quail Jul 17, 2021, 11:18 PM

#

wild flax is it deprecated in v9 still?

yeah, the client renders from both

wild flax Jul 17, 2021, 11:19 PM

#

man fuck discord

ruby terrace Jul 17, 2021, 11:22 PM

#

aj did say no more breaking changes for v9 right

oak quail Jul 17, 2021, 11:29 PM

#

i think someone said theyre removing guild.region in v9 but that doesnt sound right

#

mason #696891054355513374 message

#

the client still uses it tho bloblul

dry valve Jul 17, 2021, 11:40 PM

#

there doesn't seem to be any documentation about how context works in broadcastEval, I've spent the past hour searching and the only thing I've found is where the context should be placed, but not how to access it

#

passing a second argument seems to be the solution to this, but why is it not documented?

analog oyster Jul 17, 2021, 11:53 PM

#

here are a few oddities with GuildInviteManager

#cache is typed as Collection<Snowflake, Invite>, while it is actually Collection<string, Invite>
it has #resolveId (should perhaps be called resolveCode?), and it just returns back whatever you pass at it as long as you pass a string
- e.g. if you pass discord.gg/djs, it returns discord.gg/djs, if you pass djs, it returns djs
#resolve should support invite urls as suggested by InviteResolvable, but it doesn't. it only supports invite codes

gaunt lotus Jul 18, 2021, 12:14 AM

#

Why do I need the MANAGE_ROLES permission to edit the permissionOverwrites of a channel when I can create a channel with initial permissionOverwrites without it?

frank turret Jul 18, 2021, 12:30 AM

#

analog oyster here are a few oddities with GuildInviteManager - `#cache` is typed as `Collect...

A Snowflake is (correction: like) a string, so that's not totally inaccurate.
resolveId is inherited from DataManager. I suspect it's meant to be overloaded in situations that warrant it, otherwise it's just a base method that's inherited. And the behavior you describe looks to be intentional.
👀

tacit crypt Jul 18, 2021, 12:38 AM

#

frank turret 1. A Snowflake is (correction: *like*) a string, so that's not totally inaccurat...

Not really. A snowflake is a stringified bigint

frank turret Jul 18, 2021, 12:39 AM

#

noted, the second part of the point still stands

loud jayBOT Jul 18, 2021, 9:44 AM

#

pr_open #6084 in discordjs/discord.js by Bluenix2 opened <t:1625850843:R> (review required)
refactor(InteractionCollector): only keep Ids of objects
📥 npm i Bluenix2/discord.js#patch-2

upper moss Jul 18, 2021, 9:46 AM

#

isnt this pr a major semver as it renames some of the properties

analog oyster Jul 18, 2021, 9:48 AM

#

yes, but technically interactions haven't been released yet so shouldn't it be semver: N/A instead

tacit crypt Jul 18, 2021, 11:16 AM

#

it's still semver major i think meguFace

gusty carbon Jul 18, 2021, 11:20 AM

#

its labeled as semver minor tho

oak quail Jul 18, 2021, 11:27 AM

#

oak quail yeah, the client renders from both

so is there a decision?

tacit crypt Jul 18, 2021, 11:27 AM

#

oak quail so is there a decision?

can you find out if they'll run a migration on those messages?

oak quail Jul 18, 2021, 11:28 AM

#

they probably won't but I can ask

tacit crypt Jul 18, 2021, 11:28 AM

#

They should

#

So please do ask 🙏

ruby terrace Jul 18, 2021, 5:53 PM

#

I would guess they won't, based on the fact that old crossposts no longer return guild_id which is supposed to be guaranteed

rough glacier Jul 18, 2021, 8:37 PM

#

current CommandInteractionOptionResolver implementation leaves subcommand options as a Collection and a tiny bit fuzzy to access them. Shouldn't they be adapted similarly?

bitter peak Jul 18, 2021, 8:59 PM

#

rough glacier current `CommandInteractionOptionResolver` implementation leaves subcommand opti...

What do you mean?

#

getSubCommand() is just a getter for the name, which is resolved at construction time

junior pumice Jul 18, 2021, 9:04 PM

#

I've got an idea:
What if you could insert a filter function in the cachefactory to tell it that it should only cache what matches the filter

So (c) => c.isText() to only cache text based channels

short crescent Jul 18, 2021, 9:49 PM

#

In the file MessagePayload method makeContent makes the content an empty line only if the content was STRONGLY null. I assume you need to check for undefined as well.

Line: 110
https://www.github.com/discordjs/discord.js/tree/master/src%2Fstructures%2FMessagePayload.js

copper laurel Jul 18, 2021, 9:52 PM

#

short crescent In the file MessagePayload method makeContent makes the content an empty line on...

Definitely not - you want to be able to edit things that arent content without changing content

#

message.edit({ embeds: [embed] }) for example - you dont want this to change the content (which is undefined)

#

null is the correct way to specify something as intentionally having no value, hence editing out the content. Undefined is only the lack of its presence in the object

rough glacier Jul 18, 2021, 10:11 PM

#

bitter peak What do you mean?

well right now you can only access subcommand options as <interaction>.options.get(subcommandName).options if im not mistaken, and you get Collection<Snowflake, CommandInteractionOptionData[]> instead of another CommandInteractionOptionResolver. Maybe an interaction.options.getSubCommandOptions() is too much, but different types between first and second options layer is weird

bitter peak Jul 18, 2021, 10:16 PM

#

rough glacier well right now you can only access subcommand options as `<interaction>.options....

That's intended.
CommandInteractionOptionResolver#get() is intended to return unparsed option data. It's up to you to parse the subcommand options if you do it that way. The preferred way to use it is like this:

declare const interaction: CommandInteraction;

const subcommand = interaction.options.getSubCommand();
switch (subcommand) {
  case 'foo': {//   /command foo
    await interaction.reply('foo!');
    break;
  }
  case 'bar': {//   /command bar <baz:string>
    const baz = interaction.options.getString('baz', true);
    await interaction.reply(`baz: ${baz.toUpperCase()}`);
    break;
  }
  default: throw new Error(`Unknown subcommand: ${subcommand}`);
}

#

CommandInteractionOptionResolver#getSubCommand() returns the subcommand name as a string

vivid field Jul 18, 2021, 10:18 PM

#

Looks like you forgot to update https://github.com/discordjs/discord.js/blob/master/typings/index.d.ts#L3027 though because all the collection stuff was refactored back to arrays

copper laurel Jul 18, 2021, 10:19 PM

#

junior pumice I've got an idea: What if you could insert a filter function in the cachefactory...

This is totally up to you to do, since you can provide any cache class you like (as long as it implements the map-like interface) you can extend Collection and have it filter on add

rough glacier Jul 18, 2021, 10:21 PM

#

vivid field Looks like you forgot to update <https://github.com/discordjs/discord.js/blob/ma...

⤴️ this. first option layer is an OptionResolver, but second (subcommand options) are still a collection. They should match

bitter peak Jul 18, 2021, 10:22 PM

#

rough glacier ⤴️ this. first option layer is an OptionResolver, but second (subcommand options...

No, it should actually be CommandInteractionOption[]

#

Since the purpose of get() is to return "raw" data

bitter peak Jul 18, 2021, 10:22 PM

#

vivid field Looks like you forgot to update <https://github.com/discordjs/discord.js/blob/ma...

I'll open a PR to fix that this evening

vivid field Jul 18, 2021, 10:22 PM

#

\👍

junior pumice Jul 19, 2021, 4:02 AM

#

copper laurel This is totally up to you to do, since you can provide any cache class you like ...

So in case of me somehow managing to do it, could i do a PR?

copper laurel Jul 19, 2021, 4:03 AM

#

No I mean, it doesnt need a PR

junior pumice Jul 19, 2021, 4:03 AM

#

Uh

#

Oh hmmmm

#

Can I?

#

I thought the cache classes are just object names

copper laurel Jul 19, 2021, 4:05 AM

#

const FilteredCollection extends Collection {
  set(key, value) {
    if(filter) {
      super.set(key, value);
    } else {
      // idk, something
    }
  }
}

const client = new Client({
  makeCache: (manager) => {
    if (manager.name === 'ManagerToFilter') return new FilteredCollection(filter);
  }
});```

#

Thats a really rough example of what you could do

junior pumice Jul 19, 2021, 4:07 AM

#

Hmmm

#

I thought about something like this

[...].makeCache({
MessagesManager: {limit: 200, (f) => !f.author.bot},
EmojiManager: {limit: 1000, (f) => f.animated}
})```

#

Idk the exact names

mighty matrix Jul 19, 2021, 5:46 AM

#

Someone with perms should probably review this issue, the comments are crazy https://github.com/discordjs/discord.js/issues/6136

bitter peak Jul 19, 2021, 6:25 AM

#

wtf

copper laurel Jul 19, 2021, 6:28 AM

#

Nobody online can right now

real jetty Jul 19, 2021, 8:48 AM

#

I don't know where to ask this but the property fetchReply for reply options isn't in the documentation. Is this normal?
https://discord.js.org/#/docs/main/master/class/CommandInteraction?scrollTo=reply

zenith oracle Jul 19, 2021, 8:49 AM

#

https://discord.js.org/#/docs/main/master/class/CommandInteraction?scrollTo=fetchReply

#

Wdym?

real jetty Jul 19, 2021, 8:53 AM

#

zenith oracle Wdym?

In reply options you have a property named fetchReply to fetch the reply immediately :

// First way
const reply = interaction.reply({ content: "test", fetchReply: true })

// Second way
interaction.reply({ content: "test" })
const reply = await interaction.fetchReply()

#

zenith oracle Jul 19, 2021, 8:54 AM

#

Ah yeah, you're right, this option doesn't seem to be in the docs

bitter peak Jul 19, 2021, 10:14 AM

#

regarding #6143, I'm not too sure what a good solution there would be; interaction.options.options definitely doesn't sound too great. While their issue could be solved with a loop/helper function in their code, it seems like a use-case that would be useful to support. Maybe CommandInteraction#optionsArray: CommandInteractionOption[]?

real jetty Jul 19, 2021, 11:50 AM

#

Another thing that can be cool is to add the same thing for ClientApplication that for Client, something like :

type If<T extends boolean, A, B = null> = T extends true ? A : T extends false ? B : A | B;

export class ClientApplication<Fetched extends boolean = boolean> extends Application {
  ...
  public owner: If<Fetched, User | Team>;
  public isFetched(): this is ClientApplication<true>
  ...
}

bitter peak Jul 19, 2021, 5:43 PM

#

@wild flax @tacit crypt thoughts on the issue above ^?

loud jayBOT Jul 19, 2021, 5:43 PM

#

issue_open #6143 in discordjs/discord.js by Jiralite opened <t:1626688842:R>
Have stored values available for access for CommandInteractionOptionResolver

gaunt lotus Jul 19, 2021, 5:56 PM

#

Would it be possible to have getSubCommandGroup() return null/undefined instead of throwing an error when the command issued doesn't have a sub command group? When checking if a received command interaction has one I would do:

if(<CommandInteractionOptionResolver>.getSubCommandGroup()) {
    (...)
}

instead of something like:

let subCommandGroup: string;
try {
    subCommandGroup = <CommandInteractionOptionResolver>.getSubCommandGroup();
}
catch {}

if(subCommandGroup) {
    (...)
}

I'm using this to run certain code for specific sub command groups

wild flax Jul 19, 2021, 5:59 PM

#

That isn’t really the intended use case for it

#

Also there’s always the generic get method that doesn’t throw

zenith oracle Jul 19, 2021, 6:05 PM

#

I think the option resolver should have a public collection property, so that users can still use their own ways to handle options, like the issue mentioned above. What if I want to filter or map the options? I actually can't. It's like a collection with a really advanced get methods...

gaunt lotus Jul 19, 2021, 6:06 PM

#

wild flax Also there’s always the generic get method that doesn’t throw

This went over me, thank you

tacit crypt Jul 19, 2021, 6:30 PM

#

bitter peak <@81440962496172032> <@139836912335716352> thoughts on the issue above ^?

which issue, sorry I'm like blind

loud jayBOT Jul 19, 2021, 6:31 PM

#

issue_open #6143 in discordjs/discord.js by Jiralite opened <t:1626688842:R>
Have stored values available for access for CommandInteractionOptionResolver

bitter peak Jul 19, 2021, 6:31 PM

#

@tacit crypt

tacit crypt Jul 19, 2021, 6:32 PM

#

Could just turn _options into a Collection like before

#

wait, right, options.options

#

uhhhh

ruby terrace Jul 19, 2021, 6:33 PM

#

.raw?

opaque vessel Jul 19, 2021, 6:33 PM

#

Might I just suggest .data or .raw?

#

.data as in, the data supplied to us
Or .raw if you really wanna go that route!

bitter peak Jul 19, 2021, 6:33 PM

#

I was thinking like interaction.optionsArray

tacit crypt Jul 19, 2021, 6:34 PM

#

It's not really raw tho

#

It's already processed by us

ruby terrace Jul 19, 2021, 6:35 PM

#

eh, true

real jetty Jul 19, 2021, 6:48 PM

#

Or instead name it like a property we could name it like a function / getter :
interaction.options.toCollection -> return _options into a Collection like before
interaction.options.toArray -> return _options into an Array like now

We will avoid interaction.options._options and give the possibility to DJs user to use what they want

zenith oracle Jul 19, 2021, 6:51 PM

#

I agree, it would be really useful

bitter peak Jul 19, 2021, 6:56 PM

#

I like the idea of toArray() since we could return a copy of the array

opaque vessel Jul 19, 2021, 6:59 PM

#

I also wanna suggest a rename of the methods? Right now, they are .get(), .getBoolean(), .getChannel() etc. but imo this feels kinda weird since discord.js relies a lot on Collections and using .get() returns undefined but .get() here returns null. I know they're not the same but it's still... weird to me.

What about renaming the methods to.resolve(), .resolveBoolean(), .resolveChannel() etc.? The managers across discord.js use .resolve() and also return null which is what the current methods do on CommandInteractionOptionResolver. Also there's "resolver" in the name so it kinda matches up.

Thoughts?

wild flax Jul 19, 2021, 7:03 PM

#

Not a friend

#

It’s not a collection

ornate topaz Jul 19, 2021, 7:38 PM

#

it's the Collections that are odd ones here and return undefined

#

and it's like that because Collections extend Maps by adding Array methods, and both Maps and Arrays return undefined. We just mimic those in Collections

wild flax Jul 19, 2021, 9:02 PM

#

Right ok theres one issue @bitter peak

bitter peak Jul 19, 2021, 9:02 PM

#

👀

wild flax Jul 19, 2021, 9:03 PM

#

Def need a way to access all the options before they get transformed or anything

#

I used to do something along the lines of const args = [...interaction.options.values()];

#

which is obviously not possible right now

#

I can do _options for now I guess

#

But since its private, it will obv throw in TS

bitter peak Jul 19, 2021, 9:04 PM

#

okay, which do you prefer:

interaction.options.toArray(): CommandInteractionOption[]
interaction.optionsArray: ReadonlyArray<CommandInteractionOption>

#

or?

wild flax Jul 19, 2021, 9:04 PM

#

just make options public

#

probably

bitter peak Jul 19, 2021, 9:04 PM

#

but that looks bad

#

interaction.options.options

#

disgust

wild flax Jul 19, 2021, 9:04 PM

#

call it data then

#

and we are good

bitter peak Jul 19, 2021, 9:05 PM

#

wild flax But since its private, it will obv throw in TS

interaction.options['_options'] won't throw

wild flax Jul 19, 2021, 9:05 PM

#

yeah but you know what im trying to say lol

bitter peak Jul 19, 2021, 9:05 PM

#

ye ye

wild flax Jul 19, 2021, 9:05 PM

#

theres just no real way yet

#

id be fine with interaction.options.data

bitter peak Jul 19, 2021, 9:05 PM

#

alright I'll add that as a ReadonlyArray

wild flax Jul 19, 2021, 9:05 PM

#

since data will probably be our key moving forward for raw data on structures

real jetty Jul 19, 2021, 9:42 PM

#

In DataManager class we have this :

export abstract class DataManager<K, Holds, R> extends BaseManager {
  public constructor(client: Client, holds: Constructable<Holds>);
  public readonly holds: Constructable<Holds>;
  public readonly cache: Collection<K, Holds>;
  public resolve(resolvable: Holds): Holds;
  public resolve(resolvable: R): Holds | null;
  public resolveId(resolvable: Holds): K;
  public resolveId(resolvable: R): K | null;
  public valueOf(): Collection<K, Holds>;
}

But when you pass a snowflake to resolveId (for example with ChannelManager), TS consider that you are on this function: public resolveId(resolvable: R): K | null; instead of public resolveId(resolvable: Holds): K;... so TS consider that the result can be null. So I wonder if it would be possible to change the typing to include the snowflake type in the first function like this : public resolveId(resolvable: Holds | K): K; so resolveId(<Snowflake>) return Snowflake and not Snowflake | null

solemn oyster Jul 19, 2021, 9:43 PM

#

It should be changed tots public resolveId(resolvable: K | Holds): K; public resolveId(resolvable: R): K | null;
Although we'll have to check for the extended managers, specifically emoji ones

real jetty Jul 19, 2021, 9:44 PM

#

solemn oyster It should be changed to```ts public resolveId(resolvable: K | Holds): K; pub...

Yes, that's how I saw it. But I don't have any vision on the internal code (didn't take the time to look at it) so I propose it like that to know if it's possible 🙂

oak quail Jul 19, 2021, 10:33 PM

#

@tacit crypt they said to use (data.sticker_items ?? data.stickers)

tacit crypt Jul 19, 2021, 10:33 PM

#

CC @wild flax

#

also Bruh

wild flax Jul 19, 2021, 10:33 PM

#

for real?

#

wait

#

but it isnt backwards compatible is it?

oak quail Jul 19, 2021, 10:34 PM

#

it is

wild flax Jul 19, 2021, 10:34 PM

#

so you dont even need to change anything but do that?

oak quail Jul 19, 2021, 10:34 PM

#

sticker_items has partial sticker objects

#

yeah

wild flax Jul 19, 2021, 10:34 PM

#

then sure

#

@oak quail did you add the new error codes?

oak quail Jul 19, 2021, 10:44 PM

#

doing rn

wild flax Jul 19, 2021, 10:44 PM

#

👍

oak quail Jul 19, 2021, 10:44 PM

#

and i'll do the docs link thing

wild flax Jul 19, 2021, 11:14 PM

#

@oak quail youll need to like rebase or something

#

the lock file is out of sync

#

actually hold on

oak quail Jul 19, 2021, 11:15 PM

#

just merge https://github.com/discordjs/discord.js/pull/6147 and i'll merge it in

wild flax Jul 19, 2021, 11:15 PM

#

yeah

oak quail Jul 19, 2021, 11:15 PM

#

i dont have the new npm ver

wild flax Jul 19, 2021, 11:15 PM

#

I should have thought first

#

done

oak quail Jul 19, 2021, 11:18 PM

#

🎉

wild flax Jul 19, 2021, 11:20 PM

#

really close to release now

real jetty Jul 20, 2021, 8:50 AM

#

solemn oyster It should be changed to```ts public resolveId(resolvable: K | Holds): K; pub...

Although we'll have to check for the extended managers, specifically emoji ones
I just check all managers and I didn't see any problem with this modification. I'll look into making a pull request with the change (I've never made one) if no one sees any problems that this could cause 🙂 If someone wants to do the PR for me, don't be shy 👍

oak quail Jul 20, 2021, 9:11 AM

#

why are deferred, ephemeral, replied, and webhook on CommandInteraction and MessageComponentInteraction instead of on Interaction 🤔

copper laurel Jul 20, 2021, 9:16 AM

#

Future proofing, there might be a type of interaction that doesn't use those response types

loud jayBOT Jul 20, 2021, 9:27 AM

#

pr_open #6152 in discordjs/discord.js by Apokalypt opened <t:1626773214:R> (review required)
types(DataManager): add 'K' to type parameter of 'resolveId'
📥 npm i Apokalypt/discord.js-1#master

real jetty Jul 20, 2021, 9:28 AM

#

I hope I didn't make a mistake and did it right XD

winged dust Jul 20, 2021, 11:47 AM

#

#archive-voice from v0.5.1 to 0.5.4 newState.channelId changed to newState.channelID (Capital D) which is the opposite of what happened to discord V13. Is this right?

tacit crypt Jul 20, 2021, 11:55 AM

#

Boop @fringe temple

fringe temple Jul 20, 2021, 11:57 AM

#

winged dust <#852128888128929802> from v0.5.1 to 0.5.4 `newState.channelId` changed to `newS...

can you show me where this is?

#

In JoinConfig, everything is cased as Id

real jetty Jul 20, 2021, 12:04 PM

#

In Sticker.js we have :

  async fetchUser() {
    if (this.partial) await this.fetch();
    if (!this.guildID) throw new Error('NOT_GUILD_STICKER');

    const data = await this.client.api.guilds(this.guildId).stickers(this.id).get();
    this._patch(data);
    return this.user;
  }

!this.guildID will always be true because the variable has got renamed to guildId no ?

winged dust Jul 20, 2021, 12:06 PM

#

fringe temple can you show me where this is?

I just left my computer. But I believe if you do client.on(“voiceStateUpdate… and console.log(newState.channelID) you’ll see

fringe temple Jul 20, 2021, 12:06 PM

#

That's unrelated to the new voice library, @tacit crypt ^

tacit crypt Jul 20, 2021, 12:07 PM

#

ah

wild flax Jul 20, 2021, 12:07 PM

#

real jetty In Sticker.js we have : ```Javascript async fetchUser() { if (this.partial...

Ye, issa bug

real jetty Jul 20, 2021, 12:07 PM

#

wild flax Ye, issa bug

I can make a PR in few minutes to patch this little bug if you want

wild flax Jul 20, 2021, 12:08 PM

#

Sure

loud jayBOT Jul 20, 2021, 12:17 PM

#

pr_open #155 in discordjs/voice by SuperchupuDev opened <t:1626783015:R> (review required)
fix(Examples): change guildID to guildId
📥 npm i SuperchupuDev/voice#main

honest barn Jul 20, 2021, 12:17 PM

#

my first pr here, tell me if i have to change anything

winged dust Jul 20, 2021, 12:23 PM

#

fringe temple That's unrelated to the new voice library, <@139836912335716352> ^

So djs issue? Or no issue at all?

real jetty Jul 20, 2021, 12:26 PM

#

loud jay <:pr_open:852715356622553088> [#155 in discordjs/voice](<https://github.com/disc...

Maybe we should take advantage of this PR to change the other xID to xId (e.g. interface ConnectionOptions) in the voice lib ??

honest barn Jul 20, 2021, 12:27 PM

#

just found it, it's just one file so i can change that quickly

#

one sec

#

nvm its better to just make a new pr

honest barn Jul 20, 2021, 12:34 PM

#

real jetty Maybe we should take advantage of this PR to change the other xID to xId (e.g. i...

i have the code ready, do i make pr

fringe temple Jul 20, 2021, 12:55 PM

#

@honest barn if there are other places in the voice repo to make this change, feel free to do it in the same PR 🙂

#

i will review it later today

fringe temple Jul 20, 2021, 12:55 PM

#

winged dust So djs issue? Or no issue at all?

when you posted about it, was it referring to the code in the examples?

honest barn Jul 20, 2021, 12:55 PM

#

i searched for id on the repo to see any mentions of ID

#

the only one was in Networking.js other than in the examples

honest barn Jul 20, 2021, 12:57 PM

#

honest barn i have the code ready, do i make pr

and this has the changes of Networking.js

loud jayBOT Jul 20, 2021, 1:02 PM

#

pr_open #156 in discordjs/voice by SuperchupuDev opened <t:1626786134:R> (review required)
refactor(ConnectionOptions): change xID to xId
📥 npm i SuperchupuDev/voice#refactor-connectionoptions

bitter peak Jul 20, 2021, 1:19 PM

#

Will ReadonlyArray<T> work in a JSDoc comment?

diff --git a/src/structures/CommandInteractionOptionResolver.js b/src/structures/CommandInteractionOptionResolver.js
index ca0b1587..7d93b6cd 100644
--- a/src/structures/CommandInteractionOptionResolver.js
+++ b/src/structures/CommandInteractionOptionResolver.js
@@ -45,6 +45,15 @@ class CommandInteractionOptionResolver {
     }
   }
 
+  /**
+   * The array of `CommandInteractionOptions` managed by this resolver.
+   * @type {ReadonlyArray<CommandInteractionOption>}
+   * @readonly
+   */
+  get data() {
+    return this._options;
+  }
+
   /**
    * Gets an option by its name.
    * @param {string} name The name of the option.

#

And, should this be return this._options.slice()? Or assigned in the constructor?

zenith oracle Jul 20, 2021, 1:23 PM

#

Why readonlyarray?

#

You already have the readonly tag

winged dust Jul 20, 2021, 1:25 PM

#

fringe temple when you posted about it, was it referring to the code in the examples?

No. I did npm update and ran my bot. Then I saw it didn’t work and found out newState.channelId had changed to .xID

strange igloo Jul 20, 2021, 1:36 PM

#

winged dust No. I did `npm update` and ran my bot. Then I saw it didn’t work and found out n...

Don't use npm update for discord.js@dev, npm seems to go crazy when getting into parsing dev version (even tho this was supposed to be fixed??)
Use npm r discord.js then npm i discord.js@dev

#

But right now the property is channelId
https://github.com/discordjs/discord.js/blob/master/src/structures/VoiceState.js#L69

honest barn Jul 20, 2021, 1:44 PM

#

on my last pr in voice this workflow failed, do i need to change anything?

strange igloo Jul 20, 2021, 1:46 PM

#

honest barn on my last pr in voice this workflow failed, do i need to change anything?

You can look at what went wrong with the Details

src/VoiceConnection.ts:337:5 - error TS2345: Argument of type '{ endpoint: string; serverID: `${bigint}`; token: string; sessionID: string; userID: `${bigint}`; }' is not assignable to parameter of type 'ConnectionOptions'.
  Object literal may only specify known properties, but 'serverID' does not exist in type 'ConnectionOptions'. Did you mean to write 'serverId'?

337     serverID: server.guild_id,

honest barn Jul 20, 2021, 1:47 PM

#

oh im on mobile i didnt see it

#

~~ill fix it later~~ fixed

winged dust Jul 20, 2021, 1:50 PM

#

strange igloo Don't use `npm update` for discord.js@dev, npm seems to go crazy when getting in...

yep, that fixed the issue. I also wasn't recieving messages, weird

bitter peak Jul 20, 2021, 2:42 PM

#

zenith oracle Why readonlyarray?

ReadonlyArray doesn't have .push(), .pop(), etc.

bitter peak Jul 20, 2021, 3:10 PM

#

bitter peak And, should this be `return this._options.slice()`? Or assigned in the construct...

@wild flax these two messages ^

zenith oracle Jul 20, 2021, 4:36 PM

#

What if we clone the array instead?

#

Like return [...this._options]

honest barn Jul 20, 2021, 4:36 PM

#

honest barn ~~ill fix it later~~ fixed

gave error on a different file, gonna fix

#

fixed

#

epic

zenith oracle Jul 20, 2021, 4:40 PM

#

👍