#archive-library-discussion

25085 messages · Page 3 of 26

real jetty

otherwise nothing is set and it remains undefined

copper laurel

Yeah

Is that an issue?

real jetty

i don't know, you tell me?

I just think null would make more sense

copper laurel

I mean, I dont believe that it is

real jetty

and be more consistent with other properties

copper laurel

But it is consistent with the others

Nearly everything in _patch has an if in data check

real jetty

huh - i always thought by default it turned to null

my bad if thats not the case

oak quail

discord sends bot and system the same way: either undefined or true

discord.js now makes bot true or false

but system hasnt been updates, so its inconsistent

real jetty
oak quail

we're defaulting bot to false, so we should also default system to false, for consistency

real jetty

yeah that makes sense - i'll probably pr it when i can

vernal atlas

i didnt set system to null in my PR because i assumed it would've been a breaking change

although now that i think about it the docs say it should be null

typings say undefined docs say ?boolean which is boolean | null

snow crypt

this seems to be happening on every pr sent

vernal atlas

yeah i noticed that the other day, i suppose someone can make a PR to ignore it or make a getter that is this.client.options.http.api

lofty beacon

Suggestion:

When calling the fetch method, you can pass in an argument as false and It won't cache whatevrer you fetch yet it will return it.

raven juniper

that's already a feature on some, if not all, of the fetch methods

?docs client.users.fetch

rich iglooBOT
raven juniper

although on other managers (roles, channels, etc) i don't think it's there, but they're usually assumed to be cached anyway

lofty beacon

thanks! I might work on a PR fo it tomorrow.

ornate topaz

on what exactly

lofty beacon

no caching of other collections for the fetch method

ornate topaz

but why? library assumes that it's always there (and it's actually always there until you forcefully remove it)

due to that there is no reason to specifically fetch role or a channel

lofty beacon

oh ok. nvm

plucky jay

In the typings, should StringResolvable be string | number | boolean | bigint | symbol | any[] (primitives except undefined and null + any[]) instead of any? If content is an object in TextBasedChannel#send, Message#reply etc., it gets used as the options and won't be sent: https://github.com/discordjs/discord.js/blob/master/src/structures/APIMessage.js#L342-L345

Currently, there are no editor suggestions for MessageOptions in TextBasedChannel#send, Message#reply etc. because anything can be used as content. It also means that typos such as channel.send({embed: {tilte: 'Title'}}) do not get reported because it's still a StringResolvable.

I know this would be a breaking change, which is why I'm discussing it here.

copper laurel

Typings arent breaking changes, assuming thats all you plan to change

vernal atlas

i think it would make more sense to change what the first parameter for TextChannel.send is in the typings rather than the actual StringResolvable type

plucky jay

Yeah true because StringResolvable is also used in Util.resolveString

vernal atlas

yeah

anything with a typeof of object (aside from an array) is treated as MessageOptions | MessageAdditions

if no options are provided through the second argument*

real jetty

it appears that available can be undefined from what ive seen

oak quail

i mean

honestly there are probably some mistakes in discord's docs for whether properties are optional or not

real jetty

theres no harm in making it constantly boolean value

even if there is no mistake it doesnt effect anyone

but if it is optional it defaults to false

i too would not be surprised if there were mistakes in discord's documentation

vernal atlas

those properties seem to be sent even if they are false

vernal atlas

the docs (typings too probably) would also need updating for Emoji#animated and Emoji#id

animated isnt documented as possibly being undefined, and id is marked as ?string which implies null, but its infact undefined

should Activity.syncID be documented, its the track ID for a spotify status, except syncID doesnt really give that away so maybe under a different name?

snow crypt

This appears on every PR

vernal atlas
snow crypt

oh i see

thing is

when i run npm test it doesn't even pop up

but that might be because of errors because of prettier/prettier

which i have no idea why they don't show on GH

vernal atlas

oh wait did i just

k i facepalmed there, api is meant to be a string

but is there still an issue there? doesnt seem like its meant to iterate over the string

real jetty

Hey sugden can you read my comment to your PR review when you can, thanks PES_Heart

vernal atlas

sure

regarding that pr review comment ^ are the jsdocs incorrect for properties such as Guild#approximateMemberCount? as its documented as ?type rather than type|undefined, because ?type indicates type|null as per https://jsdoc.app/tags-type.html

real jetty

if thats the case then a lot of things are wrong in djs

such as <Guild>.widgetChannelID

and Guild#widgetEnabled
you get the picture, this is the case with a lot of properties

copper laurel

?type should indicate that something is nullable. We shouldn't have any documented properties be potentially undefined imo

real jetty

i agree that if something isn't sent by api it should be nulled but that's breaking
at this point the best solution is probably just to go with ?type even for undefined properties

unless we change literally every single one to type|undefined

vernal atlas

?type should indicate that something is nullable. We shouldn't have any documented properties be potentially undefined imo

yeah thats what i think would be the best approach, except it would be a breaking change to fix that accross the board

copper laurel

v13

vernal atlas

well for v13 / djs-next obviously yea, but as for v12 at the moment

real jetty

for now i reckon just making the JSDoc consistently wrong rather than occasionally right and inconsistent is better

because this is the case for pretty much every property which can be undefined

vernal atlas

i think the jsdoc is a bit of a mess

real jetty

the approach that i take in the pr is the best we can do for now afaik

vernal atlas

types are correct for the most part i think but its just the little parts like this

copper laurel

Yeah but I think that like

?type will do, it indicates that it might not be there

real jetty

yeah exactly

having 2 properties typed correctly and 100 not is just more confusing

i resolved the conversation since its pretty clear we can't make the changes you suggested sugden, at least not for now

vernal atlas

yeah for now it cant be done, maybe if this is an issue it can be implemented with another PR

paper furnace

can you explain me the choice which lead to te poorly designed partials feature?

why we can't have simply an event like raw previously dedicated to partial instead of dealing with them for each declared listener ? this is completely insane

while writing this part of the doc, nobody thinked: oh, finally it's maybe a bad design?

WARNING

Partial structures are enabled globally. You can not make them work for only a certain event or cache and you very likely need to adapt other parts of your code that are accessing data from the relevant caches. All caches holding the respective structure type might return partials as well!
radiant glacier

you can still use the raw event

in v12

paper furnace

nope, my IDE tell me this event don't exists anymore (using typescript)

maybe this event is missing into the typedef

the in fact partial would be incredibly more usefull if we can bind then to a custom event instead of using them globally like that

radiant glacier

well it does @paper furnace

paper furnace

mhhh really weird

lofty birch

it's not shown in the typings as you are encouraged not to use it

same as v11, wasn't shown there either

and partials are very helpful, it's as simple as adding if(<x>.partial) await <x>.fetch(); wherever a partial may occur

vernal atlas

raw is undocumented, because it emits the raw websocket data

slate nacelle
cold basin

would it be handy to add a guild property to DMchannels

which is actually the user

slate nacelle

There is recipient, isn't there?

cold basin

and dont keep asking

real jetty

its easier to tell them guild is null in dms

because it actually makes sense

cold basin

¯_(ツ)_/¯

ornate topaz

no, because guild would no longer mean guild

cold basin

thats true

ornate topaz

so you actually could not do message.guild.roles or anything similiar

real jetty

that just makes things even more confusing

cold basin

yeah ok fair enough

oak quail

honestly, imo its ok (but a bad idea) to not document the raw event, but its really dumb to not put it in typings

unique axle
oak quail

imo the typings should cover the entire library

real jetty

yes but thats not a public aspect of the library

haughty tide

Why don't you create a separate typings file and point somewhere that this file exists for those who want to use the whole power of the library?

ornate topaz

"whole power"? you mean "unrealiable power that can change at any point without any warnings, as its undocumented"?

copper laurel
tacit crypt

I mean, we can type the raw event as "unknown" so it's up to you to either type it or use a typings library for it shrug

raw is this middle-ground between undocumented and documented, it's a slightly more uh.. raw than what Souji linked, and what you should most likely use (cause raw includes things like the sequence and op and stuff, which for most is really NOT needed, however YMMV)

fallen arrow

i have a question about TextChannel#startTyping, from what i understand it was built so the promise resolves once the bot finishes typing, however it seems a little counter-intuitive considering all other api methods resolve as soon as the request returns a response. Doesnt this prevent the user from properly awaiting it for example at the beginning of a command?

