#archive-library-discussion

25085 messages · Page 15 of 26

opaque vessel
outer raven

ah alright, should probably link to that then right?

opaque vessel

I suppose there's no harm :shrugs:

outer raven

we can't choose UNKNOWN for channelTypes can we 🤔

warped crater

no, the library uses it when a new channel type is introduced and it doesn't have support for it, iirc.

I believe that's a rather new addition (uh, relatively speaking. probs about ~12.x) when people realized that "oh shit, stage (or was it the game thingy channels?) channels cause big bad crash"

outer raven

UNKNOWN was here before stage channels but yeah, ill make a PR to remove that type

opaque vessel
unique axle

unknown should be any new channel type that may be introduced but isn't yet supported, it should be the default, if the payload type does not match any known type. unsure why you'd want to remove that

opaque vessel

I'm getting this strange error where TypeScript won't compile when I'm using a channel type within a subcommand?

<Client>.application!.commands.create({
  name: "test",
  description: "test",
  options: [
    {
      type: Constants.ApplicationCommandOptionTypes.SUB_COMMAND,
      name: "test",
      description: "test",
      options: [
        {
          type: Constants.ApplicationCommandOptionTypes.CHANNEL, // Error here
          name: "channel",
          description: "A channel.",
          required: true
        }
      ]
    }
  ],
  defaultPermission: false
});

Type 'ApplicationCommandOptionTypes.CHANNEL' is not assignable to type 'CommandOptionChoiceResolvableType | CommandOptionNonChoiceResolvableType'.

node_modules/discord.js/typings/index.d.ts:3123:3
    3123   type: CommandOptionNonChoiceResolvableType;
           ~~~~
    The expected type comes from property 'type' which is declared here on type 'ApplicationCommandNonOptionsData | ApplicationCommandChoicesData'

Seems like a bug, this isn't malformed slash command data: https://github.com/discordjs/discord.js/issues/6702

outer raven
outer raven

I few weeks ago we talked about an ExcludeEnum type but I forgot to do that, we really need that type cuz these excludes are getting big

outer raven

actually @opaque vessel I have a question
the DM and GROUP_DM types are accepted by the API and throw no errors, however they result in an unusable option since there are no channels to pick from, should they still be excluded?

UNKNOWN does actually send an error so that one should definitely be removed, not sure about those two tho

opaque vessel

Ehhhhhhhhhhhh I dunno, the user is most definitely making a mistaking by allowing those types since they're not usable in the first place

Strictly speaking, if the API accepts it, I guess it should also be included...

Might have to defer to others on that :eyes:

outer raven

should I ping anyone in specific or..?

opaque vessel

Shrug

Hopefully a knight in shining armour will appear and impart their wisdom upon us

outer raven

swagcat_pray

unique axle

we usually do

ruby terrace
outer raven
ruby terrace

Weird. Well, removing those from the typings is an opinionated decision from the lib, which we don't do very much since discord could change that at any time

outer raven

yeah but I don't see why discord would allow you to set dm channel types, that seems more like an api bug to me

you can't see a dm channel on the client so you cant have it in the dropdown

and neither can you have group dm cuz bots cant join those

outer raven

I don’t know if the maintainers have any sorting settings on the pr list but the oldest pr (guild member avatars) is no longer a draft so it can finally be reviewed xd

slate nacelle

I have it open already.

outer raven

swagcat_ok

On a different topic, I'd like to discuss something
Since the properties accentColor and banner on a User need to be force fetched to appear, could we remove the else in this condition (and in banner too) so that these properties are not set so as to not lead the person into thinking the user doesn't have an accentColor/banner?

Basically the null from "we didn't get this property" isn't the same as the null from "this user doesn't have this property set" so I think they should be different in some way, and not setting the property would be the way to go imo, but since the library usually doesn't do this I'd like to get feedback first

This also applies for User#bannerURL as that should probably return undefined (or maybe even throw an error ?) when the property is missing, and null when no banner is set

slate nacelle

Probably something about "changing the shape of objects is bad" applies here, but I'm not too versed with this problematic.
Maybe we could go with undefined for "this property is unknown until fetched" cases. (No idea if this is a good idea though)

outer raven

actually maybe on the user update event but still, it wouldn't change anything I don't think

I see what you mean by "changing the shape of objects is bad" but isn't having a property set to undefined worse? I'm not sure which one you prefer here but I'd personally go with not setting

slate nacelle

How would not changing the shape be worse if that's the goal?

outer raven

like changing it to this.accentColor ??= undefined doesn't seem like something discord.js would do

slate nacelle

Yeah, we generally go for null.

outer raven

yeah exactly

so which alternative should we go for

real jetty

is this behavior intentional? shouldn't it be TextChannel?

opaque vessel

Fine for me

real jetty
opaque vessel Fine for me

That's odd, here's my entire snippet:

const channel = interaction.options.getChannel("channel", true);

        if (channel.type !== "GUILD_TEXT") return interaction.reply({content: "You need to use a Text Channel!", ephemeral: true});
copper laurel

Not sure this belongs here, but I'm was pretty sure those strings didnt typeguard unless someone has changed that

opaque vessel

Can reproduce in the interactionCreate event. Bit weird

vivid field
copper laurel

Not really a fan of that naming convention, it feels very backwards to read. Plus, although yes they're all channels what they really represent are Guild configuration settings, not the management of channels

outer raven
copper laurel

....why

That makes even less sense imo

And still ignores the first part of what I said, it's a backwards naming convention

There's no reason at all for those getters to exist on the manager because they are not managing any data

copper laurel

Happy for people to disagree but I'm really not seeing any reason for this one

visual hornet

i think it's kinda inconsistent with the everyone and premiumSubscriptorRole properties being on the RoleManager, but tbf they aren't settings
that kinda is the "backwards naming" that you mentioned though, and i think premiumSubscriptorRole having the Role at the end is kinda redundant? but that's kinda another topic

copper laurel

Hmm, yeah that's a fair comparison

outer raven
visual hornet i think it's kinda inconsistent with the `everyone` and `premiumSubscriptorRole`...

Yeah I agree the role at the end is a bit redundant so either we change that for v14 or, if we were to move those getters to the managers, we’d keep the names they already have.
The reason why I suggested moving only the getters is because the setting (only the id) belongs to the guild and can be set on the guild itself whereas the full channel is taken from cache which is managed by the manager, just like the premiumSubscriberRole and everyone. Of course those two are different as they cannot be changed but I still think the same logic should apply

outer raven
tender field

hey can i request some feedback for my PR please? the reason it's draft is because i'm awaiting feedback from the core team for the last touches

loud jayBOT
outer raven
tender field

yeah i figured as much, i'll mark it as ready for review then

white sapphire

short question, for further changes to my PR do you prefer single commits or amend+forcepush?

outer raven
white sapphire

kk thank you

but duplicating the commit message?

/ the commit message subject

burnt cradle

doesn't have to, the commits will get squashed before merging

white sapphire

perfect, thanks

outer raven
wild flax

It doesn’t really matter honestly

Since we do squash in this repo

But somewhat descriptive would be nice

copper laurel

docs

docs(Message): corrected description of property for example

fix is code fixes for bugs

Its not a new library feature

And docs is not a scope

copper laurel

fix is bug fixes (to code), feat is new library features, docs for anything documentation related

weary kiln

shouldnt there be docs for Guild.createWebhook() ?

slate nacelle

I don't think there is a relevant endpoint for that.

visual hornet

yeah there just isn't, webhooks are channel-specific

opaque vessel

Anyone know when uses here turns out to be null? https://discord.js.org/#/docs/main/main/typedef/Vanity
> Guild with vanity URL, uses is a number (0 included)
> Guild with vanity URL but not set, uses is 0
> Guild without vanity URL, uses is 0

Can't seem to find out when this is null, I fear behaviour may have changed when this was done or I'm missing something?

visual hornet

might be there for sanity? since Guild#vanityURLUses gives null if it hasn't been fetched

outer raven

Yeah that’s it ^^
If a property can be null when partial it’s marked as nullable on the docs

opaque vessel

Guild#vanityURLUses does not return Vanity, it returns a number

outer raven

Yeah but its null before you run Guild#fetchVanity right

opaque vessel

Yes

I don't see how it would be null because of that. We could just remove that property as it's never there initially and is not updated when the vanity URL is even used

Talking about the type definition though, rather than that property

visual hornet
opaque vessel

Right now just focused on when the uses property is null, don't think it ever can be, is bugging me in the typings lol

outer raven

you don't get that info on startup, so you have to manually fetch it

before fetching its null

I don't see anything wrong with that?

opaque vessel

I'm not talking about the property at all

Pretend it doesn't exist

outer raven

oh wait

opaque vessel