for example, you cannot do the following because startTyping will never resolve ```js
await message.channel.startTyping();
await message.channel.send("test");

unique axle

correct, the promise will resolve once the typing has ended.
correct, awaiting it like this will wait for the typing to end before continuing, consequently never continuing

snow crypt

can we add an option to not wait for stopping to type?

fallen arrow

currently the only alternative is not awaiting, which introduces a possible race condition

tacit crypt

IIrc you can pass a count too

snow crypt

?docs textchannel.starttyping

rich iglooBOT
unique axle

i don't really see the use case tbh, and passing a count does not change the behaviour of the returned promise

tacit crypt

well, passing 1 WILL make it return

snow crypt

but also end typing

unique axle

if you pass 1 the exact same happens, it stars the typing indicator and the promise resolves when the adequate count (1) of stopTyping has been called

snow crypt

well, what about the option idea?

fallen arrow

the use case is being able to ensure the typing indicator has started before proceeding with the command, otherwise there is a possible race condition where the command's response will be received by the api before the typing indicator, which makes the bot send the response first and start typing afterwards

tacit crypt

unless someone PRs a sendTyping (which is just await this.client.api.channels(this.id).typing.post())... Nothing stops you from using the raw API

fallen arrow

sure, there is always a workaround

vernal atlas

wouldn't be too difficult to perhaps create a function that listens to the typingStart event

for yourself - not in the library

but perhaps a wait boolean parameter for the sendTyping function could work?

which would of course determine if the promise should wait until.the typing has finished

does the count parameter even... work? i don't see it being decremented anywhere in the startTyping function and testing a startTyping(1) doesnt seem to resolve

ok yeah im either stupid or the count parameter doesnt even do what its supposed to

real jetty

Maybe adding an actual error for when you try to ban a guildmember while not providing an object as options would be nice 🤔, for now it gives an Internal Server Error which gives the user no information

oak quail

might be a change due to the new update, as djs updated to the new preferred discord behavior which is iirc sending a json object instead of query params

copper laurel

Yeah, the change mentioned above resulted in the API returning an error when invalid data is passed instead of... whatever it did before, possibly resolving a ban but not passing the reason
The error would be the same as the TypeError for Message#delete
I agree the response isn't meaningful to what they did wrong, but having said that, if they bother to read the docs and use the ban method correctly it won't be encountered

vernal atlas

should Client#generateInvite have options for other query parameters for the invite other than permissions? eg preselcting a guild, redirect URI etc

unique axle

i'm not too sure what you mean there with the count parameter not working?
.startTyping(1) -> .stopTyping()
.startTyping(2) -> .stopTyping() x2
.startTyping(3) -> .stopTyping() x3
and so on. i tested that up to 5 the other day, could not see any misbehaviour in the counts

discord.js heavily diverges from how typing works api wise, we keep the typing indicator alive as long as not all typing "instances" are resolved (the count is decremented through stopTyping to the point where none are left) and sending a message does not resolve the call, but has to be done manually.

and as was pointed out above you can't await the start call as is right now, since it resolves only after the typing is fully resolves (has stopped)

unique axle

should Client#generateInvite have options for other query parameters for the invite other than permissions? eg preselcting a guild, redirect URI etc
https://discord.com/developers/docs/topics/oauth2#bot-authorization-flow sure, the redirect is really just useful for the non-invite generation, unless i'm missing something?
You'll also notice the absence of response_type and redirect_uri. Bot authorization does not require these parameters because there is no need to retrieve the user's access token.
Since all are optional probably best with PermissionResolvable|object and deprecating the first.

vernal atlas

sweet

so do you have to actually call stopTyping?

i was under the impression the route was called count times, and resolved on the final call

snow crypt

what are the marked ones for?

ornate topaz
wild flax

Those are all legacy and we’ll prolly not use them anymore except for v12 @snow crypt

snow crypt

so is the current repo gonna move to a legacy repo when next goes live?

next being the typescript one

wild flax

no

next will be merged into the current repo

but the tags/labels won't be the same most likely

velvet tide

so i realized the docs and typings dont have syncID for the Activity class. I decided I will make a PR because it'll be useful. What exactly should I put for syncID's jsdoc comment?
Can anyone provide input on this? i did some further digging and found this PR related to it https://github.com/discordjs/discord.js/pull/2314 with the commit message spotify stuff. So should I put the comment as The ID of the Spotify track?

should Activity.syncID be documented, its the track ID for a spotify status, except syncID doesnt really give that away so maybe under a different name?
@vernal atlas my apologies for pinging you, but i'm in the same boat as you are and hoping to get a conversation out of this as well

velvet tide

so it seems the reason its not documented is because Discord API docs doesn't even document Activity#sync_id because that's their intention? ( https://github.com/discord/discord-api-docs/issues/1239#issuecomment-563425694 ) but I'm also reading this issue comment ( https://github.com/discord/discord-api-docs/issues/545#issuecomment-370604860 ) that sort of backs the previous one, but it's been 2.5 years since that comment has been made and I believe the Spotify integration has matured since then? Someone even questioned the relevancy of the property here ( https://github.com/discord/discord-api-docs/pull/436#issuecomment-349790557 )

So overall, does this go up one level higher to the Discord team to consider adding this? For now, I ended up extending the Activity interface myself and defining that missing property. I'm in limbo on it in general for submitting an issue to discord.js, but potentially the Discord API docs repo instead? Thoughts?

unique axle

undocumented fields might change name or functionality at any point without further notice. due to that discord.js just implements (and accepts PRs for) upstream (discord-api-docs) documented fields. If you get them to document it it's relatively surely going to be added.

* we can't promise semantic versioning if we introduce a change that might result in breaking functionality, discord mostly doesn't break documented fields

velvet tide

I see. I think I'll go upstream with this. Thanks for the input @unique axle

glad sparrow

what will Collector use if there is no time (in collector options)

unique axle

this is not a support channel, whatever other ending criteria you give it, if none - none

vernal atlas
dapper leaf

are there any plans of porting discord js to deno in the future many people have made their own attempts but they lose support in a month or 2

unique axle

no current plans, has not been discussed - might become easier after the planned typescript rewrite or be considered during it

dapper leaf

ok

copper laurel

@vernal atlas not sure what you're pointing out here honestly - what are you suggesting it should extend?

vernal atlas

you can extend the classes Presence and TextChannel via Structures.extend, however ClientPresnce and NewsChannel extend those classes (respectively), but they don't extend the custom-made extended class

a bit like how ClientUser extends User

copper laurel

Ahh gotcha. So you're suggesting they should extend the extended Structure, if that's even possible

vernal atlas

the ClientUser export is a getter so it can properly get the User class from Structures.get

yea

vernal atlas

well

all channel classes extend Channel

lofty beacon

I had a look through the events on the client and there isn't one for when messages are swept, so could that be added?

This would also work when calling client.sweepMessages() along with the automated interval system.

real jetty

the debug event emits whenever messages are swept. it's quite crude, but you could filter through debug messages (i.e. not the WS ones) then emit your own event based on that

vernal atlas

you could also extend the client class and override the sweepMessages function to emit an event 👀

lofty beacon

oh yea lol

also, there isn't such thing as a message.quote, might be cool.
ex:
if a message had the content of hi and you called the quote method with hello as the content it would resovle this:

> hi
@user hello
ornate topaz

what would be a use case for that

unique axle

As discord themselves are planning to remove that feature in favor of proper reply (which may or may not get proper bot API support at some point) I don't personally see much value in this

lapis talon

Yeah that new quoting thingy is pretty epic

vernal atlas

the API seems to have stopped sending embed_enabled causing Guild#embedEnabled to always be undefined, rather than a boolean as it should be, should this be left as-is or perhaps changed to use the widget_enabled value?

wild flax

Considering we can’t just up v13 for this, yes

The latter

vernal atlas

O wait- embed_enabled/widget_enabled doesnt/never come with the initial GUILD_CREATE packet

but it perhaps should be cached with the fetchWidget/fetchEmbed method, ill pr that

oak quail

has that changed, or was it always like that?

vernal atlas

thats what puzzled me at first because i could've sworn it was sent with the initial packet

oak quail

i'd make an issue on discord-api-docs

vernal atlas
tacit crypt

Guilds do not store this metadata, so it isn't returned in guild creates

GUILD_UPDATE contains them

logic 200

vernal atlas

would it make sense to make fetchEmbed just return this.fetchWidget() to avoid duplicated code?

tacit crypt

Ye, and mark as deprecated

vernal atlas

ye, already deprecated, the same for setEmbed too?

they both hit a different endpoint, guilds/:id/embed and guilds/:id/widget, but yield the same result, which is why im asking first xd

oak quail

/embed is removed in api v8

which is why the djs embed stuff was deprecated

vernal atlas

BlobThumbsUp

snow crypt
vernal atlas

im tryna implement the changes from https://github.com/discord/discord-api-docs/pull/1886/files into a PR - but the short of it is should i make a new class for IntegrationAppliation or just use the ClientApplication class

making a new class would essentially just be the same as ClientApplication, but without botPublic,botRequireCodeGrant, cover, owner, and rpcOrigins

perhaps an Application interface/base class?

i think that would actually be a good idea

snow crypt

@vernal atlas why not have includeApplications just true by default

like, not even an option

just always true

vernal atlas

well, i thought it best to add the option & default it to what discord defaults it to

oof just made me realise i forgot the typings

snow crypt

@vernal atlas well they could have done the same thing with webhook ?wait

but i really wonder, why not

it's not like it will be breaking

or cause anything unpredictable

i say just have it always true so you always have the application

vernal atlas

perhaps default to true, but still have the option if you need it

snow crypt

why would i explicitly NOT need it?

yeah i might not really have a use for it

but in what case is it gonna be "it must not be sent"?

vernal atlas

well, it would be easier for a user to filter out bot & webhook integrations

snow crypt

can you explain?

vernal atlas

well, if for whatever reason you didn't want to list bot & webhook integrations (eg in a command), simply passing false would be easier than (await guild.fetchApplications()).filter(int => int.application === null)

snow crypt

ohhh wait my bad

vernal atlas

FuzzyEyes

did you think include_applications did something else?

snow crypt

yeah

oak quail

oldeyes PR #4762 was merged before the docs pr was merged

thorny copper

Why did you make separate methods for avatars (avatarURL and displayAvatarURL)? Why not merging the second one with the first one by making a parameter "callback", which could be an image (URL, buffer or anything else) or "default", which effectively returns default avatar URL?

warped crater

avatarURL yields the avatar given by Discord as is, null in case the user has none set
defaultAvatarURL gives back an asset from Discord's API with... a default avatar; calculated as they do it in the client
displayAvatarURL is simply a utility that either gets the user's set avatar, or their corresponding default (theoretically, what you should have on screen)

this design choice makes sense to me, and either way, it can not be changed as this would be breaking

thorny copper

(Yes I know this would break existing code, but why not a change for DJS v13)

wild flax

I don't see a huge benefit in merging them

Nor a huge downside in leaving them as is

warped crater

^ the way it currently is actually allows you to tell if the user has no avatar set

thorny copper
snow crypt

Util.resolveColor returns NaN instead of throwing.

unique axle

summary:
https://github.com/discordjs/discord.js/commit/0d4eab8d241e770cbea065688ccf85aa93fe07d1#diff-fb4312d30636f670d026660847d06636R368 https://github.com/discordjs/discord.js/commit/b1473b1e4cdecc782ead80c06ff447d94479e5e4#diff-dd560c495a00593ea828a458e5e13b18R115-R118

  • has this behaviour since Jan. 2017 Dec. 2016
  • potentially breaks build pipelines for people using it that way
  • inconsistency is bad (it should probably throw)
  • still erring towards semver:major which makes this v13 earliest

thanks space for digging shibaHeart

slate nacelle
halcyon field

Add something like .bannable but for ".dmable" (checks if the user can be dm'd)

copper laurel

Discord has previously said no to this request anyway. They prefer to have the API call to send the message reject than have developers making two API calls for every DM - check + send.

deft holly

Think this has been asked several times here but with no response 😕 what's the reason https://github.com/discordjs/discord.js/pull/2910 hasn't been merged? Does it count as a breaking change in some way (please excuse my ignorance on this topic :P)

vernal atlas

its got a semver: minor label on it, and there dont seem to be any breaking changes, so no its not a breaking change

crimson harbor

Sorry for a dumb question, but I'm not sure how to use util.deprecate on a class method.

Nvm, I figured out that you use Class.prototype

copper laurel 
GuildMember.prototype.hasPermissions = util.deprecate(GuildMember.prototype.hasPermissions,
  'GuildMember#hasPermissions is deprecated - use GuildMember#hasPermission, it now takes an array');```

Theres an example from v11

patent halo

How does discord.js handles rate limit internally? PepoHmm

left moat

check the code

patent halo

i dont get it, somewhat

patent halo

Ahh, like a queue

cursive shuttle
cursive shuttle

Similarly, Discord always emits GUILD_BAN_ADD with a full user object, meaning guildBanAdd will never emit a PartialUser even though it's marked as such in the typings

Again for GUILD_BAN_REMOVE

newUser in userUpdate shouldn't be partial either as userUpdate always includes a full user structure

fallen arrow

currently there is no way to define guildsPerShard when using internal sharding set to "auto". Can i PR a client option for it?

well i guess one could always manually call Util.fetchRecommendedShards and feed its result to the client, so nvm

vivid field
slate nacelle

I'd say that's a bad autocomplete of my editor and should have been this.

vernal atlas

PR'd it

would it make sense for ChannelManager and GuildChannelManager to have overloads for the resolve function with a type parameter, so you could use
guild.channels.resolve<TextChannel>(...) or similarly, client.channels.resolve<DMChannel>(...) for the client

copper laurel

How would that handle the ID not being a text channel ID though

vernal atlas

well, this would be similar to just casting it yourself, except its just a little cleaner for typescript

snow crypt

might as well just <TextChannel>client.channels.resolve(id)

vernal atlas

ig

drifting knot

so collection.filter doesnt do great with typescript

import { A, B, isTypeA } from '../classes';

const collection = new Collection<A | B>();

// ...

const aCollection = collection.filter(item => isTypeA(item));

// aCollection is still typed as Collection<A | B>
// It should be typed as Collection<A>

can we use type guards or assertions or some other TS feature to make the return type match whatever the filter predicate is doing?

i'm asking because i don't like having type assertions for things like filtering a bunch of channels to just be voice channels

deft holly

it's worthwhile to note that Array#map has the same behavior. I doubt there's a way to fix the typings for this in a sane way since

  1. you'd probably need TypeScript to infer the return type of a function (the predicate) when called with a specific set of arguments which isn't really possible currently
  2. Array#map would have been updated with proper typings if this were possible easily

the only alternative then is something like collection.filter<string[]>(predicate: ...) which would then result in a string[] but I don't think this a big improvement over just typecasting

drifting knot

thats a good point

i kinda like the idea of passing a type param to Collection#filter

but i can just do const a = whatever as unknown as Thing

deft holly

found a github issue about this. Does seem like it's not possible without explicitly typing the value though which is unfortunate - https://github.com/microsoft/TypeScript/issues/16069

update 2:
seems like supporting type guards can actually be supported through the following overload:
filter<S extends T>(callbackfn: (value: T, index: number, array: T[]) => value is S, thisArg?: any): S[];
(this is for Array but the concept can be applied to Collection as well)

basically, this would allow -
const foo = collection.filter((x): x is undefined => x === undefined);
foo is then typed as undefined[].

long basalt

I got something that feels like a bug, but cant tell, I also dont know if this is the reason its happening, im not a expert on the request handler. so the bug is, when multple items are in queue and a 400-500 error, it wont continue in the queue, just reject. in result, some stuff will just hang indefinitely until a request gets added to queue and it continues to handle requests until it runs into that error, im not fully sure this is why it happens, its just what I think is whats causing it, im also not sure if this is a bug or not

tacit crypt

Known, reported and will be fixed 👍

long basalt

is there a pullrequest or not yet?

anyway, I think im going to evade the issue by awaiting before adding another request to queue

random shell