The vanity-url endpoint is documented (by discord.js) that the uses can be null, but it doesn't look like it can be null

outer raven

well I don't have guilds with the feature enabled so I can't test, but if you do and what you're saying is true then you can probably change

opaque vessel

I do and yeah the results were as posted initially

analog oyster

I do too, and the results were as you posted initially. doesn't seem like it can be null

outer raven

seems like the uses property isn't even documented at all on Discord's side so it makes it hard to check

burnt cradle
outer raven

oooh right I forgot the invite object has that weird documentation style

I'd say you can go ahead and make the pr then @opaque vessel

outer raven

If we change a method to throw an error where it wouldn't before, is this considered semver major?

copper laurel

yes

outer raven

Is it possible to send a warning without throwing an error?

copper laurel

Technically yes probably, but thats generally goes against library practices. What is this for?

Or at least, there isnt much precedent for it other than deprecation warnings

outer raven

A few days ago I brought up the fact that the accentColor and banner properties in the User object were misleading since they need to be force-fetched to appear and currently they are set to null before being fetched, however they can actually be null after being fetched
My idea was to make them undefined before fetching them and to make User#bannerURL throw an error saying the banner hasn’t been fetched when trying to run it

oak quail

dont we typically use null for unknown properties

copper laurel

Is that PR even merged yet. I dont see those properties on the docs

oak quail

(although a distinction would be useful here)

outer raven
outer raven
copper laurel

Its not breaking if it hasnt been released to stable

So not semver major

outer raven

Oh amazing

oak quail

was merged over a month ago

outer raven

monkaS

copper laurel

Yeah I've been out of the loop with development for a while outside of discussion in this channel

oak quail

is a release waiting on something btw

feels weird that its been sitting in main for so long

outer raven

Icons prob?

They’re out now

copper laurel

No idea

I dont think its anything specific, most likely just maintainer fatigue after releasing v13 and getting bug fixes into 13.1

Im not even a maintainer and I know I switched off a bit once it was out

outer raven

Yeah I noticed crawl hasn’t been nearly as active recently

And it was much more fun to contribute prior to the v13 release xd

Either way I’ll make the PR about accentColor and banner tomorrow
Just not sure yet if I should explicitly set them to undefined or if I should just not set them at all, guess I’ll pick one and see what the feedback is

copper laurel

I lean towards explicit undefined

outer raven

Alright, thanks

copper laurel

Theres never a case where its not a property of a user object, just cases where we dont have the value yet

Thats my rationale anyway

outer raven

Yeah true, space also seemed to agree from the discussion here the other day

opaque vessel

If you were to do that, I'd go with explicitly because of exact-optional-property-types in TypeScript.

Anyways, is it really okay to set them to undefined? If I were making use of these, I would always force-fetch them, as if the user changes these, the client does not update the properties (payload is not sent to us) so the instance you fetch them, they become potentially outdated

copper laurel

Thonk thats also a good point

Maybe we should be abandoning the concept of them as properties entirely and go with a User#fetchBannerData() route?

opaque vessel

Imo, I think that'd be wise. The same goes for others like Guild#vanityURLUses for example, where the property is only set after fetching the vanity and never updated when someone uses the vanity invite (another case of being potentially an out-of-date number right after being fetched). Should just be left as methods

copper laurel

Yeah, if we cant reliably maintain the validity of cache I don't see why we'd cache it at all

oak quail

well anything in cache can be outdated

copper laurel

In a general sense sure, but the event driven nature of discord.js keeps the cache up to date

outer raven
copper laurel Maybe we should be abandoning the concept of them as properties entirely and go ...

I don’t think so because unlike vanity data, there is no specific route for the banner info and it would be hard to have the bannerURL method if we don’t cache the banner. I think that if we set them to undefined at first and users realize that it must be fetched to be present they will try to keep it up to date, or will at least be okay with the fact that it may be outdated at any point and they’ll have to fetch again. Also wouldn’t the userUpdate event fire when these props change?

copper laurel

Not based on the discussion here, if I'm reading it right

outer raven

The gateway event documentation isn’t very explicit but I guess it would ?

I’ll try to do some more debugging later today but the properties don’t seem to be updating on their own :/

ruby terrace

Gateway user update isn't useful at all

you need a privileged intent to get real user updates

also, I think the fact that changing color doesn't fire an update could be considered a bug, though super minor (clients probably don't display correct color background in the vc window after a change)

outer raven
ruby terrace

no, the gateway user update event does not fire for anything but the currently logged in user

discord.js hijacks the GMU / PU events to get user updates

outer raven

Oohhh I see

unique axle

still think we should not do that BOYEbomb

ruby terrace

same souji, same Sadge

outer raven

Yep,same

tacit crypt
quiet viper

Why do we even need the accentcolor property?, 99% of the users will not use it and adding it in the cache, is one property more.... . The fetchBannerUrl() is a better Option.

unique axle

it's sent, it exists, API coverage is reason enough

copper laurel

You dont get to declare what 99% of users will or wont do

Besides, banners are nitro restricted. Everyone else will have a color, so your "99%" statement isn't even close to correct

opaque vessel

What's stopping us from changing how we modify the user update event for version 14? I also don't think we should do that

outer raven

But I wouldn’t invest much in v14 atm since those prs aren’t being merged rn

opaque vessel

also, I think the fact that changing color doesn't fire an update could be considered a bug
In the event it isn't a bug, I thought of removing those properties and replacing them with a .fetchBanner() method which returns an object of the removed properties via force-fetching the user. You could pass ImageURLOptions to change how the URL is received. This might be weird but I also think it's weird why we store such properties when they're potentially outdated the moment we use it

copper laurel

Waiting for Discord to give us a proper user update

outer raven
analog oyster

the Guild#owner property was removed in favour of Guild#fetchOwner() too ¯_(ツ)_/¯

outer raven
ruby terrace
outer raven
solemn oyster

It is

outer raven

banner and accentColor haven't been released on stable, so if that is merged before the next release it can still be on v13

solemn oyster

It is

You don't know how many people, me included, does === null or !== null

By adding undefined as a possible value, you're breaking a lot of code

outer raven

well on the dev release, not stable

dev releases are unstable and can change at any time, thats the whole point

unique axle

shibaThinking yes, and how exactly does that matter for the semver tag?

that's for releases, in general

outer raven

because if the feature hasn't been released to stable, it can take breaking changes and still be on the next minor release

unique axle

if something breaks the API compared to current stable that is a semver major, by definition

tacit crypt
tacit crypt
outer raven
tacit crypt

If those properties existed before its definitely major
If they were added in your PR it's minor

If those properties existed before its definitely major
If they were added in your PR it's minor

It's simple as that

I think we all get what you meant now, hold on

opaque vessel

What he's trying to say is that the features he is making changes to only exist in the main branch. It is not shipped to stable

outer raven

holding on swagcat_ok

tacit crypt
outer raven

lmao

copper laurel
outer raven

and I think it becomes a bit harder than the fetchOwner method, since you have hexAccentColor and bannerURL

copper laurel

Which is what fetchOwner does

Which is what fetchOwner does

outer raven

but that has its own endpoint

there is no endpoint for fetching the banner

and why are my messages not sending

but that has its own endpoint

there is no endpoint for fetching the banner

wow

copper laurel

Discord API issues

Discord API issues

Discord API issues

outer raven

yep

wild flax
outer raven because if the feature hasn't been released to stable, it can take breaking chan...

To clarify this:
As how I work mostly (all the time actually) with semver, I don't treat it as a "semver compard to current release" when I apply those labels. I use those labels more as a "compared to the current source code", regardless of release.
So yes, it is a breaking change, compared to the current code, but also a semver minor in the actual changelog of a release (but thats not how we use the labels, we don't use labels to indicate what a PR means to a release, we use milestones and projects for that).

outer raven

The Guild#owner getter relied on cache to find the guild owner and worked somewhat fine since all users had all intents in v12 (mostly), but became unreliable so it was replaced by fetchOwner that has its own endpoint and the method returns exactly what you get from Discord (while creating a GuildMember class obviously, I hope you get the point)
if we were to implement a fetchBanner method, not only would that be a fake shortcut for fetch() but it would also require the addition of a new class - UserBannerData (or something similar) to hold 2 properties, a getter and a method, which doesn't make sense imho
I think we should be good by just doing this initial approach for when the properties haven't been fetched at all, then store them and warn users that the properties will not be updated automatically, which I have done in my PR
This is how I see it, but please let me know if you think any of these points don't make sense

opaque vessel

That method doesn't have its own endpoint, it just fetches the owner's id in the guild

copper laurel

It's a shortcut for guild.members.fetch(guild.ownerId)

opaque vessel

However we spin this it seems a little weird I guess lol

copper laurel

My main concern is just caching properties that won't get updated, warnings or otherwise

opaque vessel

I'd be open to keeping it as it is with a warning stating it won't be updated tbh

outer raven

damn i just cant seem to send any messages

damn i just cant seem to send any messages

unique axle

has anyone filed a bug report/asked/gotten to know if that not updating is an API bug?
if it's not i'm definitely with monbrey, we shouldn't cache things that can become invalid immediately afterwards, but we don't get update events for

outer raven

but do you think a class just for those properties makes sense?

opaque vessel

has anyone filed a bug report/asked/gotten to know if that not updating is an API bug?
I don't believe so, no. I also think it may be a bug tbh

outer raven

unless you were to pass the banner url options in the fetch method and then it would give you the url with those options in just an object, which just seems increasingly stupid

and no i dont think anyone has made a bug report

do you even know if those properties not being sent with the guild member data and others is intentional?

do you even know if those properties not being sent with the guild member data and others is intentional?

unique axle

unless.. the client doesn't really update banners, it keeps them as-is and fetches them whenever you open a profile, iirc

outer raven

do you even know if those properties not being sent with the guild member data and others is intentional?

unique axle

unless.. the client doesn't really update banners, it keeps them as-is and fetches them whenever you open a profile, iirc

outer raven

discord please

unique axle

unless.. the client doesn't really update banners, it keeps them as-is and fetches them whenever you open a profile, iirc

outer raven

discord please

outer raven
dim steppe

Checked the site and couldn't find any mention of this yet — have I missed any further discussion of it? Was anything decided upon?

analog oyster
tacit crypt

If its the linting imo I'd say do it but I'd wait for @wild flax too

analog oyster

it is the linting

outer raven

@remote wasp was testing the role icons PR and I noticed that trying to set a role's icon to another role's icon URL without any options (webp format) gets rid of the transparency of the icon. What do you think of defaulting the role icon endpoint format to png instead of webp?

I would personally like for all endpoints to default to png but that would be something for later

remote wasp

makeImageURL defaults it to webp and every other CDN method did too before your PR. If it was a random choice then yeah sure, but if there was a reason why only webp was chosen as default then you might want to ask others too

earnest topaz

When is the new api change going to be added?

outer raven
opaque vessel

Discord tends to use .webp & they're also generally smaller in size for same/better quality... off the top of my head that'd probably be a contributing reason why it's used as the default

earnest topaz

Oops sry

outer raven
opaque vessel

All avatars are in a .webp format, a quick Google reveals that it does support transparency, and people may be downloading them

outer raven
golden mortar

how can i make the esm portion of the lib manually (ie. a pr)?

vernal atlas

there seems to be an error in the latest dev version (13.2.0-dev.1633089797.fe95005) - passing message options w/ an empty string content and an embed gives you an error saying the content can't be empty, but this is valid because the message has an embed

just to confirm, empty content but with an embed, which is valid but obviously the validation checks give a false negative

copper garden
vernal atlas

ah i see

would it make sense to accept an empty string too though, if there is other content on the message (last time i checked discord did accept this, don't see why that would've changed)

copper garden

I see what ur saying, but that would require a breaking change for an edge case that most ppl won’t run into

slate nacelle

I think we intentionally are treating an empty string as error since this happens a lot on accident.

ornate topaz

it's been erroring like that since at least 13.0.0-dev.b15d825bb3acdf432b94d8413a7a964ccc8734bc, though it didn't complain this way on v12

copper garden

v13 added the stricter string checks bc ppl were passing in objects into methods that only accepted string

outer raven
outer raven
oak quail
outer raven

Because iirc the api doesn’t accept empty strings at all

oak quail

the api supports both but doesnt document nullability

outer raven
oak quail

Pretty sure that is not related to sending messages because I just called MessagePayload.create myself and it passes null in the content

oak quail

that is called by MessagePayload.resolveData which is called by TextBasedChannel.send

also i tested with the raw api and both work

outer raven

Ah I was one step behind then
Well I guess the empty string check could be removed then and let users get Discord’s error instead? Or maybe it would only be allowed when there is an embed

I think embeds are the only other thing that counts as content right

oak quail
outer raven

Oh yeah

That check would be more robust then, but it’s easily doable

And would not be a breaking change

In fact it would probably be minor

remote wasp
golden mortar

thanks, exactly what i needed

summer beacon
outer raven

@ruby terrace "In order to type the response, I had to add @types/node-fetch (since we don't target DOM), which does not have a version matching the version in the library since v3 has built in types." wdym? The last published version of @types/node-fetch supports v2.5 of node-fetch

ruby terrace

and the lib has 2.6.1 installed. The types for Response should not have changed, but its a semver minor bump, so some types are wrong

outer raven

I find it weird that the maintainers of @types/node-fetch didn't bump it tho
perhaps they did and forgot to bump the version?

dim steppe
outer raven
dim steppe

that's what I've been doing until something was decided upon:
this.memberPermissions = data.member?.permissions ? new Permissions( data.member.permissions ) : null;

outer raven
slate nacelle

What's the use case for that?

dim steppe

Discord provides the member's exact permissions, all roles included, of the member which the interaction was created

DJS currently ignores this

slate nacelle

You can locally calculate them just fine through the permissions getter, can't you?

Plus they update unlike the number you get through the payload.

dim steppe

No. Now, with DJS' caching, if you don't have a channel cached, you have to fetch the channel. If the bot doesn't have VIEW_CHANNEL for that channel, you get an error

wild flax

The reason we ignore it is because we already have the ability to easily calculate it if its cached

If you don't have the channel cached you might have other issues with djs peepoLaughPoint

Just saying

dim steppe

The only issue I've run into so far is trying to fetch channels that the bot doesn't have VIEW_CHANNEL for. You can still respond to interactions in these channels, but you can't fetch permissions of members/roles for them

slate nacelle

You surely can do that.

You get all channels of all guilds your bot is in, no matter the permissions.

dim steppe

Right, and if you don't cache them, you have to later fetch them. Manually fetching a channel which you don't have VIEW_CHANNEL for is a missing access error

outer raven

yeah and the api docs don't specify any limitations in regards to permissions

outer raven
dim steppe

Yessir

So we need to implement the permissions that Discord sends us with the interaction

Otherwise, if we don't cache channels, we need to fetch the channel (which could result in an error), manually calculate the member's permissions based on their roles, fetch the default/global permissions for each role they have, etc... When Discord just... Sent it to us

outer raven

you can fetch any channel as long as you're in its guild

vivid field

it also seems weird to me that you have the guild cached, but not its channels

dim steppe
outer raven

you're doing something wrong then

slate nacelle

/guilds/:guild_id/channels includes it, it's also part of the guild create payload.

Not sure what the point in discarding channels is however.

outer raven

@dim steppe see

dim steppe
vivid field it also seems weird to me that you have the guild cached, but not its channels 
bot.on( 'interactionCreate', async interaction => {
  await interaction.guild.channels.fetch( interaction.channelId )
} );

DiscordAPIError: Missing Access
    at RequestHandler.execute (C:\Users\Stev\projects\rocket-planet\node_modules\discord.js\src\rest\RequestHandler.js:298:13)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async RequestHandler.push (C:\Users\Stev\projects\rocket-planet\node_modules\discord.js\src\rest\RequestHandler.js:50:14)
    at async ChannelManager.fetch (C:\Users\Stev\projects\rocket-planet\node_modules\discord.js\src\managers\ChannelManager.js:114:18)
    at async Command.handler (file:///C:/Users/Stev/projects/rocket-planet/commands/reputation/mjs/handler.mjs:30:9)
    at async Command.handleCommand (file:///C:/Users/Stev/projects/rocket-planet/structures/command.mjs:395:30)
    at async Promise.all (index 11)
    at async Client.handler (file:///C:/Users/Stev/projects/rocket-planet/events/command-interaction/command-interaction-handler.mjs:47:9) {
  method: 'get',
  path: '/channels/658894775814062096',
  code: 50001,
  httpStatus: 403,
  requestData: { json: undefined, files: [] }
}
outer raven

🤨

dim steppe

The only permissions it doesn't have for that channel is VIEW_CHANNEL.

As soon as I give it that permissions, it works

vivid field

but as I said, if you have the guild you should have its channels too

dim steppe
outer raven
outer raven
dim steppe

So far I've been able to account for that by manually fetching the channels. The only issue I have now, because of the error cause by the VIEW_CHANNEL permission, is determining permissions in channels that the bot cannot fetch, but interactions can still be used in

Still, it seems silly to not implement information that Discord sends, and instead jumping through hoops to attempt to manually fetch and calculate the very same information

It's not an issue for small bots. It's an issue for large bots that can't afford to cache literally everything

wild flax

It's not silly at all if you think about how we don't even support overriding the channel manager lol

But I guess we can do it, space just asked for a use case and a reproducible example how it errors

Not that its dumb, sooo

It just needs to nicely work with the current permissions and not be confusing

dim steppe

I find it even more silly that you can't fetch channels without view channel. 😕

wild flax

Because best case scenario, you wouldn't want to use interaction.member.perms if you can calculate them differently

Except in your use case I guess

outer raven

I think a memberPermissions property in the interaction object works just fine, seeing as you cannot overwrite member.permissions

is that present on all interaction types or just some? The api documents it as optional with no extra context so idrk

wild flax

well

dms

dim steppe

Yeah, global interactions used in DM don't have it.

I wonder if the permissions issue was somehow Discord's way of attempting to address the problem of "hidden" channel names and topics being technically visible to anyone using a self bot or BD or something.

slate nacelle

Probably just inconsistency.

The client still has all of them.

opaque vessel
outer raven
opaque vessel

Ah yes, okay

outer raven

and it can't be 0 because the member needs permission to send messages and use application commands to send the actual interaction

outer raven

I just noticed we can't use ThreadChannel#setLocked on a thread that is archived, could this be documented with an info on the edit and setLocked methods?

ruby terrace

archived threads are completely immutable other than unarchiving, which is pretty well documented IMO

outer raven
ruby terrace
outer raven

On another note though, it seems like setting locked to true on a thread doesn't do anything if archived is false, both of them have to be true for the thread to be locked

Am I doing something wrong or is this an actual bug?

slate nacelle

That seems to be somehow intended behavior.

outer raven

well then the setLocked method should be changed right?

because currently it only edits locked, not archived

slate nacelle

It's not called setLockedAndArchived though.

outer raven

but setLocked is supposed to, well, lock the thread

and currently it doesn't

slate nacelle

It does, it not doing anything until the thread is archived is on Discord however.

outer raven

that's very counter-intuitive

slate nacelle

Concur

outer raven

so that the actual behavior on the client can be reproduced

slate nacelle

No, at this point you should use the edit method.

outer raven

well the setLocked method just seems useless in that case then

slate nacelle

I'd say very niche, not useless however, since it's doing what Discord intended for some reason I don't know.

They even added that to the client by now.

outer raven

ah I see

well but the setLocked description says this, and the latter part is not true I believe

slate nacelle

How is it not?

outer raven

because after doing thread.setLocked() everyone can still chat in it, and to me that sentence gives off the impression that the thread would be actually locked so that people couldn't talk in it

slate nacelle

Yeah, I agree that locked is a misnomer here, but unfortunately that ship sailed long ago and there isn't anything we can do about that.

outer raven

well yeah changing the method now would probably be somewhat breaking so I guess we'd have to wait but it would be nice to change this method to lock and archive the thread

slate nacelle

I'd be in favor to explicitly document this "quirk" and suggest to use edit instead when one wants to archive and lock.

outer raven

that could be done now then

Wait so if a thread is archived and not locked do I need 2 api calls to make it archived and locked?

slate nacelle

I don't know, but could be necessary due to how Discord handles threads.

outer raven

seems like it is since you get an error when trying to do anything on an archived thread

well that's extra annoying, but thanks

outer raven

Is it just me or does editing threads not support a reason?

the parameter is there on djs but it doesn't show up on the audit log

opaque vessel

Loads of methods do that, it's a client bug no one cares about

outer raven

Ah I thought it wouldn't be present on the audit log entry but it is so nvm I guess

I thought this was the same scenario as the message.delete method

lone vector

I'm gonna post here cause I can't really verify if this is a definite library bug, but I've been noticing that fetching certain invites are throwing errors from InviteStageInstance, which I believe is because in my case, my bot is fetching an invite for a guild it's not in, which has a public stage instance, then the library is trying to set the members cache for that stage instance but it's using this.guild.members where this.guild is a getter from guild cache, so guild ends up being null

Maybe someone else would be able to verify this?

opaque vessel

Public stages are going to removed anyway, right?

outer raven

well yeah but that doesn't really matter

outer raven
lone vector

I can give you an error stack but that's all I really have

outer raven

I suggest submitting a bug report on github, surely you have the code you're running

lone vector

I do yes, but it's not really "stock" code

slate nacelle

The descriptions seems clear to me. Now the question is: How to address this?

lone vector

I had thought for a simple fix, wrapping some of the code in an if (this.guild) check might have worked, but I wasn't sure if there could be better solutions

outer raven

well you have the guild in the data from the invite fetch, so it shouldn't be too hard

although you don't have all the members in the guild but you get the members in the stage instance

lone vector

Yeah, another option may be to send the guild directly to the stageinstance class instead of just the ID

outer raven

yeah

opaque vessel

this.members should default to null, and if a guild is detected, it should become a Collection

slate nacelle

members is only on the "actual" guild class, not on invite guild.

outer raven

yeah but you have the members inside the stage instance object

and those seem to be full guild member objects

slate nacelle

That's the problem.

outer raven

only issue is that the roles are ids and we dont get any other info abt them

slate nacelle

Easiest way would be to just have rawMembers or something, but that's probably not optimal.
And make guild an InviteGuild (assuming the props are the same) - might be breaking blobupsidedown

outer raven

btw this warning is a bit dangerous isn't it? I feel like it's way too alarming for no real reason since the user should be able to tell when each one of the properties will be present in their case

outer raven
slate nacelle

It's documented as Guild or null.

outer raven

on what class?

slate nacelle

The one reported, InviteStageInstance.

outer raven

ahh

I was looking at Invite (which has a bug on the guild property that causes it to never be a Guild, will try to fix that)

But yeah it seems like the only solution here would be breaking, which makes it useless since public stage instances won't be a thing when v14 starts to be developed

slate nacelle

A non-breaking fix would be to have new props for both things, not really nice, but would work.

outer raven

it would take longer for that pr to be merged than it will for stage discovery to be removed soooo

outer raven
copper laurel

I dont see whats wrong with having that warning?

outer raven
copper laurel I dont see whats wrong with having that warning?

like I said, it's scaring users for no real reason. Basically what that warning was meant to mean is that those are the only properties that will be present on the invite object no matter the type of invite, but it gives off the impression that, for example, when fetching a guild channel invite, the guild object won't be guaranteed to be present

copper laurel

I dont get that impression

outer raven

I'm saying this because I actually saw that when I first started and that was the idea it gave me, but now that I understand the discord API a bit better I know that's not the case

copper laurel

It just says they can be missing

outer raven

in a way that makes it seem like it's an unreliable behavior

because in a message, the member object can be missing too and that doesn't have a warning, because users can deduce that

copper laurel

I think the point of this warning is not to focus on things that are clearly contextual like a guild or member, and more relating to properties like uses

outer raven

when can uses be missing?

the api docs don't document that as nullable nor optional

oak quail

public stages are very useful beyond discovery

outer raven
oak quail

they show in invites and on profiles

outer raven

On profiles? 🤔

outer raven

I have this on my fork so I assume the upstream repo has this too. When will this be addressed?

copper laurel

Did you check what it actually is?

That can show up in some obscure dependency of a dependency of a dependency and its fixed whenever that dependency fixes it

outer raven

yeah they are dependencies of dependencies but you should be able to fix them without the main dep fixing it on their side

only 1 of the 4 alerts doesnt have a fix available, all the other ones do

copper laurel

Thonk I dont see that on my fork

outer raven

is it up to date?

you can even see this on npm with npm audit

and they offer you the fixes, however one of them is downgrading dtslint to v3, not sure if that would break anything

copper laurel

Yeah I just updated it

Thats kinda my point though - we wait for dtslint to release a v4 version with it fixed

outer raven

dtslint is deprecated tho, it won't be updated

copper laurel

Probably should replace it then

outer raven

it should be on eslint now, lemme look into it

copper laurel

dtslint relies on @definitelytypes/utils which relies on tar which is where one issue lies, and npm-registry-client which relies on ssri which is another

tar and ssri are the actual flawed packages

outer raven

there is an open PR on the definitelytyped utils repo to bump tar, but ssri comes from another dep of theirs

tawny warren

is this a library bug? i used message.guild.stickers.fetch()

    at Sticker._patch (/usr/src/moonlightbot/node_modules/discord.js/src/structures/Sticker.js:81:50)
    at GuildStickerManager._add (/usr/src/moonlightbot/node_modules/discord.js/src/managers/CachedManager.js:48:26)
    at GuildStickerManager._add (/usr/src/moonlightbot/node_modules/discord.js/src/managers/GuildStickerManager.js:31:18)
    at /usr/src/moonlightbot/node_modules/discord.js/src/managers/GuildStickerManager.js:161:65
    at Array.map (<anonymous>)
    at GuildStickerManager.fetch (/usr/src/moonlightbot/node_modules/discord.js/src/managers/GuildStickerManager.js:161:32)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Command.exports.run (/usr/src/moonlightbot/src/commands/Owner/eval.js:10:16)
    at async module.exports (/usr/src/moonlightbot/src/events/command.js:127:20)```
slate nacelle

Yes, has been fixed in main with #6421.

wild flax

If we get some more approvals on stuff I guess we can release 13.2

Nothing stopping us really

outer raven

Role icons should be on 13.2 imo

space saw this message

outer raven

Hey I'm pretty sure this is false as I just tested and the event didn't fire, can someone else confirm?

quiet viper
outer raven

And it was in my tests

userUpdate fired, not guildMemberUpdate

by looking at the code it does indeed look like guildMemberUpdate should be firing, but idk why it isn't

opaque vessel

From glancing at the code, it looks like it happens when the user is first recognised and in a guild

visual hornet

that seems like a bit of a stretch, since nicknames aren't really part of the user's info

opaque vessel

Look at line 10-15 in the source code for that event

outer raven
opaque vessel

I guess the description isn't entirely inaccurate then

outer raven

what I think is that there's a bug here

opaque vessel

Yeah, the bug is discord.js emitting user updates manually >_< lol

outer raven

it its documented saying the event can fire on username updates, then it should be firing with username updates

outer raven

Ok so I found out why guildMemberUpdate isn't firing on user changes, it's because we check if !member.equals(old), which returns false since the member didn't update, only the user did. My question is, do we wanna remove the line saying the event fires on user updates or do we wanna fix the event so that it fires on user updates?

@tacit crypt I see you online so could you give me your opinion on this pls ^^

tacit crypt

I have no idea to be honest

outer raven

tbh since discord emits guild member update events for user changes I think we should also do the same

slate nacelle

Not exactly trivial to do properly.

outer raven

it should be easy

slate nacelle

Surprise me

outer raven

give me a min

@slate nacelle

old is a GuildMemberObject

slate nacelle

Aren't those user objects reference equal even?

opaque vessel

So you're firing user updates in there now?

slate nacelle

That's how we currently do it.

outer raven

tested and it fires the event

slate nacelle

This should only fire for the first guild member update, you should receive one for every shared guild however.

opaque vessel

Couldn't a fix also be to simply emit the guild member update in the user update actions as well

outer raven

I found some flaws with that fix so lemme do more tests to make sure it works

outer raven

Ok so after a lot of head scratching I just keep finding more bugs and I need feedback on these

So the check in https://github.com/discordjs/discord.js/blob/a8c21cd754d634b4d40047f85264528681a61b41/src/client/actions/GuildMemberUpdate.js#L13 is broken and will always return false because the equals function now compares banner and accentColor, which are not present in a user object received from a member object. Should we make a private method to compare the user to the api user? This isn't really a major issue though because in the userupdate action it compares 2 user objects, so that will work as expected
The User#equals method does not compare user flags, causing the event to not be emitted when it should
Due to the first point, we cannot use that same check to emit guildMemberUpdate because we cannot have a user object to compare the cached one with, unless we call the User constructor which I'm not sure we should do ?

So overall the action is sort of working as expected with a lot of things that don't work in the process, which makes it impossible to make guildMemberUpdate fire when the user updates, like Discord intends us to do

outer raven

@opaque vessel @slate nacelle btw ^^

outer raven

Gonna give my final 2 cents on the matter

Discord sends presence updates where there should be user updates and doesn’t send user updates without the presences intent
Even with that disabled it’s impossible to send a guild member update event with the old data. The only way to do that is to not send a user update event. Either one of them will not have the updated data due to how we store them. I did find an issue in the user equals function though so I’ll submit the PR to fix that and another one to undocument that sentence saying guild member update can fire with username changes

And also due to that I think we should keep the manual userUpdate event emits because otherwise the event will never be emitted except for client user updates which also happens to be a manual call

If anyone wants to attempt a fix for this however be my guest, but I just spent an entire evening banging my head against the wall with this and it didn’t go well

tacit crypt

Or we could, you know, drop the equality checks since if we get a GUILD_MEMBER_UPDATE event, something updated.

Just my 2 lightboulb

outer raven
tacit crypt

Does it matter?

It doesn't

outer raven

Wait so what exactly are you suggesting

tacit crypt

Our fake events for things that don't come from the ws are just a maintenance headache imo

tacit crypt
outer raven

The issue is that you have to patch the existing user and if you do, you’d end up emitting an event with nothing changed

Because the current workflow is
Member update received
User equals check
User gets patched and userUpdate event is sent
Guild member equals check
Guild member update event

If we remove the checks, the guild member update event will fire without any changes since the oldMember.user will already be patched

I think that’s the reason why those checks are in place

tacit crypt

Doesn't a clone clone the user too

outer raven

It does yeah but that only applies for the user update action

It works correctly on userUpdate, but when you get the old member from cache, it’s user object is already patched because the user update action modified it

And yes this is very painful to work with and would be much simpler if we just didn’t emit fake events, but we can’t do that now wahh

real jetty
outer raven

otherwise only for the client user

real jetty
outer raven

can you show me the updated data?

real jetty

You mean my code?

I only logged a few, like trying to change my Hypesquad house but it didnt emit

outer raven
real jetty 
if (oldUser.flags?.bitfield !== newUser.flags?.bitfield && newUser.flags?.bitfield > 0) {
      console.info(`Flags of ${newUser.tag.red} changed: ${oldUser.flags?.bitfield} => ${newUser.flags?.bitfield}`);
      await User.updateOne({ id: oldUser.id }, { $set: { flags: newUser.flags?.bitfield ?? 0 } });
    }

This is what I am trying to do. Update database entry when the bitfield changes. I know its not the best code. When the event emits oldUser.flags.bitfield equals undefined and newUser.flags.bitfield equals 0. Otherwise it does not emit

I also logged data before the if statement same result

outer raven

that's odd, I could not reproduce this in the 3 hours of testing I did on these actions, it always worked perfectly

unique axle

friends, this isn't really a support channel
you have to fetch user flags, and if the old ones weren't cached you won't be able to obtain them after they changed

visual hornet

isn't uncached stuff supposed to be null though

outer raven

and it is

outer raven
visual hornet

oh i brainfarted, excuse me

real jetty

I am sorry for asking here, but no one could help me in the help channel. So I need to fetch the users/members on ready event to have the flags cached?

outer raven

I don't think you do, they get sent with the member object

real jetty

But shouldnt the new flags still be greater than 0?

unique axle

do they actually ship them with every user payload now? that was def. not the case before

outer raven

for some reason there's also flags and public_flags

real jetty

Huh

unique axle
outer raven

yeah but when will they not be sent?

unique axle

catShrug do you know how many things come with users we cache?

real jetty
unique axle

if our equal check doesn't account for null, possibly so
user updates are a bit of a headache anyways atm, because they don't really exist in this capacity, we forge them from the inner user payload on receiving GUILD_MEMBER_UPDATE event payloads

and iirc the current implementation just only emits the first and drops all others (in case your bot shares more than one guild with the user)

outer raven
loud jayBOT
real jetty

I just want to fix my problem 😫

unique axle

see if the linked PR does

real jetty

But I will need to wait for the next version to release or use dev build

outer raven

nah, install it

you have the command there

real jetty

I am currently not home but I will try

But why not just drop the equals check from discord.js side?

Doesnt it make no sense if the event emits then it is a new user object?

outer raven

like we were discussing, the user update event emits with other events

outer raven

So I'm gonna remove that phrase from the guildMemberUpdate docs. Should I add an info tag saying this will change in v14 or do we not do that here?

wild flax

I'd rather not make predictions for semver versions

outer raven
copper laurel

Development on v14 starts whenever people start opening semver major / breaking PRs

outer raven

there already are semver major PRs tho

copper laurel

Then they are for v14

Or might be anyway

outer raven

well yeah I know but I'd rather wait for those to start getting merge than clogging up the PR list

copper laurel

Theres some technicalities in there

outer raven

guess ill just delete it then

wild flax
outer raven

ah yeah

wild flax

so putting it on the docs now might convey a wrong image

outer raven

sure thing then thumbs

dim steppe
analog oyster
dim steppe

The introduction of banners, I believe, added new sizes. The one that comes to my head currently is 300. The sizes can also be used with avatars etc

https://cdn.discordapp.com/avatars/228937760390643713/353889e1eb10ab1e61d7274028a69500.webp?size=300

IIRC, default banners are 300px

analog oyster

do you know the full list of valid image sizes that aren't documented?

perhaps the image size validation should just be removed

dim steppe

Not off hand — I had the ones I had found when I was at work earlier. Only one I remember off the top of my head was 300

Oh and 600

It's not pretty, but could just:

const AllowedImageSizes = [...Array.from({ length: 9 }, (e, i) => 2 ** (i + 4)), 300, 600].sort((a, b) => a - b);
copper laurel

Would they not be two totally separate sets of image sizes

dim steppe
copper laurel

Fair enough

real jetty

I think if you have the message partial enabled then djs should send the messageUpdate event even if the old message isn't cached, instead of just ignoring the newmessage entirely

Sometimes you don't need the old message at all, like for link scanners or swear detection.

slate nacelle

That's the idea behind partials.

copper laurel

Isn't that exactly what it does? I'm not sure what's been suggested / addressed here

outer raven

Could you change the dev build interval to be in the middle of the day or something? The majority of the time the release at midnight is the same as the release at midday because everyone is asleep during the night and barely any prs ever get merged then

dawn merlin

So I found a place that tslint is fighting with prettier, TSlint wants this:

export class ...
  extends ...
  implements ... {

But prettier wants this:

export class ...
  extends ...
  implements ... 
{

it seems that setting one-line to false in the tslint.json fixed the issue

outer raven

Which one do we want tho

I’d say the first

ruby terrace

I don't think prettier should be running in the .d.ts files

tacit crypt

I don't think we should be using tslint

ruby terrace

well...yeah, eslint needs to support .d.ts files for that though Sadge

outer raven
outer raven

but I've never gotten eslint errors on the typings

dawn merlin

my vscode runs prettier formats on save

ruby terrace

by using eslint src

outer raven
ruby terrace

vsc can, its a setting

dawn merlin
outer raven

but I remember when you made the messageReactionRemoveAll PR every PR created after that changed the formatting on that event xd

dawn merlin

that might be because prettier isn't supposed to be run on the typings files? idk

outer raven

what else would we run then? It has to follow some formatting rules

dawn merlin

It looks like it does run prettier in ts files on commits so idk why that in PR caused issues

copper laurel
outer raven

which we probably should, but I personally don't know how

copper laurel

oh i thought we did

outer raven

make a pr pls

copper laurel

I mean it doesnt seem that important, whats your concern with it releasing?

outer raven

my initial suggestion was to just change the schedule

wild flax

You know timezones are a thing right?

What you describe as midnight only works for the UK

real jetty

Will yall ever add a onclick function to interactions?

outer raven
wild flax

Yeah I don't think thats necessary sorry

its "nightly" for a reason

outer raven
wild flax

what will that change?

unique axle
analog oyster

clicking on a button emits "interactionCreate"

real jetty
unique axle

if you want to target a single specific button, like a onClick, a component collector is what you want

outer raven

not extremely important, but would be nice thats all

wild flax

dont really care about npm versions honestly

outer raven

aight

opaque vessel

I noticed the change log that gets generated references pull requests as issues o,o

For example

wild flax

prs are issues

theres no difference

opaque vessel

Welp, alright then (:

surreal bough

has v13.2.0 been pushed to npm?

not yet right?

outer magnet

since ephemeral messages can now be fetched, does that mean we can create component collectors on em?

nevermind, tested them and it works

wild flax
surreal bough

oh okay tysm

outer raven

@wild flax github had an outage on actions when you released 13.2, could you try to re-run the docs publish action?

outer raven

this is still not fixed pensivebread

thorny radish

hey do you guys think it might be a good idea to export the type of type as a type?

export interface ActivityOptions {
  name?: string;
  url?: string;
  type?: ExcludeEnum<typeof ActivityTypes, 'CUSTOM'>;
  shardId?: number | readonly number[];
}

cause as of the latest d.js update I can't use as ActivityType anymore when setting type to an environment variable and I instead have to add the whole ExcludeEnum to my code since that is not exported either

outer raven

wait no I didn't, it's still there

thorny radish
outer raven

can you show the error?

thorny radish

neither is ActivityTypes

when using as ActivityType

outer raven

show the code please

thorny radish 
presence: {
    activities: [
        {
            name: process.env.CLIENT_ACTIVITY_NAME,
            type: process.env.CLIENT_ACTIVITY_TYPE as ActivityType
        }
    ]
}
outer raven

hmmm that's a very niche case

what happens if you remove the "as"?

wait you can just use Exclude<ActivityType, 'CUSTOM'> iirc

thorny radish

oh that worked, swear i tried that but apparently not. thanks

outer raven

this is also erroring out

someone re-run the actions

copper laurel

Im sure he will once he sees it

Probably asleep or busy

remote wasp

does @discordjs/rest module supposed to be used outside of node too?
It's related to this PR: https://github.com/discordjs/discord.js-modules/pull/55. The issue mentioned in the PR is still there and you still have to opt in for dom types in tsconfig, else it won't compile. I imported the URLSearchParams type from node's built-inurl module, which let's you compile without adding dom types. So, what was the reason that they went with that solution and not this?

wild flax

cc @strong hull

warped crater
strong hull
remote wasp

oops, turns out your fix was released in the @canary tag. I installed that tag and it does fix the issue. Still, why didn't you went for the node type here

burnt cradle
warped crater

eeerm, my bad

*named capture groups

burnt cradle

Is there a reason to make it a named capture group over a non-named one?

warped crater

yeah I didn't realize it was grouped in the first place, cus I was looking for named groups meguFace

but yeah, it could be beneficial

bit redundant but could be

.groups.id is much nicer to access

strong hull
wild flax

You’d have to ask the person who implemented it

Git blame it maybe?

strong hull
tacit crypt
strong hull
tacit crypt

Because the latter needs an import when its otherwise global anddddd the only reason the node typings don't make it global is bc of the clash

strong hull

this Well there ya go @wild flax @remote wasp

remote wasp

I see. How about using the global URLSearchParams when using it as a value and importing the type when we just need to use it as a type? This way we won't have to do anything related to dom

tacit crypt

You.. won't have it in your globals if you don't use the dom typings pretty sure

And TypeScript will yell

ruby terrace

that's correct yeah (had this issue with Response typings in the main lib)

remote wasp

got it 👍

analog oyster

Right now, by the time the ClientPresence class is instantiated client.user is not available so there's no easy way to get the client id

grand void

Hey, i want to ask about Error [USER_BANNER_NOT_FETCHED]: You must fetch this user's banner before trying to generate its URL!. I always get this error when json stringify User and fetching User, I not using method <User>.bannerURL(), but why i get that error message?

my bot always getting crashed because that error

outer raven
grand void

yes i know, i can use JSON.parse and JSON.stringify method

outer raven

Ah wait I know the fix

Don’t have the time to open the PR rn but I will later if no one else does before me

Also the toJSON method exists, it’s just not documented so there’s probably a reason for that

grand void

i'll test it later

outer raven

Me and a friend of mine managed to get the guildMemberUpdate event to fire on userUpdate's like the Discord gateway does. My question is: should we also fire presenceUpdate when we receive it from the API as a userUpdate or should we leave that for guildMemberUpdate only?

wild flax

presences don't really exists on users

outer raven

Yeah ik despite Discord sending the user with the presence update payload, but we won't fire it then, thanks thumbs

wild flax

yeah I never really checked how it works when you are only friends and don't share a server

im sure theres something happening for this

but it doesnt really concern bots

outer raven

yeah I can't check that either but it shouldn't matter and it's way out of the scope of this PR we're making anyway

oh btw @wild flax I noticed you didn't really review the vscode extensions PR, do you not agree with it or did you just not see it?

dawn merlin
outer raven

yeye I know, I was literally about to create that PR

dawn merlin

im out of the loop here, but why does #bannerURL need to throw in the first place?

outer raven

my idea behind it was to force users to fetch the banner before trying to generate it

but in theory you could throw undefined, but that could be badly interpreted

dawn merlin

but doesn't undefined imply it just isn't part of the object at all. And then null just means it could be part of the object but it isnt

if it isn't part of the payload, just returning undefined seems fine, no?

grand void

👍

outer raven

also unrelated but this doesn't follow commitlint so it won't show up on the changelog, maybe someone should force push to fix

wild flax

What do you want to hear, good luck it being a semver:major now

Not like we can change it

outer raven

it's not tho

removing an error is patch, adding it is major

wild flax

Removing throwing is very much a major

You aren't fixing a bug, you change behaviour

outer raven

how is it? I've had prs that removed errors on semver minor

or patch even, can't remember

wild flax

Feel free to dig them up

So I can actually see the context

loud jayBOT
outer raven

it was patch actually

wild flax

Thats quite different

Look at what the PR does lol

You made something generally work in all reliable cases

Whereas before it wasn't

outer raven

well yeah and here we're making the bannerURL method work in all cases, but output something different in the cases where it throws atm

so going from error thrown to no error + undefined return doesn't seem like a major to me

analog oyster

the throwing if not fetched behaviour is documented. it would be semver: major.

zenith oracle
outer raven

Eh I guess so

Both alternatives work

And we should’ve had this discussion before the PR was merged anyway

If anyone wants they can fix it when v14 development starts I guess

dawn merlin

Can't anyone submit PR's for semver major now?

outer raven

Well you can

But I am saving those for when we’re merging semver major prs so as to not clog up the pr list

wild flax

you arent really clogging anything up

outer raven

Just a personal preference tho, you can do whatever you want

wild flax

so we have a nicer view of things

outer raven

Ohhh I thought you went through the list

Aight thats good to know

wild flax

hell naw

outer raven

But still, we have TODO comments for some stuff so I’ll wait

dawn merlin

also one last question on the bannerURL method, is there a specific reason it isn't a getter?

zenith oracle

It's a method as other methods like avatarURL, iconURL etc...

Also it takes an options parameter so it couldn't be a getter

outer raven

It should always be customizable

dawn merlin
outer raven

What do you guys think of a possible CommandInteraction#toString method that would return the command that was run in a way that can be copied and executed again? It would basically be /commandName {subcommand group} {subcommand} {option1:value} ...

zenith oracle

Sounds good to me

vernal adder

What would be the use case for that?

You can already do that inside the client.

Not that it works 90 % of the time, but. (parsing the copied command, not copying it)

opaque vessel

Logging commands. I do it

vernal adder

I see.

coarse ginkgo

Do we know if bots can have avatars per guild?

vernal adder

They can't.

coarse ginkgo

👍

outer raven
opaque vessel Logging commands. I do it

question for user/channel/role options though, how should we display these? Discord accepts the ID on all and this would be the easiest to implement but it also supports @Username#discrim for users for example

for roles that is just @RoleName tho so maybe we shouldn't go that way as that's quite unreliable

if the point is to just make it executable the ID works fine

opaque vessel

You only get the id though, so, there's only one option to use. This also gets opinionated so I wouldn't recommend adding it really

outer raven

the other toString methods are all for utility purposes and if the point is to make the command re-executable (if that's a word lol) easily then I don't see why not

but ye I'd def like some more input

copper laurel

Does pasting in slash commands even work?
But I'd still go with ID, it would be the only reliable way to actually ensure you're getting the right values that were used the first time

Is it even possible to replicate the command in order though? Is the raw data reliable for that?

outer raven

I can test if you want tho

visual hornet
copper laurel

Which is why I'd probably go with ID

Though the new editor is coming soon too, might be worth waiting? idk

visual hornet

editor?

outer raven
copper laurel

They released this at the same time as the autocomplete docs and permissions overhaul info

But since its client side rather than dev focused maybe it got less discussion here

outer raven
copper laurel

I mean its not just a visual change

Its an semver major upgrade of the editor in the client

Slate, the library that powers the editor, has undergone a massive rewrite since we originally built this component.

outer raven

that rewrite was internal, the editor update shouldn't remove any features, and should just add those boxes and multiline strings from what I understood

copper laurel

...of course it wont remove features

But theres a very good chance it will change how it behaves if you were to paste in a slash command input

outer raven

still the data passed to the API won't change for sure, and that's what we're using here

it's only a client-side update

I don't see how that update could break this function tbh

outer raven

Working as intended btw, will PR

copper laurel

That's still not what I meant, at no point did I suggest the API data would change

But the whole use-case for this feature, if I'm not mistaken, was to output a string that could be used to copy and paste into the editor to re-use the command, right?

And the editor input is going to change

outer raven

Hmm so you think the option:value format will change?

copper laurel

It might still work perfectly fine afterwards, but its just something to keep in mind

outer raven

Or could at least

copper laurel

Could, yeah. I wouldn't say I think or know it will

outer raven

Alright then
I have the branch published and will make the PR once that update is out

copper laurel

Since I think one of the issues they did want to address was how you tab through options and stuff

outer raven

Yeah I think all options are these by default, not sure how optional options work tho, they didn’t really show those

copper laurel

You could probably PR it now if it works now

If it doesnt work with the new editor we can probably patch it? idk

outer raven

What if its not patchable? Not likely but its possible

Did they give us any ETA for that update btw?

copper laurel

nah

outer raven

wahh

I’m just worried this isn’t feasible in the new editor but that chance is so small that we might as well just go with it

Anyway quick question: if a property is set to undefined on an object and we change it to not be present in the cases where it is undefined is that breaking?

copper laurel

Uhh, probably

Since you're changing what Object.keys/entries would return

outer raven

wahh

Aight

ruby terrace
outer raven

Yeah ai actually stumbled across that issue on mobile but we can’t really worry about that

junior pumice

Are there any plans om removing client.api access?

unique axle

Client#api is an internally used private and unsupported method and is not subject to semver, so it might change at any point without notice

junior pumice

but there arent any plans to lock it down like classes

copper laurel

There isnt really a way to "lock down" a specific propertyu

We could try JS's new private fields but im not sure what compatibility on those is like

junior pumice

good

bc i use it a tone

unique axle

once those are reliable, definitely going to do that

junior pumice

Awesome guess ill have to switch bc my whole bot is relying on those

ruby terrace

If you're relying on client.api you should really be using a package like @discordjs/rest

unique axle

if your whole bot relies on raw API access i'm not too sure why you even use discord.js to begin with?

junior pumice

and the client.api

junior pumice

is there also a thing like for the classes

copper laurel

..yeah.... discord.js

use it properly

junior pumice

I use it properly

copper laurel

Clearly not or none of this would be an issue

Not really a library discussion anymore

junior pumice

Okay then, just sad that everything more advanced people use is getting locked away

unique axle

if you were to elaborate on your use case we could maybe understand what you are doing and suggest changes/adapt to make that possible
but since all we get is

Okay then, just sad that everything more advanced people use is getting locked away
over and over (even after explicitly asking for those use cases, yesterday) we really can't do anything productive with this feedback

junior pumice

Okay sorry i didnt mean to do that, lemme explain

copper laurel

Its not "more advanced" though, if you want to make raw API calls you can just use node-fetch

Using discord.js's internal methods does not make you more advanced

junior pumice

So I've made my bot with djs in v12 and back then interactions weren't a thing so I made my own classes and stuff using client.api and to use some useful djs stuff. I used e.g. the message class and invoked a message with the message data given with e.g. message context interaction or the message collector from textchannel class by invoking it with the interaction channel
I use it so frequently that I would need to change 50 of the bot to bot use the classes and I use client.api even more

junior pumice
copper laurel

But now they are classes and you dont need hacky workarounds

junior pumice

Changing to those would be even more work than removing the djs class usages

wild flax

Which is unsupported and always was

So saying we “take away” things us untrue

It could just break

Or it might not

But that’s your own fault at this point for “I’m too lazy”

junior pumice
wild flax

What was?

copper laurel

client.api has never been documented

wild flax

The classes? Because of extended structures when we had them

Not to create them yourself

junior pumice

Fine about client.api since discorjs/rest exists

wild flax

You got your answer

junior pumice

Yup

outer raven

@tacit crypt in https://github.com/discordjs/discord.js/pull/6790 I changed both ApplicationCommandOption and ApplicationCommandOptionData but I'm not so sure about the first one since all options have all properties (yes, even channelTypes) and some just have them set to undefined. Should I update the Option interfaces to include this or do we just pretend like they aren't there?

tacit crypt

You only changed SubCommand(Group) options

I don't understand your question

outer raven

well yeah but I changed both the Option and OptionData interfaces

i just said it that way because the actual names are too long lol

tacit crypt

I still stand by my I don't understand your question. You fixed the issue in this PR. If there's other issues, make another PR?

outer raven

in that PR I removed the required property completely from sub command and sub command group options, but this property is actually present, it's just set to undefined on these types of options

tacit crypt

If it's set to undefined it won't even be sent to Discord

So...like...??

outer raven

no like

there's 2 different interfaces, one for data we send to discord and the other one for options we receive

and I'm talking about the options we receive

tacit crypt

We wouldn't receive required??

outer raven

but we create it that's the thing

that's why it gets set to undefined

hope that makes sense

tacit crypt

That's an oversight on our part, tho it doesn't affect it dramatically because JSON.stringify skips undefined props

But yes, you can submit a PR to fix that I suppose

outer raven

to fix types by adding undefined or to remove those undefined props?

because that would be a semver major right

the second option

tacit crypt

If it's strictly internal code that doesn't interact directly with the users, fairly certain it's not major but citation is needed

and it wouldn't be major anyways because its a bug that they're present but undefined

so

Like

outer raven

yeah I feel like it wouldn't be since the public methods always return values and not the full option (except for get)

maybe that exception is what makes this semver major

tacit crypt
outer raven

alright then, will do that

thanks!

also gonna address your review in the toString pr in a bit

is there any difference/preference in using Boolean() over !! ?

tacit crypt
outer raven

yeah seems like !! isn't used anywhere so I'll go with the Boolean constructor

actually, ckohen suggested using only Boolean in the filter but I think this would be more verbose, or does it not make a difference?

vs

wild flax

makes no difference here

outer raven

Alright, will just use Boolean then

visual hornet
outer raven

well Boolean does exactly that so ¯_(ツ)_/¯

visual hornet

it is for readability isn't it

outer raven

I think so too but crawl said it makes no difference

outer raven

say for example a STRING option with no choices, should it have choices: undefined or should it not be present at all

tacit crypt

Ideally, the former, but even if it's the latter, it still won't be "present" once we send it down the pipe

outer raven

aight I'll just default it to an empty array then, since the types and docs never included undefined

actually, can I?

yeah seems like that's fine by the API

tacit crypt

No?

Leaving aside the fact I mixed my former and latter this time, if a user doesn't specify choices it shouldn't be sent period

Hell, YOU asked if it should be undefined or not present and now you forcefully make it present??

outer raven
tacit crypt

if the property is not required, it shouldn't be present PERIOD unless the user set it

outer raven

Alright then

tacit crypt

Thats how builders and most of our patch methods do it too

Send just thats needed

outer raven

But here I’m talking about the received options in ApplicationCommand#options

Well both that and the options sent to the API in this case but I guess it still applies, just making sure we’re on the same page here

tacit crypt

Ok, maybe you need to explain a tad better because I don't understand what the issue is still

Code samples would be 🙏

outer raven
outer raven but we create it that's the thing

This is still the best example I can provide
My goal is to remove properties from options that cannot have them (e.g required in subcommands) but keep them if the type of option allows them
If the prop is allowed and not set by the user then it could either default to an empty array or not be present in the object at all, which is what I’m asking you

For context I’m editing the ApplicationCommand#transformOption method, which changes the ApplicationCommand#options property and the options sent to discord

tacit crypt

I've already said, if it's not set by the user it shouldn't be present

(I mixed up my former and latter but thats a different issue)

outer raven

Alright, will go with that then

Thanks thumbs

outer raven

Does APIApplicationCommandOption from discord-api-types not support channel_types?

dawn merlin
outer raven

ah it should probably be released then, was gonna use that in a PR

it's only a return type for a private function though, nothing urgent

tacit crypt

One day I will make a workflow that does a full release every friday

That day is not today

KEKW

outer raven

do you make commits every week though?

tacit crypt

Does it matter? Thats not even part of the idea meguFace

outer raven

oh idk then, was just thinking it could make duplicate releases that way

zenith oracle

Why wouldn't it be an array 🤔

outer raven

There are a lot of type checks though and when the error isn’t explicit (same when not passing an array to embeds or components in MessageOptions) it should probably throw a custom one

There’s some errors that are fine as it is but when the error references a function in the lib people tend to think its a bug and not an issue in their code

Problem is finding all these instances but I think at least the most common ones would be good enough

outer raven

is there any reason why the Channel#delete method doesn't support a reason when the GuildChannel and ThreadChannel methods do?

tender field

because DMChannel doesn't support a reason, does it ?

outer raven

so it theoretically does

zenith oracle

Why should you provide a reason for DM channels though? There isn't an audit log lol

outer raven
opaque vessel
vernal adder

Should be done.

opaque vessel

It seems there's also a repeated instance of this in the ChannelData type definition >:

visual hornet

isn't there like a massive json or something that's for generating the docs
couldn't we run smth like /(\w+) \1/ on that

opaque vessel

The error will still be in the source code. I don't think that's appropriate to do

visual hornet

well yeah but we could find where it's from from that, no?

opaque vessel

Oh you mean for finding duplicate words

Sure, I guess, you can try do that

visual hornet

yeah, just needs a human filter

ill try that later ig

opaque vessel
burnt cradle

The double for is actually grammatically correct there but if you make it a single for it's still correct just a slightly different meaning

vernal adder

"The properties to check `required` for."?

That has a different meaning

How about "The properties for `required` to check for."

visual hornet

got 4 matches, both on stable and main

  1. WebSocketShard#setHelloTimeout (private method) parameter desc time
    "If set to -1, it will clear the hello timeout timeout"
    https://github.com/discordjs/discord.js/blob/stable/src/client/websocket/WebSocketShard.js#L498
  2. seems correct (the one jiralite just mentioned)
  3. ReactionCollector#create (event) desc
    "...as opposed to Collector#collect which which will be emitted..."
    https://github.com/discordjs/discord.js/blob/stable/src/structures/ReactionCollector.js#L72
  4. "the the" that jiralite initially mentioned
vernal adder
visual hornet

"hello timeout timeout" is correct?

vernal adder

That is my question, yes.

visual hornet

idk i was debating if it was, but it doesn't seem like it to me
i don't think adding another "timeout" there really helps

in the code for that method it just says "HELLO timeout" as well

vernal adder

I think it's more "correct" to have them both, but having just one works as well.

Or, actually...

opaque vessel

I don't think we should have double words, correct or not. It's just... ambiguous and seems like a mistake at a first glance. Sentences like that should be rewritten

vernal adder

To me it only adds confusion.

But yes, that was changed an hour ago.

visual hornet

other methods like the setHeartbeatTimer below it doesn't say "heartbeat timer interval" either, just "heartbeat timer" in the desc and "heartbeat interval"/heartbeatInterval in the code

visual hornet
vernal adder

But it's not a timeout for the timeout.

It doesn't clear the timeout's timeout. It just clears the timeout.

opaque vessel

Literally below that is a debug message "Clearing the HELLO timeout.", it's not correct and is already inconsistent

The extra one should be removed

visual hornet
vernal adder

I believe I already included all of those

visual hornet

nice

opaque vessel
unique axle

constructive criticism usually does not involve insults
however, please note that we abide to semantic versioning, the API will stay consistently usable within patch and minor versions.
semver major (11.x -> 12.0, 12.x -> 13.0), however can include breaking changes

you can generally not rely on functionality to stay the same when doing a major update, in no library
if we were to stick to always make our versions backwards compatible we'd throw any chance for innovation and fixing up design mistakes or change the direction the library takes

if you want a consistent API stick to version 12, however, please note that new features will not be ported back to old versions and that discord will eventually (no known date) shut off the version of API version 12 uses

that is precisely why they are introduced in semver major version changes, because they are not minor changes and break the public facing API

now, as to why these changes have been made:
v11 -> v12: optional parameters have been moved into objects for consistency
v12 -> v13: we removed an inconsistency, no other delete method ever had a built in timer + this one was broken beyond repair, with more and more introduced checks against edge cases of when a message may have been deleted already

proven mountain

I think a good thing to think about is that I would assume none of it is done out of fun, cuz like I don't think any of the devs find entertainment by breaking other peoples code, changing discord API version can break certain stuff which may require for stuff to be changed, it may also be to code new code more efficiently.

I get it, it's really annoying to update a large portion of your code just because of breaking changes, but that's kinda to be expected when upgrading a major version, this is something your gonna find happen more times with both this library and others, but fortunately developers leaves this big changes for major versions so we can prepare more for these changes.

wild flax

I dunno, we usually give people months and months to update

If over a year isn't enough, then I don't know what to tell you

kindred lagoon

Why in each new version do you make drastic changes to the package and these changes are just a change in functionality and perform the same function as the first version? Why don't you just stick to each version and add features and if there is a modification to the Api from Discord, just implement the modifications and not modify the way the function is created, I know everyone agrees with me on this point
@wild flax@unique axle

clever spoke

theres usually good reason to most of the changes to djs, like consistency and whatnot

visual hornet

drastic changes
because that’s how semver works?
if it wasn’t drastic, it would be on a minor version

kindred lagoon
clever spoke

well could you give an example of a "drastic change" because changes that arent backwards compatible go into major versions (https://semver.org/)

visual hornet

nor are the contributors required to not make changes
updates to the library are to utilize new feature of js or node, to comply with the discord api, to create consistency between other parts of the library
we’re on major version 13, semver says that this means there are breaking changes, if we didn’t have drastic changes we’d be on 1.45.7 or something

kindred lagoon

This is the explanation of the program version if you do not understand it well
@clever spoke

Major version numbers change whenever there is some significant change being introduced. For example, a large or potentially backward-incompatible change to a software package.

Minor version numbers change when a new, minor feature is introduced or when a set of smaller features is rolled out.

Patch numbers change when a new build of the software is released to customers. This is normally for small bug-fixes or the like.