is there anything special I need to do when importing the webpack version (I'm using 12.3.1) via a <script> tag?

it seems to always result in errors, so I feel like i am missing something

vivid field

about to implement the newschannel following feature, would targetChannel.follow(newsChannel) or newsChannel.follow(targetChannel) fit better into the structure of the library?

vernal atlas

i would go for NewsChannel#follow

crimson harbor

I think that NewsChannel#addFollower would be a better name.

ornate topaz

Imo that points slightly more towards a user than a channel

deft holly

typings: any plans to use named tuple elements (ts 4.0 feature) for the ClientEvents interface? benefit of this is that client.on('message', ...) - intellisense no longer shows args_0, args_1, etc. as the names for the parameters but rather the labels

For what I mean by this, see the following two screenshots. If you can't see the difference, it's args_0 vs name.

This may be a breaking change for TS users not on 4.0 though

oak quail

API/GW v8 changes:

  1. permissions is now back to the original field, as a string.
  2. game fields are now removed, replaced with the first element in activities. For widget endpoint, game field on users is renamed to activity, since there's still a singular field.
  3. Channel Permission Overwrites have their type field serialized as a number (e.g. 0) instead of a string (e.g. role)
  4. embed_enabled and embed_channel_id fields are removed, replaced with corresponding widget_ fields.
  5. Form error responses now have a format that is better aligned with "regular errors".
  6. Rate limit headers are sending seconds instead of milliseconds now. As such, the X-RateLimit-Precision header is no longer respected.
  7. Bots no longer get CHANNEL_CREATE for DMs on v8.
  8. Many routes have been deprecated. (Consult: https://github.com/discord/discord/pull/29722/files)
  9. Bans delete-message-days is no longer available. Use delete_message_days instead.
  10. Removed roles, premium_since, and nick from Presence Update gateway event.
vernal atlas

that link is a 404 page ASC_PepeThinking

unique axle

of course. that's not discord API docs, that's discord/discord it's not public.
that information is a direct copy from the DM of mason with one of their engineers

vernal atlas

yeah figured the repo wasnt public, just wanted to point that out incase it was meant to be a different link

copper laurel

@deft holly technically speaking we don't consider the typings to be breaking changes since its a JS lib, however one that major is probably worth holding off on. discordjs-next is a TS rewrite though so we can definitely make use of it there

vernal atlas

well- maybe that property isnt the best example but im sure theres others
edited to a better example

tacit crypt

Eh. I just document the types, how they come is not something i should spend time documenting... Plus I link the direct object url so people can read when using it

vernal atlas

oki

tacit crypt

Besides.... Think about it this way: properties can come and go at random from different places... And may even be changed randomly, requiring a patch release for it

oak quail

well

owner and permissions are based on the specific context of "get current user guilds"

because its "is this user the owner" or "what perms does this user have"

real jetty

Is it not possible to add the "GuildMember" who triggered an event on events such as channel create/update/delete as a parameter?

unique axle

we can only implement features around data discord provides, this is not provided by discord

"who did something" will always work through audit logs only

real jetty

Can it not be pulled by the lib through audit?

so the user doesn't have to do it for each event?

unique axle

the library does not auto-fetch data.
additionally audit logs are not guaranteed to have arrived when the websocket event does
(there's also a bunch of other caveats, which i won't get into here)

so: no, we can not do that

mint iron

What the utility for each folder inside packages in discord.js-next?

For example where should goes structures like Message, Guild...

tacit crypt

Those are probably gonna go in packages/discord.js

each folder is a submodule

for instance rest will host everything rest related

drifting knot

since new djs will target new node, can snowflakes use bigints please

idk if this is a "node doesnt support it" issue or a "we have been using strings for 999999 years" issue

raven juniper

Discord does provide them as strings on the API, so I suppose one question would be if we'd want to convert it, iirc we don't change any other API types

vernal atlas

i think there was a discussion about this, i remember it being said that it seemed silly to just convert back and forth between strings and bigints all the time

drifting knot

doesnt erlpack use proper int64s

also its like 2 operations

if its all written in TS it shouldnt be an issue

just call id.toString()

it will never fail

drifting knot

my user id takes up 36 bytes of memory as a string, a bigint would take up 24 bytes

for a large bot you could easily be saving 1-2 mb ram lmao

tacit crypt
drifting knot

how

surely this is because erlpack is still written as if bigints dont exist in electron or wherever they're running their code

copper laurel

Just because they exist doesn't make them a particularly good type to use

vagrant holly

Wait wait wait

ShardingManager and Shard cache eval calls?

Dang that's why sharding is so fucked

vagrant holly
frank turret

I know role#equals just compares properties from both roles, but is it really needed/within the scope of the library to provide a method like that? Even in the methods description it advises a person to just compare the ids for most use cases. I understand it would be a breaking change, but would it be worth opening an issue/making a PR regarding removing it?

oak quail

its for internal checks

frank turret

Fair enough, my next topic is out of the list of v8 changes (for discord-api-types)(https://github.com/discordjs/discord-api-types/issues/12#issuecomment-692972229), I made a sub list of which changes are actually related to the issue itself. Could someone look it over and inform me if I'm mistaken about anything?

APPLY:
>permissions is now back to the original field, as a string.
>Channel Permission Overwrites have their type field serialized as a number (e.g. 0) instead of a string (e.g. role)
>Removed roles, premium_since, and nick from Presence Update gateway event.
>embed_enabled and embed_channel_id fields are removed, replaced with corresponding widget_ fields.

DONT APPLY:
>game fields are now removed, replaced with the first element in activities. For widget endpoint, game field on users is renamed to activity, since there's still a singular field.
>Many routes have been deprecated.
>Bots no longer get CHANNEL_CREATE for DMs on v8.
>Bans delete-message-days is no longer available. Use delete_message_days instead.
>Rate limit headers are sending seconds instead of milliseconds now. As such, the X-RateLimit-Precision header is no longer respected.
>Form error responses now have a format that is better aligned with "regular errors".
deft holly

About the stores -> managers change (yes, I know I’m several months late on this):
Have shortcut methods like XYManager#filter() (which calls XYManager#cache#filter() internally) been considered? From my (admittedly poor) understanding of the issues at hand (https://github.com/discordjs/discord.js/issues/3485):

Poor Inheritance
Since the design doesn’t really change from right now, I don’t see why this would be an issue.

The design is not extensible
As XYManager#filter(), XYManager#find() and so on simply call their respective cache methods internally I don’t see why this would be less extensible than the current approach either.

Naming conflicts
An example of a naming conflict before was MessageStore#delete vs MessageStore#remove (which deletes the message and which removes from the cache?). This would be solved by simply not creating an alias for MessageStore#delete -> MessageStore#cache#delete, i.e MessageStore#delete would make an API call and MessageStore#cache#delete would be used to remove the message from the cache.
I personally believe that the proposed change (shortcut methods) do not remove any of the benefits of managers without any significant drawbacks. About whether this change is needed – from the many, many users who complained about having to add .cache everywhere in their code, I believe that the majority of the user base would appreciate this change. In addition, as these are simply shortcut methods, no existing code has to be modified.

Looking forward to any thoughts about this, perhaps I’m missing something really obvious which prevents this from being a possibility in which case I apologize.

oak quail

@frank turret im pretty sure this will have to be changed too

game fields are now removed, replaced with the first element in activities. For widget endpoint, game field on users is renamed to activity, since there's still a singular field.

copper laurel

I both agree and disagree with what you're saying here @deft holly

The intention was to achieve a separation of API and cache - for that reason I'm not sure I totally agree that providing methods that call the cache from the Manager is necessarily the right approach.
However Managers themselves really don't act as a good API Manager yet - making API calls still relies on having the cached instances. For example, if you want to pin a Message, you have to get the Message object from cache, and call Message#pin.
A method like MessageManager#delete which lets you delete uncached messages by ID is a much better representation of how it should work imo (and Message#delete actually calls MessageManager#delete now internally, because the Manager is responsible for API calls)

This is something I'm working on literally right now now, providing much more extensive API coverage on the Managers so that they can fulfil their purpose, and better reflect that separation of "I'm making an API call" vs "I'm operating on the cached items" such as you would be with #filter.

deft holly

The intention was to achieve a separation of API and cache - for that reason I'm not sure I totally agree that providing methods that call the cache from the Manager is necessarily the right approach.
I agree that in an ideal world we would want to have this but the reality is that most Discord.js users do not really care about a separation of API and cache - no one (to my knowledge) brought this up when v12 was still using stores until the beforementioned issue was raised. It is a (admittedly mild) inconvenience to type .cache every time you want to access the cache, and I think having shortcut methods would mitigate that somewhat. I can absolutely understand not adding it though, as I personally don't find .cache to be too much of an issue :P It's just that I'm reasonably sure a lot of people would appreciate not writing it everywhere. It's just that I believe that Discord.js would provide a better experience for users of the library if not chaining .cache everywhere were an option (without causing more issues).

wild flax

While user experience is an important topic, I simply do not care what the "majority" does, when the majority follows youtube tutorials or does not know JS at a level enough to even write a complete bot by themselves

left moat

Also I would hope that a clientOption field is eventually introduced to change the cacheType of managers in v13 or in the next big v12 update

afaik this isnt a breaking change bc nobodies code will break because of it

unique axle

Making chaches be exchangeable is definitely on the roadmap, however more expected towards -next.
V13 will focus on adapting the existing js codebase (v12) to the new API versions.

vagrant holly

Gonna ask here cause I feel y'all might know better than the help channels, is there a good way to completely block a channel and its messages from ever being cached in the library (got a channel where each message has a solid 80MB+ of reactions data on it, so I'd rather just not deal with that channel ever)?

left moat

well you could just remove the bots perms to see anything in that channel @vagrant holly

vagrant holly

Not a channel that I have any control over at all, so curious if the lib has any decent internal way to block out a channel from caching at all

unique axle

please do not use this channel as an upgrade of help channels

no, you can not
you can sweep messages on an interval or modify source

vagrant holly

Is the raw processing of a new message exposed, or would the best bet be to just listen for a new message normally and then purge it from cache at that point?

If not, would it be worth looking at a way to add a custom callback/filter to a raw message arriving in the lib, or would that PR never get anywhere if created?

tacit crypt

Granular cache controls would be nice...but I don't think it'll be a priority for v12/v13? (don't quote me on that)
However, nothing stops you right now from creating a sweeper of sorts

undone ravine

messageCacheMaxSize is a thing also

vagrant holly

I have super strict sweeping set up for messages themselves with what the library provides in options, this seems to be coming from a small subset of messages that don't match the main sweeping criteria

vagrant holly

Is it possible to request an issue be assigned to me on GH?

vernal atlas

working on a PR to handle channel type changes (text => news, news => text), what would be the best way to handle this? just remove the channel from the cache and re-add it?

oh wait- its already handled

tender field

is there a plan to have better stacktraces in djs-next? I know it would require a lot of changes and i don't expect it to be on v13, but will it be taken into account for -next?

raven juniper

at some point yes, there will by async stack traces and will include the line of the code

DiscordAPIError[50006]: Cannot send an empty message
    at RequestHandler.makeRequest (/home/archid/workspace/alestra/node_modules/@klasa/rest/src/lib/RequestHandler.ts:192:11)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at RequestHandler.push (/home/archid/workspace/alestra/node_modules/@klasa/rest/src/lib/RequestHandler.ts:101:11)
    at async Promise.all (index 0)
    at Map.add (/home/archid/workspace/alestra/node_modules/@klasa/core/src/lib/caching/stores/MessageStore.ts:65:23)
    at default_1.eval (/home/archid/workspace/alestra/node_modules/klasa/src/commands/Admin/eval.ts:66:14)
    at default_1.run (/home/archid/workspace/alestra/node_modules/klasa/src/commands/Admin/eval.ts:24:43)
    at CommandHandler.runCommand (/home/archid/workspace/alestra/node_modules/klasa/src/monitors/commandHandler.ts:58:23)
    at CommandHandler._run (/home/archid/workspace/alestra/node_modules/klasa/src/lib/structures/Monitor.ts:110:4) {
  code: 50006,
  status: 400,
  method: 'post',
  url: 'https://discord.com/api/v7/channels/642137151626018818/messages'
}``` this is an example, you can see where it points to the command file where the occur originates from
tender field

incredible! did you write this from scratch or is this already available somewhere ? 👀 (like a branch or smthg)

raven juniper

it's not available yet, i'm not technically proficient enough to tell you where it comes from

i'm just a copy-paste monkey. i just know it's planned to be in an upcoming version of d.js, whether that's 13 or next, i don't know

tender field

ok thanks 😄 still really nice to see we might have better stacktraces at some point 🙂

left moat

djs-next/13 will most likely take design ques from klasa/core such as the ws/rest design

oak quail

well -next will be v14

but vlad said that v12.5/v13 might use the updated rest stuff

tacit crypt

Keyword might

unique axle

well -next will be v14
-next will be the next major at the point it's completed.

We might need to release another major before -next if discord decides to introduce breaking changes and release other versions of the API, which we might need to reflect to support new features

oak quail

@frank turret @tacit crypt for api-types, should we maybe keep most things only in v8 and just import&export them in v6 unless they're different

so not everything is duplicated

because theyre gonna keep updating v6

tacit crypt

Ehh.. dunno honestly
Personally I'd prefer duplication and once discord marks a version as basically dead, we delete it from the module

It'll be annoying sure.. but idk, depends what others suggest too

vivid field

shouldn't Client#emojis and Guild#emojis be separate managers? for one, the types are different (i.e. guild vs no guild) and also there's methods like create() accessing this.guild, wouldn't that throw an internal error when trying to do client.emojis.create()?

unique axle

DiscordAPIError: 405: Method Not Allowed does throw, correct

vivid field

so shouldn't the library handle that instead of sending an invalid request? just throwing an error when calling it from a client would probably be enough imo, but idk if that's seen as patch or major

unique axle

following the discussions here https://github.com/discordjs/discord.js/pull/4816#pullrequestreview-487356231 it'd be major (i'd also agree with that, it changes the "return type" of a function in the case of failure, which is then no longer a DiscordAPIError.

though as i mentioned earlier v13 will be needed to comply with new api versions and bring new features before the full rewrite anyways, so this might be the right time to propose a change here.
Maybe even on a larger scale, trying to spot and change similar occurences like this (though that could also easily blow out of scale)

solemn oyster

incredible! did you write this from scratch or is this already available somewhere ? 👀 (like a branch or smthg)
@tender field we'll likely do that for 12.5 (or 12.4 if Crawl doesn't release it before), the trick is that the new REST uses an async lock implementation, versus using stateful deferred promises

As such, the queue is done in a natural manner, and by design (unless V8 is bugged, which is not), it cannot deadlock, it cannot halt, errors bubble up naturally, the entire request can be retried entirely (adds support for retried AbortErrors!), and by design, it has proper async stack traces, which is done thanks to the fact that it does await this.queue.wait() before proceeding, it doesn't use middle steps nor makes an object representing the state, it just awaits for all the previous requests and then it jumps directly to the fetch()

A try/catch/finally tells the queue to resolve the first promise (wait obtains the last promise, it any, pushes a new deferred promise, and awaits the previous), it may be a little hard to understand, but it's really simple

But yeah, before I can use this, I need @tacit crypt to either give me async-queue's code, or upload it so Crawl can publish it, although it might be weird having @discordjs/core as depencency in 12.x

Since... that package is for 13.x

I still plan on using this ASAP, I have been trying to make d.js have useful stack traces for years, I think my first PR adding them (although it used V8-specific APIs, and broke error messages in some cases yet unknown to me) dated back to 2018, but Lewdcario and a few more people were having issues, so we had to revert it.

My point with telling you all of this is, it's not a new issue, we've been looking for a solution that works for everyone without needing to use non-standard things, and we have now found one :)

real jetty

how can VoiceChannels have a Channel Topic? Thonk

frank turret

it inherits from GuildChannel, which I guess has that. Shouldn't that be something more like on a TextChannel/NewsChannel?

real jetty

Yeah, according to discord api docs, only Text, News can have topics GWbruhThonkNoHands

frank turret

In addition to that, the setTopic method exists on GuildChannel but not the topic property (at least on the docs), leading me to believe this is a mistake?

copper laurel

I've raised before that the placement of properties on GuildChannel doesnt really make much sense

frank turret

worth opening a pr for?

copper laurel

Like Souji hinted at earlier, now is probably the time to make semver major changes

Its actually pretty extensive though

createInvite for example

frank turret

Well, at least for now I'll open a draft pr for removing it and putting it on textchannel

and since newschannel extends textchannel, i only have to do it on textchannel

copper laurel

Yeah make it a collab PR or issue or something

oak quail

speaking of semver major changes, maybe remove disableMentions (replace with allowedMentions) in v13?

frank turret

Should i apply the setTopic change to TextChannel or TextBasedChannel, since either way it will end up reaching NewsChannel

oak quail

@frank turret TextChannel, because DMChannel shouldnt get it

frank turret

blobthumbsup

Updated in the pr, do I have to do any wonkyness for the docs?

oak quail
  • Sets a new topic for the Text Channel.
  • @param {string} topic The new topic for the Text channel
  • @param {string} [reason] Reason for changing the Text channel's topic

this is unnecessary

just say channel

frank turret

Alright, will change

oak quail

while ur at it i'd change the description for .topic too

its inconsistent with the rest and is wrong for news channel

frank turret

wb the example, should i also make that channel.setTopic? that seems somewhat misleading

oak quail

well everything else is channel.

because it assumes that you have a variable named channel

also u gotta fix up the spacing and stuff so eslint stops complaining

tender field

@solemn oyster thanks for these explanations! I think i understood 😅
Looking forward to these change, then 😄

solemn oyster

@wild flax one question, are we allowed to use @discordjs/core in v12.x?

Or should I yeet AsyncQueue and port it to JS?

tacit crypt

I..don't see why we wouldn't be allowed to?

solemn oyster

Voilá!

@tender field in case you want to try it out 😄

tender field

This is really nice! Thanks 😄

solemn oyster
old kelp
solemn oyster

Yeah, it should, although, wouldn't that be rawPosition?

old kelp

I guess, but wouldn't that create a bit of inconsistency since GuildChannelManager#create and the endpoint both take the position as the param? the user could always just specify it themselves as well.

real jetty

Hey
What do you think about Message#awaitMessages return a Message object if Collection contains only 1 Message Object ?

old kelp

imo that's the expected behavior, it would be a breaking change if to do so. there are plenty methods that also do that, if you go at changing that yet - it becomes inconsistent with the rest.
see MessageManager#fetch, with the limit of 1, or GuildMemberManager#fetch with the query option, for instance.

unique axle

i heavily disagree with this, this makes the API and typings inconsistent and hellish to use. We can only do that with Collection#first because of the distinction no args/ amount

adapting this based on a number that's passed as optional parameter does not seem an improvement in usage

ornate topaz

that would change from always getting a Collection, with first() method to obtain a Message, to a Collection or a Message, making you need to constantly check how many elements your awaitMessages was supposed to collect.

oak quail

maybe followTo would be a better name then addFollower?

unique axle

is there anything speaking against just... #follow ?

oak quail

I thought it might be confusing as to which channel is following which

snow crypt

lets say you have channel1 - announcement channel; channel2 - text channel
channel1.follow(channel2) could now refer to either:

  • channel1 should be followed by channel2
  • channel1 should follow channel2
    it depends whether you write it as follow(follower) or follow(toFollow) which I find very confusing
warped crater

I... absolutely disagree here.

To me channel1.follow(channel2) makes zero sense

you have to follow, at the 3rd person, singular, it obviously refers to the channel you're passing in?

snow crypt

not really
yet i can't really phrase it in a clear way

  • you can call #follow on a channel, and now it is followed
  • you can call #follow on a channel, and you've just instructed it to follow another channel
ornate topaz

channel1.follow()
message.pin()

warped crater

not to mention how the news channel shouldn't have a follow method either way, imo

ornate topaz

to me it's clear - follow is method used on channel1

warped crater

which makes it even more clear

snow crypt

um
what would you use in that context instead of message.pin()

ornate topaz

channel.addpin(message)?

if you're asking about that

snow crypt

god language is weird

warped crater

okay

I just saw the status of the PR

snow crypt

i can't really explain it but not naming it follow makes sense

ornate topaz

imo, just like in the app, you follow an announcement channel, selecting into which channel it posts

snow crypt

so you want NewsChannel#follow ?

ornate topaz

so channel1.follow(channel2) would make sense, assuming c1 is news channel

yes, that would make it consistent with app, as well as logically with rest of the lib

warped crater

mmm

I feel like TextChannel#follow should also exist

to sit in fashion with other helpers

such as Member#ban literally pointing to Guild#members#ban

as for the naming of NewsChannel#addFollower im starting to see the issue lol

snow crypt

but naming them both follow just makes it confusing again which is what we tried to avoid in the first place

warped crater

probs fine keeping NewsChannel's as is, then

snow crypt

now we have news.follow(follower) and text.follow(news) and without static typings in there you just fuck readability

you could accidentally throw by trying to follow a text channel because you got the order wrong

you might want it to exist on both but naming them both the same, especially with inheritance, is just wrong

ornate topaz

followIn() for news?

snow crypt

i still agree with addFollower

ornate topaz

imo that sounds odd and everytime i look at that i think of users

snow crypt

same for folllowIn

ornate topaz

well, technically, news.addFollower() doesn't really say much on its own. something like "add follower, yeah cool". could argue that news.followIn() asks for a channel to be passed

tender field

I think followIn() is way clearer than chan.follow(chan) ^^

copper laurel

This is why documentation exists and has a description

tender field

that's true, but it doesn't mean that methods should be named anyhow

like when you write the code you have the documentation, but when you read someone's code you shouldn't need the doc

copper laurel

I mean... thats why they should comment their code

This is now offtopic though

tender field

well imo they should comment their code only if their fault that the code is not readable. If the library has confusing methods, it not their fault

oak quail

so, no to NewsChannel#followTo?

tender field

i find it clear too 🤷🏻‍♂️ but i think the conversation moved to #archive-offtopic 🤔 they proposed a manager

ornate topaz

nothing was really proposed

solemn oyster

@snow crypt as in, adding @private to AsyncQueue class's description?

snow crypt

yes

solemn oyster

Added 👍

Good call 😄

Added the comment, thanks! @snow crypt

snow crypt

with love 🎉

solemn oyster

Also @vagrant holly, do you have any progress on the message edit limit?

vagrant holly

Also @vagrant holly, do you have any progress on the message edit limit?
@solemn oyster I have some stuff locally that I'm testing out in prod, will probably make it into a PR in the next week or so 👍

vagrant holly

Just noticed this mem creep

From a heap dump of the shard, this array stands out as fuckin huge compared to any other shard

Any thoughts on figuring out what's causing this, not familiar enough with heap dump or lib internals

left moat

OR

kinda different idea but for follow mayb add it to guildchannelmanager?

guild#channels#follow(c1, c2)

raven juniper

That opens up more opportunities for a screw up, if they put the wrong channel in the news channel spot

Although it is more use of the manager, which I know was a discussion lately... Could see it, I suppose

left moat

also it makes it less bork when u have followTo on newschannel and follow on regular textchannel which will confuse people imo

and u can unfollow aswell

but imo unfollowing should be on channel but that may be a bit inconsistent

vernal atlas
analog oyster
warped crater

uh.. yes?

because its not part of the public api and shouldnt ever be?

its an internal property for handling your http requests

and should never be touched externally

analog oyster

what if I want to use it

copper laurel

Too bad, its private

warped crater

feel free to use it, but because its private anyone is free to change it to api2 just for the lols in any release and break your code

or for any other reason

you shouldn't make http calls that way

copper laurel

TypeScript wont let you use it though

solemn oyster

Just do Reflect.get(this.client, 'api') to bypass TypeScript's system, @analog oyster

Alternatively you can do (this.client as unknown as { api: any })

tender field

Hey what's the difference between TextChannel#overwritePermissions and TextChannel#createOverwrites ? They both replace the existants overwrites, and it's just that the later takes a userOrRole and overwrites, where the former takes an array of objects with userOrRole and overwrites

vivid field

overwritePermissions clears all permissionoverwrites and applies the ones provided
createOverwrite clears only the permissionoverwrites of that user/role and applies the ones provided
updateOverwrite clears nothing and updates the permissionoverwrites for that user/role

vagrant holly

This mem creep/leak continues to grow. Any tips of getting to the root of it? Feels internal somewhere

copper laurel

Respectfully something "feeling" internal isnt a lot to go by haha, would probably be best to discuss outside this channel until we have something more conclusive to look into the library for

vagrant holly
tender field

@vivid field ohh ok fair enough, but maybe the doc should explain that better ^^

vagrant holly

bad shard

good shard

oak quail
unique axle

issue link please, i want to track that

oak quail
gaunt flare

me too because I just tried to start using djs in a browser. djs claims to support browser but i'm not actually sure it does anymore? I'm running into CORS issues which lead me to https://github.com/discordjs/discord.js/issues/4710 (which you also just commented on recently)

vernal atlas
vernal atlas

also, should an error be thrown for GuildChannel#edit if a parentID was passed, but couldn't be resolved? as GuildChannelManager#resolveID returns null in that case - which will have the un-intended effect of removing the category from the channel

oak quail

@gaunt flare djs supports it but Discord seems to have blocked it

gaunt flare

🤔 hmm that's unfortunate...

oak quail

as a temp workaround you can try {http: {api: 'https://discordapp.com/api'}} in your clientoptions

that may break in the future tho

oak quail
oak quail

oh, it merged as addFollower, f

drifting knot

when djs next gets released can it be under the package discord

2 emails to npm and they will give it to you

no publishes in 8 years

here is the email

to: msteigerwalt@gmail.com
cc: support@npmjs.com

Hi,

I'm writing to see if our organization Discord.js could use the "discord" package you published 8 years ago.

Thanks,
Discord.js Representative

the package will be yours within 4 weeks

oak quail

but why

also it might sound too official

ornate topaz

discord*.js*, discord*.py*, discord*.rb*
those aren't there for show

drifting knot

npm explicitly requests you dont name package with .js

ornate topaz

but discord.js is not discord

drifting knot

keep the github as discord.js

make the package discord

ornate topaz

but why

what's the point

drifting knot

its cool

and follows npm convention

ornate topaz

what convention

drifting knot

Don’t put “js” or “node” in the name. It’s assumed that it’s js, since you’re writing a package.json file, and you can specify the engine using the “engines” field. (See below.)

ornate topaz

where is that

drifting knot

many other packages do this

ex. google is an unofficial lib for using google search

google is also heavily active on npm, but they use their scopes of @.google-cloud, @firebase, etc.

ornate topaz

also, it's a tip

not a rule

it's a tip because google.js sounds worse than google

but discord.js is not discord

drifting knot

i didnt say rule i said convention

ornate topaz

it's a tip

and it's explained why it's a tip in the second sentence

also, even if, this will cause literally nothing more than confusion

using discord to code bots for discord.

drifting knot

logical

ornate topaz

anyway, that went offtopic.

drifting knot

people already import it as

const Discord = require('discord');
ornate topaz

what

drifting knot

module.exports

when people import the whole module

but you use the cringe discord.js

ornate topaz

cool, they can also name it lkidfhgkjdfghbkjhdfgekjlihvdfkjbh, should we rename package to that?

drifting knot

yes

simply begin annexing package names

ornate topaz

ok, case closed

drifting knot

i am going to take the discord package name and publish malware to steal bot tokens

ornate topaz
wild flax

We can double publish packages, that should be no issue

tacit crypt

For all intents and purposes, it would be better if Discord themselves claimed that name, not us

solemn oyster

Should AbortError retries be tied to its own option, or share with the 5xx one?

snow crypt

when would you deploy the new changes to npm?

solemn oyster

tfw noticed a type bug while trying to fix 3471... I re-arranged the handler to be... easier to read - handle 2xx and 3xx, then handle 4xx, inside it, 429, then handle 5xx

Also, this comment was written 2 years ago, it's not really true, at least, not for today's spec:js // NodeFetch error expected for all "operational" errors, such as 500 status code

Both of those codes resolve with 500 and 501 respectively:js require('node-fetch')(`http://httpstat.us/500`).then(r => r.status);``````js require('node-fetch')(`http://httpstat.us/501`).then(r => r.status);

real jetty

Heya, I was messing around with guildMemberSpeaking on a new project that I forgot to install the voice dependencies for. On the latest version, when I try to play a stream that I've received from a user (so, voice audio), it fails silently if I dont have the dependencies installed. This seems to be different from previous versions, like 12.2.0, where it would throw an error if I was missing deps.

Not sure if this is intended and whether or not I should open an issue for this

real jetty

Actually, scratch that, that may have been a fluke while listening on guildMemberSpeaking. The event doesn't seem to be firing at all, and I'm not the only one with this issue

solemn oyster

@tacit crypt read above please

tacit crypt
solemn oyster

wdym

tacit crypt

Uh

remove the comment if its inaccurate

and i guess keep the rest of the code as is?

solemn oyster

I moved code around because the flow was hard to understand

tacit crypt

thats fine

solemn oyster

Because it was handling this way:

2xx/3xx
429
5xx
4xx

I made it be:

2xx/3xx
4xx

  • 429
  • !429
    5xx
tacit crypt

coool

solemn oyster

I guess by removing the comment, you meant Isa's comment (git blame points to her). I replaced it withjs // Retry the specified number of times for request abortions

tacit crypt

ya

real jetty

Actually, scratch that, that may have been a fluke while listening on guildMemberSpeaking. The event doesn't seem to be firing at all, and I'm not the only one with this issue
@real jetty I seem to have found the solution to this problem
https://github.com/discord/discord-api-docs/issues/808#issuecomment-457962359
Now you cannot receive anything unless you first send several packages through the UDP connection.
This issue describes it.
Would it be possible to add this to the library so we don't have to worry about sending empty packets first before listening to user streams?

ornate topaz

@real jetty tmk library already does that, but you still have to send some data before receiving it.
There's also a fact that voice receiving is not documented, and is changed from time to time

real jetty

Yeah, as I figured from the issue. I just spoke with someone from #archive-djs-v12-voice-deprecated and they said the solution to listening on guildMemberSpeaking was to send an audio file at volume 0, but then what's the difference between that and creating a readable stream from /dev/null LUL

ornate topaz

That audio files are not os dependant shrugCat

real jetty

lol guess so

lofty birch

When will #4835 be published? I see it's been merged

copper laurel

Whenever 12.4 is released, we don't do scheduled release cycles

solemn oyster

@lofty birch at very minimum, 4852 needs to be merged before releasing, because I introduced a bug in that PR

I didn't notice the bug because it's only noticeable when you do an instanceof check, which I didn't during testing

snow crypt

well

both merged now

any release soon?

unique axle

as per usual we do not give an ETA on releases

tacit crypt

@unique axle how is a major change? In relation to the edit limit

You're still holding edits, and for 99% of people it isn't even a problem we'd now hard limit to 100

Even then, we could just... Default to -1, thus minor

unique axle

it changes the public facing behaviour of the library, how would it not be major?

currently there is simply no limit on it, any form of imposed limit by default is thus changing and major

-1 being "no limit" would make it minor, sure

tacit crypt

it changes the public facing behaviour of the library, how would it not be major?
@unique axle it doesn't change anything public facing. The edits array is the same, and its contents. What it changes is a "limit" (that for all intents and purposes is not breaking at all)

An internal limit at that

unique axle

pardon? before you could access 51+ edits, now you can't, how would that not be breaking?

tacit crypt

who says you can't

message.edits[51] is not different. the only diff is how big that array WILL grow (and come on man you can't find me a person who DEPENDS on that many edits [also i won't comment how personally i dislike the idea of the array but alas])

unique axle

huh? if you limit an array to a max length of 50 where before there was no max, that's a change in behaviour
or am i completely misunderstanding this pr

tacit crypt

which, fyi, for all the user knew, it could've been limited at 10 or 100 or Infinity.. shrug

Look, idk, I don't agree with setting the limit to 100 being a major change. If it is, thats stupid semver wise, and at that point for every thing we now let you control its cache we.. release a new major??

(assuming we don't have 1 big PR with it, but split it into pieces)

and fyi

unique axle

how is that stupid? what? i really don't get it
you set a default limitation where there was none before
adding the ability to let the user manage this is minor, if it behaves the exact way it did before the change by default
as soon as the default behavior changes in any way that's semver major

tacit crypt

MAJOR version when you make incompatible API changes,
MINOR version when you add functionality in a backwards compatible manner, and
PATCH version when you make backwards compatible bug fixes.

its not incompatible at all

unique axle

introducing a limit where none was is def. incompatible

tacit crypt

...you what?

Adding a configurable limit is in no way incompatible api change in my eyes

unique axle

but the default behaviour changes

if you have to apply a configuration to get the same behaviour you got before without any that's breaking

tacit crypt

No, cause its backwards compatible

unique axle

only if the default is no limit

tacit crypt

you added functionality (cache size limit), that defaults to 100 ("major") BUT you can set to (-1) => MINOR

unique axle

if you default it to no limit it's minor, yes
if you default it to 100/50/x it's major

tacit crypt

HOW

Souji. You CAN SET IT TO THE BEFORE LIMIT

unique axle

you have to apply a setting in a minor release?

you have to add code to set it to that

you have to make a change to your code base after updating to get the exact same behaviour

tacit crypt

you have to add between 1 and 3 lines of code MAX. Even then it's still backwards compat imo

unique axle

the whole idea behind semver is you can update the dependency without anything breaking without you having to apply any change, whatsoever

as soon as you have to now apply a setting to get the same behaviour as in the earlier version you made a breaking change that requires someone to maintain their code base

tacit crypt

doubt

I don't see it like that

unique axle

that is legit what semver is for

02Shrug

tacit crypt

I'm referring to the "requires a setting => major"

Not the versioning itself

If I delete the prop altogether, thats DEFINITELY major (INCOMPATIBLE API change, no more edits prop)
If I artificially LIMIT the prop to have a length of 50, it still has the prop, its still there shrug

lofty birch

An update where you have to change anything to get the same behaviour is breaking

unique axle

you still changed how a public facing part of the library works

you have to make a change to keep what you had working before, even if it's just a single letter, that's not minor anymore

tacit crypt

Yeah no, I disagree, but thats my opinion. The language itself is vague as is (and no, adding an option to limit a cache is not breaking)

What next, we release v13 or w/e, then I make a PR to limit another cache and we jump to v14? that's absurd

And from a G search

It sounds like they just mean "mostly backward compatible".

The definition of "backward compatible" pretty much is "not breaking stuff". But if there are some small changes to rarely used things, that only affect a very small portion of users, it may still be accurate to describe a new version as backward compatible overall.

unique axle

then we'll have to agree to disagree here, the point of semver is so you can specify a range in your dependencies and experience no change in behaviour whatsoever.
after all you guys (maintainers) will have to decide that, i'm clearly not making the decisions in that regard as merely a contributor and triage

tacit crypt

Technically this PR with a def limit of 100 would be mostly backwards compatible shrug

I'm not trying to say you are 100% wrong and I'm right, or w/e. I'm just stating my opinion that this PR, imo, is not major but minor. It's not in my hands when it'll be merged/released with what ver number. But personally, I would've merged it as minor

Technically with this one we're both right

vagrant holly

^ Setting it to 100 makes it a major change per semver, I concur

Setting it to -1 means the behaviour matches master by default, thus minor. PR now has that 🙂

solemn oyster

@tacit crypt JFYI, we can change limit from -1 to 50 (or 100) in v13, if not entirely remove _edits

We didn't need _edits when we made KC, and it worked just fine, I don't see what its purpose is, or why it was added in the first place

unique axle

that was the initial point i was trying to make, seeing that v13 is not going to take very long having the feature be implemented without changes in behaviour now and changing the default/behaviour with v13 seems to be a preferable solution to me

solemn oyster

Yeah, in v12, any change we make must make everything that has been documented, behave like so

tacit crypt

We never documented edits having an infinite length 0_EyesMoving

solemn oyster

It does document "the oldest", anyways

tacit crypt

Considering how message updates work tho..

That's wronf

And inaccurate of..messages fetched

weak kraken

Have you heard if they plan on implementing anything like an event emitter/stream for audit logs? or upgrading the guildMemberRemove one to indicate kicked users states?

copper laurel

Nothing like has come from Discord, no

weak kraken

kinda shocked, would seem like the audit log would be a natural place to track all manner of events

albeit redundant

ornate topaz

discord doesn't send anything for when audit logs are created

weak kraken

i know, i was asking if its planned

ornate topaz

it just reflects changes sent to clients in other events

(which already would make audit logs events redundant)

weak kraken

just feels weird to have to use audit logs to discover if a user was kicked, for example

ornate topaz

while true, there isn't much we can do about that

snow crypt

@vagrant holly can't you just remove the -1 for infinity? Infinity exists and works in that case so -1 too seems weird

vagrant holly

We seem to use -1 in a lot of other settings for no limit, so I reflected that with this PR too @snow crypt

snow crypt

i see now, my bad

vagrant holly

Should I skip hooks to revert the change, or should we just leave it?

vernal atlas

perhaps you could sneak in a prettier-ignore comment or something?

unique axle

what even is the actual issue here? eslint conflicting with prettier or something?

because i'd much rather fix the underlying issue than monkey-patch it for each PR

vagrant holly

yeah, eslint & prettier seem to be fighting

lofty beacon

What are peoples thoughts on User.mutualServers? Just gets the guilds the bot and the user are mutual to.

unique axle

that information is available

lofty beacon

I know that, but adding a shortcut on the User class. Do you think that'd be a good idea?

unique axle

if you know that the information is not available and requires an oauth2 request, how would a shortcut on User be of any use?

lofty beacon

Can't you just map the guilds and check if the user is present in it?

unique axle

that information is inaccurate due to just affecting cached member instances

lofty beacon

You could fetch them, set the cache option to false and see 🤷

unique axle

no. we don't need a utility function that iterates over n servers, fetches members on all of them and bloats cache into oblivion

lofty beacon

alright

thanks

unique axle
  • the official way to retrieve that data (shared servers) is an oauth2 request
    we're not going to support some way to prevent api limitations and make N requests for the user in the background, hidden behind a single seemingly harmless function call
lofty beacon

makes sense

cerulean siren

Is there a planned version with Deno Support?

raven juniper

No

cerulean siren

Ok, thanks for your answer

tender field

it is a bit weird that AuditLog is not a manager, isn't it ?
Like we have to fetch it to get the actions enums... And i feel like some cache capacity could be added 🤔 Is it planned for v13? Is there good reason to justify the way it is now?

unique axle

managers manage (hence the name) a cache, neither audit logs nor bans (where this also applies) are received through websocket events (and caches/cached structures) and consequently also don't require managers

tender field

oh ok

but why do we have to fetch it to get the enums ? This also feels weird to me

unique axle

pardon?

tender field

(what i call the enums are GuildAuditLog#Actions)

oh no i'm dumb

i can juste call AuditLogActions

then it makes sense the way it is

copper laurel

So, currently drafting the PR for inline replies
The Discord API property for the message a crosspost references is message_reference, which we implemented as Message#reference
The API property for the message an inline reply references is referenced_message

So I need a naming convention for this property - Message#replyReference?

opaque vessel

That sounds okay! It feels like in the next major version, .reference should change to .crosspostReference to keep the a clear distinction between that and .replyReference? 😅

copper laurel

That definitely makes sense - Souji has a PR to replace Message#reply with functionality that would act as a helper for inline replies, and might also include other breaking changes like renaming properties like that

oak quail

@opaque vessel @copper laurel message_reference is also used for replies, as well as pin messages and follow add messages

message_reference is guaranteed on replies while referenced_message may not be present

copper laurel

Right, that's what some comments suggested but it wasn't clear

So referenced_message is only on replies, but message_reference is all of them?

Why does the API bother sending both, because a reply needs author data etc for the reference?

oak quail

So referenced_message is only on replies, but message_reference is all of them?
yeah

and it sends both because message_reference just has the IDs which can always be provided; referenced_message has the full message object which is for client rendering (they don't want all clients fetching the message) but in some cases that won't be returned

copper laurel

So then if its for clients, is there any reason for Discord.js to implement referenced_message as I have as a Message property? Or just cache it using that data, and rely on the properties from message_reference if it needs to be grabbed from cache and/or fetched later?

tacit crypt

I mean we can still do it, Discord gives us the data, we might as well hand it over as a Message object (so then we also don't need to fetch the reply)

lofty birch

Would message cache sweeping behaviour not affect this? If it was set to delete all messages older than 10 minutes, for example, and a message younger than that includes a reference to a message that has been removed from cache, how would that work?

ornate topaz

would probably be removed again in next sweep

which would be to be expected, honestly

slate nacelle

The PR in its current state seems to be referencing the object directly, so it would no longer be in the cache, but still accessible, through the referencing message, after sweeping.

lofty birch

Is that necessarily good behaviour though? By being accessible through the referencing message, it's still cached, and will only properly be swept once the latter message has been also. Perhaps making it a partial message object once swept would be better?

copper laurel

Sweeping a Collection doesnt modify any objects that arent being swept though

slate nacelle

It's also possible to make it a getter, like User#lastMessage and friends. Messing with the object after sweeping is imo a no-no.

copper laurel

That would actually become quite a complex operation, checking every Message still in the cache

Yeah, I did suggest it could be a getter based on the Message#reference property, and we just cache it when it received, not store as a reference

Then sweeping would work as normal

lofty birch

I'm just thinking of the memory usage more than anything. A getter would fix that

How would it behave if we then wanted to fetch that other message though?

slate nacelle

Similarly, by having a field with the id, which you can use to fetch.

copper laurel

As normal

MessageManager.fetch(message.reference.messageID)

lofty birch

That's a lot better behaviour than the sweeping not working. I'd be totally happy to use that

vernal atlas

im starting a PR to update to the v8 API, should i bundle it all in one or create multiple PRs

im thinking multiple PRs might be better but want to know if it ultimately matters

oak quail

@vernal atlas you should clearly label that this pr switches to API v8

vernal atlas

aight will do

oak quail

or tbh just do all v8 stuff in 1 pr like fyko said

did any maintainers say anything

vernal atlas

got no pings about it

copper laurel

Note that I already added message type 19 in my inline replies PR, so that's v8-ready

oak quail

or a string

in RESTPostAPIGuildRoleJSONBody its permissions?: number | string | null;

vernal atlas

might have been an oversight

oak quail

should i just remove it so its the original value

because afaik only the id is different

vernal atlas

ye i guess so

oak quail

also i'll change

export type APIGuildCreateOverwrite = Pick<APIOverwrite, 'type'> & {
    id: number | string;
    allow: number | string;
    deny: number | string;
};
```to
```ts
export interface APIGuildCreateOverwrite extends RESTPutAPIChannelPermissionsJSONBody {
    id: number | string;
}
vernal atlas

oh wait i think i know why i added them

i think it was to do with

permissions being deprecated and x_new being a thing

oak quail

heres RESTPutAPIChannelPermissionsJSONBody in v6:

export interface RESTPutAPIChannelPermissionsJSONBody {
    allow: number | string;
    deny: number | string;
    type: OverwriteType;
}
vernal atlas

i think that'll be good actually tho since RESTPutAPIChannelPermisionsJSONBody (god, thats a mouthful) doesn't have _new

oak quail

yeah

vernal atlas

👍

vernal atlas

i think i've covered everything, i went through the diff from the api docs commit, i imagined it'd be bigger but i guess not

this PR is mainly just the basics btw, i haven't really got too much into handling uncached data from missing intents, but i think that this was planned for the next major update in discord.js-next

oak quail

@vernal atlas did you test if messages in DMs work

Bots no longer receive Channel Create Gateway Event for DMs
this might break it

also did u update ratelimit stuff

The Retry-After header is now based in seconds instead of milliseconds (e.g. 123 means 123 seconds)

The X-RateLimit-Precision header is no longer respected. X-RateLimit-Reset and X-RateLimit-Reset-After are always returned at millisecond precision (e.g. 123.456 instead of 124)

vernal atlas

hm, i didnt test that, but at a guess you would need the CHANNEL partial type enabled

oak quail

then should enable it by default i suppose

or just not make it an option

vernal atlas

also have not updated the ratelimits, wasnt that done in another PR or am i mistaken

oak quail

i dont think it was

vernal atlas

oh yea that was just the async queue

ill update the ratelimits tomorrow

oak quail

did u account for this

Channel Permission Overwrite types are now numbers (0 and 1) instead of strings ("role" and "member"). However due to a current technical constraint, they are string-serialized numbers in audit log options.

vernal atlas

oh

i dont think i did

should be an easy enough fix

oak quail

that probably wouldn't have been documented if i didn't mention it bloblul

vernal atlas

FedoraLUL

that wasnt on the api docs v8 diff hmm

oak quail

it was

vernal atlas

oh i couldnt find it

maybe im just blind, happens

lofty beacon

hey
when fetching members, it allows you to pass in an object when fetching
shouldn't it be like this for fetching roles, channels maybe?
i might make a pr of it just was wondering if well people thought it was a good idea

lofty birch

You can't fetch only a particular channel or role. The API just doesn't support it

lofty beacon

oh does it not?

sorry then

lofty birch

The reason you can fetch members with a query is because there could be hundreds of thousands of members

lofty beacon

yea

lofty birch

And returning all of them would be a huge waste of resources

Unlike channels (500) and roles (250)

lofty beacon

yea makes sense, thanks.

sullen mortar

Any solutions to insane memory usage?

I have two shards ~1.8k guilds, shards are allocated 1gb of memory and restarts daily due to memory usage

message cache set to 50

My bot holds no state

unique axle

you can manually sweep caches on an interval
apart from that currently not much, addressing custom caches is on the roadmap for the planned typescript rewrite, it requires us fundamentally rethinking the inner core structures so it's really nothing we can feasibly do under the current library.

if your bot can completely work without state you are probably better off using a more low level approach to websocket events and a rest API (for now at least)

left moat

make sure ur using the correct intents

sullen mortar

my intents are GUILDS, GUILDS_MESSAGES and GUILD_MESSAGE_REACTIONS

@unique axle great info, thanks! I'm pleased to see the project making steps to address these memory issues

I know you can sweep messages cache on interval, but other caches too?

unique axle

sure, caches are at the core Collection(s) which are extended from the base JS Map structure, you can #clear #sweep them as documented, providing a predicate function to selectively clear for the latter

sullen mortar

yeah - gotcha

thanks

oak quail

i don't see X-RateLimit-Precision in the code; does djs use second precision? is ms precision even supported in apiv7?

unique axle

@tacit crypt (i think that's your thing) this
where thing := area of proficiency

tacit crypt

I don't think it was ever PR'd to d.js (or it was in that rest PR of mine that is dustier than dust)
And iirc api v8 doesn't support it sooooo

oak quail

yeah was just wondering if ratelimiting would break if i updated something to v8

but v8 sends decimals now, so maybe that could break it? idk

it looks like it shouldnt break

cc @vernal atlas might not have to update ratelimit stuff

vernal atlas

ah

copper laurel

Going back to the referenced_message data, which is a complete Message object to support the message_reference properties of a reply, what do people think:

  • maintain a complete replyReference property on Message
  • cache the message, but make replyReference a getter rather than maintaining a direct reference

Also to note, that getter needs to know if the properties exist because its a reply and not if its a crosspost or pin notification

warped crater

And iirc api v8 doesn't support it sooooo
source?

copper laurel

Which leans more to the first option being easier, as the second would need to define something like isReply

The X-RateLimit-Precision header is no longer respected. X-RateLimit-Reset and X-RateLimit-Reset-After are always returned at millisecond precision (e.g. 123.456 instead of 124)

warped crater

ahhhhhh I see, that's much better then

I guess they did it in a backwards-compatible fashion before as a feature on top

but now it's just always ms

good good

real jetty

Hm, I'm experiencing a strange bug, when the bot removes its own reaction from a message it sent, it seems to be ratelimited, as in the rateLimit event is fired. This this intentional?

Or, wait actually this might be a combination of things, one moment let me see if I can reproduce it

Never mind, it's just my bot doing many things in quick succession; what I was doing that would trigger the rateLimit event was I was adding a reaction to a message sent by the bot and then immediately removing the reaction.

I guess Discord didn't like that.

surreal geode

Message reaction ratelimits are 1 per time period so you will always hit a ratelimit when reacting/removing reaction

copper laurel

Going back to the referenced_message data, which is a complete Message object to support the message_reference properties of a reply, what do people think:

  • maintain a complete replyReference property on Message
  • cache the message, but make replyReference a getter rather than maintaining a direct reference
    Additional thought - would the getter need to just be for replies? Couldn't it try to get the referenced message no matter if its a reply or a crosspost?
lofty birch

What if it was crossposted from a different shard (with traditional manager) though?

copper laurel

Its just a getter, so if it cant get, its null

Anyone using sharding generally should expect to have to do fetching of things like that

eg

get referencedMessage() {
    return this.reference.messageID && this.client.channels.cache.has(this.reference.channelID)
      ? this.client.channels.resolve(this.reference.channelID).messages.resolve(this.reference.messageID)
      : null;
  }

That should have a check for this.reference too but yeah

lofty birch

That's my point. A reference is different to a crosspost due to which servers they can come from

copper laurel

Well no, message_reference provides message/channel/guild ID for crossposts, pins, and replies

For a reply, we also get a complete referenced_message object which allows us to cache that referenced message also

So when it comes to the getter, should it be generic and try to get any sort of reference from cache, which would mostly be replies, or should we restrict it to only replies
And if the latter, we'd need to set a flag like Message#isReply to enforce that restriction

vagrant holly

👋 Gonna play around with this some more, but can anyone think of any immediate issues to sweeping cached channels? Should the lib cleanly handle a message arriving in an uncached channel etc.?

ornate topaz

to my knowledge it doesn't emit messages for uncached channels, but i neither kept track of it nor checked on my own

analog oyster

node v14 will become LTS soon, are you guys planning on supporting it

oh ok

vernal atlas

@raven juniper regarding #4890, doesn't seem out of scope to remove it from the tests too

raven juniper

i changed the method to fit in the test/random.js file, i'm just saying the whole line seems unwielding

it's the whole message.guild.member(message.mentions.users.first()).kick() like... why are we using that there instead of message.mentions.members.first()

vernal atlas

ah i see

raven juniper

ideally it'd be ```diff

  • message.guild.members.resolve(message.mentions.users.first())
  • message.mentions.members.first()```
vernal atlas

yeah seems kinda odd why that was done

raven juniper

that's why i said it's out of scope for the pr, 4890 is only for changing guild#member

vernal atlas

might have been done because of MessageMentions#members not existing at the time

checked git blame - it used to be guild.member(message.mentions[0])

raven juniper

guess i'll make a note to update it after the pr, though it'll be a while since it's major

copper laurel
vagrant holly

Yeah I'm wondering if my calculations on guild member sizes are wrong

Channels are showing up as the biggest culprit currently

copper laurel

Is the message cache counting towards that?

oak quail

the MessageCreate action handler only works if the channel is cached
well that has to be changed in v13

as u wont have dms cached in v8

ornate topaz

technically you could achieve one without the other making use of properties on the object, but imo, at this point it wouldn't make much sense to continue dropping data for uncached channels

oak quail

@copper laurel UserManager#send maybe?

just ran into a situation where i want to use that lol

copper laurel

Yeah I'm still planning to move onto other managers haha

But sending is implemented by TextBasedChannel

Although its really just a wrapper around createDM which is client.api.users(this.client.user.id).channels.post so yeah, could do

quartz beacon

does this officially support typescript?

so in the repo, I was told this discussion belonged here:

const testChannel = bot.channels.cache.get(DISCORD_TEST_CHANNEL_ID)
testChannel.send('send does not exist')

in typescript send does not exist, the reason i was given was: Channels retrieved from cache need to be cast to TextChannel in TypeScript

because cache.get is supposedly agnostic or something

the thing is, cache.get is executed from within the manager, meaning if any casting is needed, it should happen in that concern should it not?

vivid field

<Client>.channels.cache does not only contain text channels, so it would make no sense to type it like that

however i don't see the problem with type casting when you're getting by id anyway

quartz beacon

i see the problem now thanks

maybe as a mid-point it could return all 3 as a union type, right now it's "any"... is that possible?

I know it's not written in TS so idk how hard it is to make it do stuff like union types

vivid field

should return Channel, not any Thonk

quartz beacon

let me double check tonight

tender field

I know it's not written in TS so idk how hard it is to make it do stuff like union types
It should be fairly easy, because it would only require modification to the .d.ts file, which shouldn't be that complicated

copper laurel

It been attempted before, and ultimately caused some broken edge cases which cant be resolved by the developer, such as never typing.
So this has been the best solution for now, until such time as the full TypeScript rewrite reaches stability

We could attempt this again given that several TS versions have been released since then, but that's the current rationale for not doing union types

ornate topaz

well, the d.js-free example from the pr still produces same output on 4.0.3 🤷‍♀️

copper laurel

Probably still not a viable option then, thanks for testing

real jetty

Would you consider adding a setter to <User>.createdTimestamp? it would be useful for testing things that require the account age to be something and not have to create a new account.

Personally I don't feel like that should be part of D.JS (not that my opinion is worth anything lol), but what you can do is use Structures#extend to extend User and add that functionality yourself

Ah yes I forgot about that, I'll do that happy

copper laurel

Shouldn't even need to do that, just generate a new Snowflake with the age you need and set the ID

tacit crypt

IIRC discord doesn't allow connecting via web anymore

left moat

theres no pr open yet for the member object that is now passed thru messagereactionadd right?

vernal atlas

is this new for V8 or is it still sent on V7? @left moat

on that topic - is there anything more i need for my V8 PR that anyone knows of

left moat

uh this is v8 afaik but ill confirm in a few

oak quail

@left moat @vernal atlas v6

was documented back in November

warped crater

With djs v13 being on it's way

I wanted to express a possible change

Changing Message#attachments to use an array, just like Message#embeds

I think a map (or a collection) is just generally not suited for something like attachments; most operations on this property are either checking if the size is > 0, operating on the .first() attachment or just on all of them via iteration

nobody really... .gets them by their identifier

overall this would make it a bit cleaner to work with and be a bit more efficient as well, as collections implements array methods itself by... taking keys/values as arrays

as of cons, I can't really think of any, besides this not being that big of a deal and the potential of "just having a breaking change for the sake of it"

vivid lodge

You definetly have a point, but a breaking change for the sake of it isnt something people are fans of, so I guess we’ll see

I’ve never used any of the Collection/Map methods other than .first() and .last() on MessageAttachments, it just feels really wasted.

An alternative would be having a class extend from Array with a first() and last() even if that’s simple to put together yourself with any array, but even then it would break stuff since arrays use .length and Collection uses .size

copper laurel

There'd be no reason to make some other sort of middle ground which isnt a Collection but has collection methods

unique axle

attachments do have ids lending themselves rather easily to Map structures. I really don't know about "more efficient"? you can still iterate a collection with for..of iterators which are quite efficient themselves

copper laurel

How often do you actually have an attachment ID to use though>

warped crater

^

It probably isn't a noticeable difference but on paper it's def there, nevertheless

merry python

while in most cases you dont need it doesn't really mean it has to be removed there. You dont lose anything by having it as a Collection.

unique axle

the greater point is probably the latter, i don't really see any benefit in not having it be a hashmap

raven juniper

I think the point is it that it's almost never used as a Collection. From experience in support, users just want to know if there's one in a message, and then 9/10 times just access the first one via first(). I understand they have IDs and all, but their actual use is not really reflective of a hashmap structure, although tbh there's no real difference between the two here
.size vs .length
[0] vs .first()

left moat

i personally use partition alot

vivid lodge

I personally prefer arrays for the attachments, but I dont think we should force every discordJS bot to update their stuff because we have opinions

and besides, .array() is there if you really want to use it xd

unique axle

if you are using Collection#array in 9/10 cases you are doing it wrong and add O(n) complexity to your code without any need for it, maps are iterable

vivid lodge

Then why do you even have it

to help with the other methods like filter?

frank turret

Is it worth opening a issue regarding swapping esbuild (https://github.com/evanw/esbuild) for webpack? It seems like a faster alternative to webpack, however it is somewhat of a trivial change so I wanted other opinions on this.

unique axle

it currently looks like discord does not support login from web apps at all anymore

oak quail

for editing and deleting webhook messages, would webhook.editMessage(id, data) and webhook.deleteMessage(id) be good?

webhook.messages.___ would be pretty weird since the webhook doesnt actually store messages

ornate topaz

Would be slightly closer to rest of the lib, even if it wouldn't be an actual manager but just an object with 2 functions.

On the other side, it would already be only .messages that wouldn't be a manager

copper laurel

I wasnt even aware they could be edited

oak quail

they just added it today

patch/delete /webhooks/:id/:token/messages/:id

copper laurel

Ahh when they broke webhooks too meguFace

cool

Should it be webhook.editMessage or should message.edit() be able to act differently if message.webhookID is defined.... probably not since token is needed

So yeah Im on board with that naming convention

unique axle

ugh.. this messes the entire pathing up again

unique axle

WebhookMessage extends Message maybe? and have the webhook details/routes on that?
Won't play nicely with WebhookClient, I suppose, since that's at the moment just returning API message data, and can't really have the references a Message instance requires.
might actually better be its own class

tacit crypt

at that point we'd need a base message ASmeguFace

so we don't dupe a lot of code

oak quail

stickers are gonna be a new permission

so d.js needs to updoot to _new fields

or just release v13 soon

vernal atlas

well i guess it would make sense to update to v8 for v13, as vlad pointed out my PR for this needs extensive testing, i'll get round to doing this but if anyone else wants to do some of their own testing too that would help

oak quail

in the meantime as a quick non-breaking fix we could backport the permission code to v12 and use _new

but we'd probably need separate fields as it would be breaking to change the permission bitfield number to bigint

weak kraken

hmmm so I ran into some issues with cleanContent actually not returning "cleanContent", had to look at the library to see it was injecting the '\u200b' unicode character if a mention exists. My bot commands trigger off of the mention, and I was using cleanContent to parse out the text. Guess I don't have an issue other than to say the docs should probably indicate that.

Its sort of weird to do string based javascript functions (.contains, etc.) commands when you are contaminating the end results with unicodes in a "cleaned" variation of the content. <shrug> not sure I'm communicating this very well.

I do understand why though, guess I just need to account for it on my end

is there a way I can create a PR to add a note to the documentation on cleanContent to indicate that?

copper laurel

What exactly did you expect the behaviour to be when it says "mentions removed" - replaced by the users tag or something?

muted aurora

I don't know if this has been discussed before - but wouldn't it make sense to check in the library to see if a message is deletable before try deleting it? Currently the library is throwing DiscordAPIError: Missing Permissions in the task queue (no chance to .catch() this in my application). Now I'm writing my own method for deleting messages - but I think this is also confusing for other developers 😅

raven juniper

There is such a property, Message#deleteable

remote wasp

why inviteDelete doesn't trigger when an invite gets expire?

unique axle

because discord does not send it

remote wasp

hm

muted aurora

But why is this property unused in the library itself? Not sure how to throw errors 🙈 But I'm thinking of something like thists public delete (message: Discord.Message, options?: { timeout?: number; reason?: string }): Promise<Discord.Message> { if (!message.deletable) throw DiscordAPIError('Missing Permissions'); // ← Something like that return message.delete(options); }

raven juniper

Because the library does not do permission checks for the user

That's not the responsibility of the library, and in addition, if the API were to change permissions, then that could require major, breaking changes in the library

muted aurora

Would it be an idea to include such a check function in the guide? Would move to #archive-guide-discussion then of course (or is there already an article about this topic?)

slate nacelle

Where is the library deleting a message without a chance to catch the potentially rejecting promise?

muted aurora

If a command is successfully exectued I delete it with this codejs message.delete({ timeout: 0, reason: 'Befehl gelöscht' // ← "Command deleted" }).catch(Logger.ignore);Logger.ignore looks like thatjs public static ignore (error: any): void { // ignore error - can be used in try-catch blocks }and anyway the code throwsjs You have triggered an unhandledRejection, you may have forgotten to catch a Promise rejection: DiscordAPIError: Missing Permissions at RequestHandler.execute (.../node_modules/discord.js/src/rest/RequestHandler.js:170:25) at runMicrotasks (<anonymous>) at processTicksAndRejections (internal/process/task_queues.js:97:5)

At least I thought that the error is thrown there - the unhandledRejection doesn't give any information about the source

slate nacelle

Can you create a minimal reproducible sample for this? I seem to be able to catch this error.

muted aurora

I'll try @raven junipers advise to log the unhandledRejection more detailed and will be back here if I can track the issue better - my logs are full of this unhandledRejection and I don't know where they are from 🙈 Thank you so far - maybe I forgot to .catch() it somewhere, but I checked this multiple times now and was thinking it comes from this method

tacit crypt

Missing permissions aren't necessarily just message deletes.. If you're on d.js 12.3.0 (or w/e latest is on npm) and are willing to try it out even for a bit, you can install the master branch from gh as it has fixed rest stack traces (so you'll see exactly what is throwing)

vagrant holly

What does the release schedule look like, if there is anything formal?

wild flax

Most likely today/tomorrow

Since we gotta push v8 support soon too

So v13 will follow relatively quickly

vagrant holly

Oh, awesome

warped crater

I'm wondering

slate nacelle

They are supposed to stay type 0 on GWv6, aren't they?

warped crater

nada, I spotted some replies in the wild, lemme get one of the payloads I snatched

slate nacelle

Sounds like a bug then

warped crater

"type": 19

it could be a bug yeah

that was really the only thing "off" about the payload except the fact that it had referenced_message or whatever

oh

Inline replies are type 19 in v8, but remain type 0 in v6

yeah hmm

is the client running on v8 then?

slate nacelle

I think so

warped crater

yeah it is for a while now