#archive-library-discussion

25085 messages · Page 24 of 26

wild flax

Strictness is kinda important

Otherwise you could just use raw js

Nothing is stopping you there

That will always be an issue with ts

People want type safety and strictness but not deal with types to a certain degree

Seems like a huge clash of ideologies

golden mortar

Yep I'm not saying to loosen type strictness, only that right now it requires some underlying knowledge of the types to use properly (ie. understand that MessageActionRowComponentBuilder is part of the AnyComponentBuilder union). It's not as easy to use as v13, or even previous versions of v14. Especially since the ts errors, in part with the types themselves being confusing, aren't the most amazing.

warped crater

realistically the types will always be confusing (if accurate) because of the complexity of those data payloads

you do end up with very convoluted generics and deep inheritance on all those interfaces but at the end of the day

only that right now it requires some underlying knowledge of the types to use properly
so long as you understand the type system and are familiar with the data the method takes and what properties it accepts depending on other properties (e.g. the component type), it really shouldn't be all that much of an issue

tacit crypt

It won't be as simple as v13 as we bring in strictness. That's a given no matter where its implemented or how.. and I'm sure if you asked there's probably someone in here that can guide you

outer raven

I'm sorry to interrupt, this is just a simple question can we backport https://github.com/discordjs/discord.js/pull/7507 into v13 or add back the rule that was removed in a commit in v13 so that we can get the old casing on commits back? Also if so, which one of these options would yall prefer?

zenith oracle

Sincerely, if I want to send an embed in response to a command, but don't want to use an EmbedBuilder because it's slow, than I'll simply use a raw JSON embed... Why should I use new UnsafeEmbed().setAuthor({ name: "idk" }) when I can use { author: { name: "idk" } } (and djs still provides types for that since there is the fantastic discord-api-types)

golden mortar

Yes we've been over this. It's not realistic for people to rewrite massive portions of their bot to do this.

I've been working on my bot since around august 2020, so there is a lot of maintenance going into it just for a v14 upgrade (many thousands of lines already), I can't realistically be expected to remove hundreds of embeds... EmbedBuilders.. UnsafeEmbedBuilders.. and replace with json. It would take days lol

tacit crypt

So just keep using the builders as is..

golden mortar

I'm addressing his point because I hate that argument ("if you don't like it just simply don't use it")

tacit crypt

Backport commitlint rule

zenith oracle
golden mortar Yes we've been over this. It's not realistic for people to rewrite massive porti...

I see what you mean, since I've already done big rewrites for my bots and that's why I started using raw JSON embeds. It's surely another refactor but... probably the last as I don't think (and hope) djs will drop raw JSON support and if you're lazy you can continue using a Builder, which is surely totally safe with its validation.
I'm not saying Unsafe*s should be removed, just that, as we just said, "If you don't like it, don't use it", there's no much difference at the end...

mighty bough

Why don't GuildAuditLogsEntry objects have targetId and userId or executorId properties?

copper laurel

Because the API returns full user objects when you fetch audit logs, theres no need to rely on cache getters based on id

mighty bough
copper laurel

I mean I guess it could be converted to cache getters
https://github.com/discordjs/discord.js/blob/988a51b7641f8b33cc9387664605ddc02134859d/src/structures/GuildAuditLogs.js#L170 caches all the users returned in the payload when they're fetched
https://github.com/discordjs/discord.js/blob/988a51b7641f8b33cc9387664605ddc02134859d/src/structures/GuildAuditLogs.js#L387 the _add call on this line would return an existing user if one was in cache anyway, which they would be due to the above, same for target

peak zinc

like, why would you want to continue using the old object after updating it

unique axle

patch cache, return pre-update clone so you can still emit both in before/after events

peak zinc

ahh that makes sense ty

how about Base#_patch()? this implementation seems kinda useless, why not have this be abstract and throw an error if someone tries to call it?

outer raven

it's overwritten by other classes that extend Base, that's an abstract method

ig they just dont want that error to be thrown

peak zinc

it seems like all the other implementations of this in the library don't return anything, so it seems weird to me that this one does return something

peak zinc
bitter peak

I'd think an error would make more sense to have

outer raven

why do you care it never reaches that point anyway lol

vernal atlas

assuming that every structure should have its own _patch then IMO it should throw an error, not return the data

real jetty

what User-Agent a bot uses when making a request to the API? I'm trying to access a resource with Postman and I get Access-Denied

real jetty

i mean, when i put my browser's user agent i get denied

when i leave it empty it works

slate nacelle
real jetty

thanks!

outer raven

Can we please revert https://github.com/discordjs/discord.js/pull/7759 ? This PR just sounds authoritarian as it is restricting users to one method of doing things where the other one is perfectly valid and not "confusing" at all unlike the PR originally claimed. One can send rest params and it will work and they can also send an array and it will also work, if anything the confusion is on the jsdocs, not the typings and that can be improved without needing this

copper laurel

This was discussed a lot before the PR even went up. We were frequently observing users in support passing arrays since theyre a naturally intuitive container object in Javascript and one that even beginners understand. So for that reason, it was important to support arrays.

As for why we no longer support rest parameters, supporting both forms of input has a performance impact - it becomes necessary to flatten or otherwise unwrap the array, if used, from the rest param argument. We are still looking at options to improve performance in that area though to see if supporting both can be viable

outer raven

from the people that say the performance impact that validation has on builders is negligible and then say rest parameters are an issue that sounds quite hypocritical if I'm being honest

and honestly this diff from v13 to current v14 just looks ridiculous at this point

- embed.addField("test", "abc")
+ embed.addFields([{ name: "test", value: "abc" }])
copper laurel

There's also an argument to be made for an inconsistent interface if it accepts two different forms of input, but I know we've been over that on other topics and will probably disagree on it. Can I ask if/why you have a preference for rest params, or why its important to you to see both supported?

outer raven

because 90% of the time I'm adding 1 field to my embed and being forced to put that 1 field in an array is just annoying and to me looks like it would impact performance more than using rest params would. For the situations where I'm adding more than 1 field it's still way more practical to pass the objects consecutively rather than throwing them into an array as that slows down development time by quite a lot (specially for someone using a portuguese keyboard like me where the [] are quite annoying to reach)

copper laurel

to me looks like it would impact performance more than using rest params would
Supporting rest params OR an array are exactly equivalent performance wise, since rest params become an array argument anyway. Supporting rest params AND an array is where the performance hit is introduced, since thats where we need to do args.flat() or Array.isArray(args[0]) ? args[0] : args or something.

I can't really comment on the portuguese keyboard thing since I don't use one, it doesn't take me any time at all to hit [] really

In fact a good IDE will put the ] in for you if you hit [ lol

outer raven
copper laurel

Not if we also support arrays

outer raven
copper laurel

...args, if receiving an array, is [[arg, arg, arg]] vs [arg, arg, arg] if rest

It becomes necessary to flatten, unwrap it, whatever

To be clear I have no preference either way on if we support both or not, I'd be fine with the performance hit of doing so. But I was in strong support of it not being only rest params because it was clearly unintuitive for users and yeah, the docs display for it sucked

outer raven
copper laurel

Yeah that was the other option I listed

Array.isArray(args[0]) ? args[0] : args

Which I personally am fine with

inland lotus

I believe supporting both array and rest would be a good option, if the issue is with typings/docs it could be easily solved by adding a note or making jsdocs..well better

visual hornet

I feel this becomes an issue when multiple arrays are passed, feels like that could lead to some unexpected behaviors
and i feel with accepting only an array, the old singular addField may become requested to avoid having to wrap a single option like rodry mentioned

outer raven

multiple arrays would never be passed

what

inland lotus

what if someone, for some reason, does .addFields([...], [...])

visual hornet

oh wait, misread some stuff, disregard that
thought we were having rest and array support and were transitioning to array

copper laurel

Yeah passing multiple arrays is just incorrect

inland lotus

meh, true

copper laurel

Although flat would work for that, we're certainly not extending support to a third set of rest-arrays or some shit

Yeah tbh my first response to embed.addFields([{ name: "test", value: "abc" }]) was something like "oh so what" but the more I look at it I kinda get it lol

visual hornet

yeah i thought it was going like how setAuthor/setFooter got deprecated

outer raven
outer raven
inland lotus

isnt the only issue with supporting both rest and array is docs/typings? or is it performance issues?

copper laurel

Theres a small performance hit

visual hornet

when addField was removed, addFields was still just the rest parameter of objects, so it was arguably trivial to just wrap it up, but now with the array it just seems like it's a bit much for a single field

inland lotus
copper laurel

¯_(ツ)_/¯

inland lotus

yeah I dont see an issue with supporting both lol

visual hornet

i think if supporting both does happen then it might lead to the thing i mentioned with multiple arrays, since although it's not typed like that, users using javascript unfamiliar with the stuff here might just assume that, arrays are valid, rest is valid, why shouldn't rest of arrays be valid
might just be a moot point though

outer raven

I don't understand why anyone would come to that conclusion

inland lotus

I mean, at first instinct you would either choose to pass one array or rest, only if you're doing it on purpose would you use a rest of arrays 🤷

outer raven

also that already happens in v13 and I've never seen that level of stupidity yet

also why can't the users that pass an array simply add "..." to the start of their argument tree?

wouldn't that make everyone's life easier?

copper laurel

Sure but they can just add [] too

Except Portuguese people I learned today, sorry about that 😛

As an experienced developer, I agree, ...arr is easy, but it really isn't a well understood concept for most people

outer raven
copper garden
copper laurel

back to singular methods? 😄

quiet viper
dawn merlin

but imo, yes the argument to add singular methods back would be valid now if we were to stick with arrays only

copper laurel

The argument not to use builders at all because you can embed.fields = arr or embed.fields.push(...arr) or embed.fields = embed.fields.concat(arr) or embed.fields = [...embed.fields, ...arr]

JSON/JS objects superior, supports both arrays and rest

outer raven
copper laurel

I mean... this is an interesting debate imo

dawn merlin

bc for rest params it's the same method just with an 's'

copper laurel

Because we're not "paying" but people do spend time (and time is money) to maintain it

"alternatives" are inconsistency, maintenance overhead, in some cases code duplication

outer raven

addField is a method that requires no maintenance after it’s implemented. It’s caused more maintenance overhead to deprecate, change syntax and then remove it than it would’ve to just… leave it alone

inland lotus

with all honesty, removing singular methods was uncalled for, it was perfectly fine as-is

outer raven

Arrays vs rest is 1 line to support both and it’s over with, you don’t have to change that line ever again and you only need to care about whatever else the function does if anything

copper laurel

yeah im fine with supporting rest

outer raven

Once again we’re back to creating problems to justify solutions that didn’t need to exist in the first place :/

dawn merlin

well the original issue was forcing people to use rest over arrays and not allowing both

copper laurel

the solution is not using builders

dawn merlin

like so many people were passing arrays in bc they didn't know what rest was and it wasn't expected

inland lotus

all these the builder changes would force someone to not use them and just use raw objects lmao

copper laurel
outer raven
inland lotus

I have also switched to json instead of builders

copper laurel

Builders are good if you want your input validated

UnsafeBuilders are, imo, pointless over objects

outer raven

No, builders are good for type safety and code organization, I could not give one singular fuck about validation yet I use builders everywhere in my code

copper laurel

const embed: APIEmbed = {}

typesafe

inland lotus

just type it as api embed, yeah

copper laurel

We have object types

outer raven

In fact builder validation is just stupid, the api validates it for ya

outer raven
copper laurel

yes

copper garden

The point of client side validation is to avoid 400s responses

inland lotus

well, as the developer you should know what types of params you're passing to builders, so is validation really needed in that case?

outer raven

And why are those bad?

dawn merlin
copper garden

There is a rate limit for requests resulting in a 4XX

visual hornet
outer raven
outer raven

And not let it sit there

visual hornet

the validation tells you to fix your code
is that not what it means

copper laurel

There isnt actually a type for the camelCase djs EmbedBuilder constructor param that I can see

outer raven

So do 400s

copper laurel

But assuming we made one you could typesafe using that

visual hornet
outer raven
copper laurel

Thats not typecasting

outer raven

So you still have to fix that asap

analog oyster
outer raven

Exactly

visual hornet

reactively/proactively with regards to sending the request

copper laurel

How is it less safe, TS wont let you assign props to that object incorrectly

outer raven
inland lotus

typescript fixes all issues with type safety and all that, purely validation is for runtime validation I believe

copper laurel

I havent

outer raven

It’s never as bullet proof as a function arg

copper garden
outer raven

Again, why does that matter

It’s an error you’d have to fix regardless

So having validation is the same

copper garden

Doesn’t consume global rate limit nor the 4XX limit

analog oyster

ik it does, but builder validation isnt being fixed proactively

outer raven
analog oyster

because it happens at runtime

copper laurel

This argument is getting really off track

outer raven

And, if you were following the chat, I said you wouldn’t let those errors accumulate so you’d fix your code before hitting limits

copper laurel

You praise the benefits of typesafety in TS but you dont praise the benefits of input validation

dawn merlin

idk even know at this point, I'm just over builders in general. Note to future me: don't make builder PRs

copper laurel

lol

dawn merlin

like im so done with all of that shit

inland lotus

remove builders altogether meguFace

outer raven

Revert back to v13 djs builders happy

dawn merlin

I don't even use builders so idk why I even get passionate talking about them

let alone make PRs for them

inland lotus

I have started using json when I saw the changes to builders in v14...v13 builders were better imo

visual hornet
outer raven

Only if you’re sending thousands of reqs per second lol

Either way

visual hornet

doesn't seem that unlikely for me for that kind of erroneous code

outer raven

It would be much easier to have what we had in v13 where builders organized data nicely, didn’t validate and supported all sorts of input for everyone’s likings

copper laurel

✨ objects ✨

inland lotus

just use typescript

analog oyster

not really the point of this conversation, but allow me to show this

dawn merlin

but anyways make builders how you want them to be, I don't want to be involved with something I don't use anymore, it's tiring trying to be involved with them

visual hornet

why did we have builders in the first place again? to go with rest?

dawn merlin

can't remember tbh

analog oyster
outer raven
bitter peak
copper laurel
outer raven

Ahh

analog oyster
copper laurel

In the sense that unsafe could have done 98% or 102% that many ops

analog oyster
outer raven

Right

inland lotus

I will just put what I would want builders to be..

1 - either support both rest and array, or bring back singular methods
2 - stop removing functions that were working perfectly before
3 - and bring back v13 builders

outer raven

Ok then ye builders are way worse than I thought wow

copper laurel
outer raven
analog oyster
inland lotus
copper laurel

I'll do a PR that brings back the singular method and make it require a length=1 array OMEGAlul

copper garden

Unions just create unnecessary complexity in the method just to bring it back to a single type or otherwise handle each case

copper laurel

The reason why builders exist is basically:
SlashCommandBuilders existed with input validation (and are good)
/builders now exists over there as a weird little thing with no other builder classes in it
discord.js has a bunch of builder classes in it so we move them to /builders and add the same sorts of validations
Those classes end up fairly different to what people are used to because the standards for builders (e.g. snake_case etc) don't align with discord.js
They have to be reimported back into discord.js to avoid code duplication
They have to be extended in discord.js in order to align with discord.js standards while still trying to minimize code duplications

Interestingly enough, if we fully aligned something like EmbedBuilder with SlashCommandBuilder, the interface would be embed.addField(field => field.setName('foo').setValue('bar'))

inland lotus

now if you left builders alone in djs that wouldnt be the case, as you moved them to builders, then moved them back to djs to allow camelcase...why all that hassle?

copper laurel

It was essentially a sort of repository design/architectural philosophy decision I guess

Not one I'm trying to defend or justify, only explain

inland lotus

¯\_(ツ)_/¯

just from personal experience, these builders changes werent needed and moved me to use objects instead so... yeah ig these decisions werent so good imo

wild flax

If you moved to objects, they were good enough

tacit crypt

Like the amount of headaches that builders have caused over something that's meant to help you is getting tiring :p
If you don't care about the benefits of validation or you do validation yourself, use unsafe builders or objects. Otherwise use validated builders

Builders are meant to help you not send invalid requests to Discord, which depending on your bot throughput and amount of invalid data sent could outright ban you for at least an hour from doing anything with the api

analog oyster

If you don't care about the benefits of validation or you do validation yourself, use unsafe builders or objects
... unless if you want to use the features that were added in the extended builders in djs, if so you'll have to use objects as the "unsafe" builders arent being extended in djs

tacit crypt

For the 90% of users that's fine and probably much better than getting some obscure error from discord (which I've seen plenty of people not be able to read)

tacit crypt

Which affects only component builders currently

(ala customId and the gang)

analog oyster

embed setColor accepts hex strings
button setEmoji accepts everything that parseEmoji does

tacit crypt

(which is something that should be used very sparingly as it does no validation)

outer raven

Wdym it does no validation?

analog oyster

values passed in the constructor arent validated

tacit crypt

What's the use case for hex strings? It's the same as the number variant, only replacing 0x with a hashtag

analog oyster

i dont use builders, i was just pointing out the inconsistency

tacit crypt

For setEmoji we can probably add shortcuts/overloads for it but there's no inconsistency as builders are working with raw api data

analog oyster

but since its available on the regular EmbedBuilder, i assume there are valid use cases

analog oyster
solemn oyster

Unsafe builders are meant to be bare metal, they're to be used only by very advanced/experienced developers who know what they're doing, not by the regular users.

And the target public they have doesn't whine about little things such as casing or easily fixable abstractions.

analog oyster

👍

tacit crypt

Creature comfort shortcuts, like setEmoji accepting a string, not just an object, fit more in djs than builders. At their core builders are meant to work with raw api data in a nicer, chainable way (see application command builders), and also bring in validation so the API limit isn't drained from possibly invalid data sent.

outer raven
tacit crypt

API doesn't return hex values for anything

The only time it did was for the user accent color and that was shortly after changed to a number

outer raven

Not Discord

Others

thin obsidian

Is it best practice to just use objects instead of the e.g addFields function?

mighty bough
thin obsidian
mighty bough
zenith oracle
thin obsidian but what's considered best practice

It really depends on you. Some prefer to use JSON objects because they are faster, easier to adapt to new features and comfortable if you want to statically send an embed (so you already have all the properties before); others prefer to use Builders because they allow you to validate all properties before sending the embed and are more convenient for example to add fields or do something else dynamically

golden mortar

JSON. nothing about builders is more convenient or easier than a normal object

copper laurel

This is not the channel for this discussion, and if the question is for best practices than the tool that provided validation would be the one to recommend, personal biases aside

outer raven

So what's the takeaway from yesterdays convo about builders and rest vs arrays / singular methods / whatever the hell we talked about here?

can we bring back both rest and arrays and singular methods?

copper laurel

I dont think that was the takeaway

We could bring back rest or singular methods. If we bring back rest, addFields wont require an array and operates as a single method also

If we dont bring back rest, singular methods have a more distinct purpose that isn't being duplicated by another method

wild flax

I like the idea where we don’t do either

outer raven

You also like the idea of breaking every single thing in the library for the sake of it so why bother right

tacit crypt

You're the one using an in dev version of WIP software

outer raven
tacit crypt

There have been very few if any poor decisions, and most have changed since. And it does matter because this isn't the final product so things are bound to change

outer raven

it would make sense to say that if you had any plans to change these things, but it doesn't seem like you do

solemn oyster

It's simple, it all comes down to iterative design and development, we can't make perfection in one go, also should be expected to be unstable considering /builders is literally v0 at the moment

copper laurel

This is not a constructive conversation, rather just accusations and insults towards the maintainers of the library.

ember steeple

@outer raven I agree with you, don't worry 👍

but i think these changes doesn't change anything for users you can easily change rest parameters to array

outer raven
fading magnet

are they gonna kill off v13 like they said with v12 but never did

unique axle

i have no idea what you are trying to say with that.
we do not provide support for older versions some time after a stable version is released (if you still want to use v12, feel free, but don't come here for support with it).
we said that v12 will stop working with some new features discord releases (messages in voice channels, to name one) or (ultimately) if discord decommissions the API version used in older versions.
discord continuously pushing that deadline back has nothing to do with us.

old versions (v12 and earlier) will not receive any updates, new features or bug fixes and will break if certain unforeseen things are introduced (messages in voice channels, to name one).

fading magnet

like oh yea v12 is gonna be dead in a few months!1!!1

unkempt bear

Not anymore. It's been extended

That being said, move to v13 :)

unique axle

cool, so if you know this doesn't need to be discussed - so why even bring it here in the first place

ember steeple
solemn oyster

It's not a matter of which code looks better, but which code is the most expected for end users

Most people expect arrays when seeing addFields, while almost nobody expects rest params to be used there, only very advanced users would

fading magnet

well know i do bc im here

outer raven
warped crater

it pains me that just about all of those conversations start with insults or "there's no reason to do X", constructive criticism has become so incredibly rare

even in this instance you're being told the new interface for the method is meant to be more intuitive to people new to the library or language in general and your response is "but I've been using the library since the time when it wasn't this way and as such it feels less intuitive to me" which just about boils down to "change bad"

unkempt bear

It's the same with every new release. People choose to collectively hate it, they get convinced to change, realise that the newly added implementations aren't utter horseshit and actually contribute to the library's experience, and make that release their new basis for future criticism to change

outer raven
warped crater even in this instance you're being told the new interface for the method is mean...

no I never said "change bad", in fact, change is often good when it has reasoning behind this other than pure personal preference. They are saying arrays are more intuitive to new people, and I simply argued that, when I was new, rest parameters were more intuitive and explained why.
Change is more than welcome if it benefits people in the long run. If it's changing for the sake of changing it's stupid and should be avoided. The library should broaden people's choices and not limit them to what the maintainers like

ember steeple
outer raven

I could but I don’t know if it’d be worth it because the maintainers outright ignore the PRs they don’t like and leave them sitting there forever

So if we learned anything from this convo is that the same would happen to that PR if I made it

strong hull
outer raven

the shocker here is them not caring about what the community wants and going against it

wild flax

Oh don’t worry, we do care.

Just the bigger picture and not so much a vocal minority

slim moat
visual hornet

aren't there only like 4 constructors that you're supposed to use

wild flax

I stopped arguing about this a long time ago

Development is iterative

If you use a -dev version, no guarantees youll have to change all the way until things land in stable

and if we can't land on a good compromise, and change things back and forth 3 times, youll have to suffer and sit it out

At the end of the day, 90% of the user base won't care though, because they don't get to have to change 4-5 times, they have to change 1 time, from v13 to v14

raven juniper

Or just pin to a commit if you're using dev for a feature

solemn oyster

JFYI, nothing stops you from making your own class extending our builders that take spread arguments, Rodry. Something as trivial as addFields(...fields: Parameters<EmbedBuilder['addFields']>): this doesn't need any extra maintenance.

golden mortar

is Collector.endReason broken?

import { Collector } from 'discord.js';

const collector = new Collector({}, {
    filter () {
        return true;
    }
});

setTimeout(() => collector.emit('collect', { hi: 'there' }), 1);

for await (const [i] of collector) {
    collector.stop('end reason');
}

console.log('end reason is:', collector.endReason); // undefined
void rivet
golden mortar

having issues with InteractionCollector too, it's documented too, just doesn't work

void rivet

do these just not support custom reasons

looking at the code i don't see this, nor do i see MessageCollector using custom reason

visual hornet

they're supposed to but they only work for the end event
i've brought up endReason being kinda... arbitrary before and gotten not any response
#archive-library-discussion message

golden mortar

okay so it's just broken, ill see if i can make a pr to fix it

loud jayBOT
outer raven
copper laurel

But then it wouldn't be passing the message id at all when it is available

And then what would be the point of coalescing it later?

outer raven

idk why that workaround is there but it looks useless

copper laurel

The interaction that sent a message is not the same interaction as this quite often though

outer raven

so is this meant to be like if you execute a command then press a button on it and update the button response it returns an interaction response with the original command?

doesn't make sense to me

and you can use buttons on messages

copper laurel

It passes both though

The first parameter is this, why would it be passed twice

outer raven

yeah but why

copper laurel

I didn't look into the functionality, I fixed a bug

outer raven

the class does that anyway

copper laurel

Yes I know

outer raven

if id is null it uses the interaction ID

copper laurel

if the optional message id isn't passed

Which certainly suggests there a valid reason for it to be passed

outer raven

what could be the use case for wanting the id of the command ran originally in the return value

also that description in the docs is misleading

copper laurel

I don't know I didn't build those classes. I fixed a bug and have no interest in arguing about it

outer raven

but i wanna know if this is a bug worth fixing or not

copper laurel

Well the library isn't broken now. Seems worth it to me

outer raven

what kinda answer is that

copper laurel

One that comes from frustration that I seem to have to justify why I made a simple change to prevent the library from crashing. I was not looking to redefine or change any functionality. I didn't design that functionality. I just prevented a crash which certainly seemed "worth fixing" then, and still seems worth it now

outer raven

all i asked was if there was another bug here jeez

oak quail
wild flax

rip

real jetty

⚰️

green valley

why does discord.js still use node-fetch v2.6.6 and not 2.6.7

warped crater

because 2.6.7 was just a security fix for something that discord.js wasn't even effected by

i.e. the question you should be asking "why should discord.js update to 2.6.7?"

and the answer is "there is no reason"

cursive jackal
green valley

but dependabot is too worried for me

warped crater

unlucky

vernal rose
green valley

yarn didn't did that

oak quail

is it intended that GuildExplicitContentFilter and GuildDefaultMessageNotifications aren't exported? seems like an oversight

also

this should be ActionRowData right?

dawn merlin

uh which branch of the docs are you looking at?

oak quail

main

also this in SelectMenuComponentOptionData

dawn merlin

In that case yeah it should be ActionRowData

dawn merlin
oak quail

in the types, SelectMenuComponentOptionData.emoji is ComponentEmojiResolvable, which is APIMessageComponentEmoji | string, but looks like it doesnt actually handle strings in v14?

dawn merlin

In discord.js it does in builders it doesn’t

oak quail

strings worked in v13 but in v14 looks like it passes directly to Discord instead of serializing to the object

i am not importing builders anywhere in my code

dawn merlin

Hmmm it should work, it was added

oak quail

this is in an object being passed directly to interaction.reply

dawn merlin

Oh you’re using the json not the builder

Iirc json doesn’t take emojis directly as strings anymore

oak quail

d.js passes it to a builder internally ```js
const components = this.options.components?.map(c => (isJSONEncodable(c) ? c : new ActionRowBuilder(c)).toJSON());

outer raven

Getting a 405 error on the permissions set endpoint saying "Method not allowed", how do we set permissions now? Does djs need to update?

analog oyster
ruby terrace

working on it, you cannot set permissions without an oauth2 bearer token

ember steeple
outer raven

Well but my bot was setting permissions 2 hours ago and now I need to change everything because Discord decided it would be nice to put everyone on a crash loop?

@copper laurel Discord didn't deprecate the endpoints, they removed them

ruby terrace

that's not how that works, if they removed it, you'd get a 404

copper laurel
outer raven
ruby terrace

that is correct yes

copper laurel

Yeah not really a deprecation either in fairness

Since its an immediate breaking change

outer raven

I thought Discord was against these kinds of immediate changes but god was this update dropped awfully

ember steeple
copper laurel
ruby terrace

We literally warned them against doing this too

opaque vessel

Angry

hearty nymph

Disabled the batch editing endpoint (PUT /applications/{application.id}/guilds/{guild.id}/commands/permissions).
I see that it is already being discussed, but why would you do that.

So, there's no proper way for bots to edit command permissions (yet), correct?

visual hornet
hearty nymph
mighty bough

is stable branch still receiving updates?

honest barn

it will once a new update gets released (v14)

zenith oracle

13.7 should be the next stable version actually iiuc

honest barn

it should but it wont go to the stable branch, i guess it would be in the 13.x branch

copper laurel

Nothing ever "goes into" stable unless youre Discord releasing a breaking permissions update

There will be a new v13 version that becomes the "stable" release when it's ready

honest barn

i meant getting something merged into the literal stable branch

copper laurel

We'll likely also have a stable v13.7 and a stable v14.0 at some point

honest barn

from main

copper laurel

There's different ways to release - you could merge the v13 branch into stable, delete the stable branch and re-tag/branch it off v13

honest barn

o

outer raven

@ruby terrace does this mean your PR will add support for user tokens or what exactly is this about?

ruby terrace

bearer tokens, as it says, user tokens are not bearer tokens

outer raven

they are ?

bots use bot tokens, users use bearer tokens

ruby terrace

users use tokens, no prefix

outer raven

Then what’s a bearer token

faint grove

I think a bot token on behalf of a user

You basically give the bot permissions with your user account, which must be in the server and have permissions etc.

unique axle

this is absolutely not a discussion for here, tbh

faint grove

True, sorry.

tacit crypt
outer raven

Why would djs support that now if it doesn’t anywhere else

ember steeple
warped crater

because there isn't much to "support" in oauth land regardless - and what there is is generally encouraged to be separate from your actual bot (e.g. dashboards with discord auth) so there's no need. in this case those are endpoints that require bearer tokens that you might actually find yourself wanting to use within the bot itself.

tacit crypt

Such as add guild member

outer raven

Will there be guides on how to get such token and how to update permissions through the bot?

copper garden

There's alr a section in the guide for obtaining an OAuth token

unique axle

i think (and hope) that devs wanting to do settings for peoples permissions is so much of an edge case that it's not worth it to explain the entire payload in all its intricacies and the above OAuth section is enough

will ofc. have to explain member and dm defaults, once those are implemented and released in the library vs. the permission endpoint

outer raven

my bot is only in 1 guild so it'd be nice if I could set the permissions on that guild automatically and I currently don't know how to get that token so that's why im asking

it's an edge case tho ik that

unique axle
outer raven

alright, thanks

outer raven

apparently djs can throw this type of error? I've never seen this before and it doesn't look intentional, does anyone know what happened here?

tacit crypt

internet had a stroke

Not related to djs either, its just that when we tried a request, your internet said shrug (you can read up what ECONNRESET means online)

outer raven

hmmm weird cuz this was on a hosted server but alright, I thought djs wasnt meant to throw raw node-fetch errors but thats fine then

inland panther

Isn't this description of the event slightly misleading, since when partials are enabled even if the message isn't cached before the Action, a partial is cached in it's place
Saying it this way doesn't make this completely clear

tacit crypt
inland panther

Generally speaking the documentation of changes in behavior due to enabling partials is lacking in Client events

Or am I missing something Thonk

warped crater

this goes for the gateway as well

nothing is gonna be more informative than "econnreset" coming straight from your OS

golden mortar
outer raven

seems like not even the code knows

solemn oyster

It's meant to test extreme cases

golden mortar
golden mortar
warped crater

probably just that, if non-sensical cases are handled "correctly"

golden mortar

partially a bug in undici that I accidentally caused, but the test just seems pointless. If discord ever sends a 601 status code, they are doing something very, very wrong

solemn oyster

Or could be April Fools

timber owl

I noticed that the README mentions zlib-sync and erlpack as optional dependencies, but neither discord.js itself nor any of its dependencies have them as optional peer dependencies. Should they be added to the package.json?

solemn oyster

No

timber owl

Why not?

solemn oyster

Because npm v7 pulled a funni and tries to install peer deps

timber owl

I've been using yarn pretty exclusively so I guess I wouldn't know. Still, I think it would be helpful to mention what version(s) are accepted

solemn oyster

Which makes them behave the exact same as optionalDependencies

Any version, tbh

If there was incompatibility with a certain version, we would probably add code to handle it, but they never change

timber owl

I just saw that nothing in my lockfile mentioned them and I almost just assumed they were unused

I suppose one thing you could do, something I've seen other libraries do, is just add them to peerDependenciesMeta but not peerDependencies itself

void rivet
golden mortar
timber owl

Why not? I just tested it with npm 6, 7, and 8, and it seemed to work just fine in all of them

well I tested it out with a package of my own that has

  "peerDependencies": {
    "typescript": ">=3.6.2"
  },
  "peerDependenciesMeta": {
    "typescript": {
      "optional": true
    }
  }

and installing just that and nothing else, with npm 6, 7, or 8, did not install typescript. I fail to see why this situation is any different

outer raven

what would be the benefit of adding that then

oak quail

@dawn merlin I'm confused, channel flags have nothing to do with permissions v2

the pinned flag is for forums

dawn merlin

Oh lol my bad I knew it was something newer I’ll fix that

real jetty

They do, also this is not the channel for support it's #djs-help-v14, this channel is only for developing the library

real jetty

I feel like it's some sort of bug now, because if i type 5 in the value of the key type, it doesn't give any type errors but if i use the enumResolvers to do the same thing it doesn't work

client.application?.commands.create({
    name: 'ping',
    description: 'tells The ping of the bot',
    options: [{
        name: 'ephemeral',
        required: false,
        description: 'Should the message be only visible to you or no?',
        type: EnumResolvers.resolveApplicationCommandOptionType('BOOLEAN'),
    }]
})
golden mortar

at that point it's just easier to use the actual type than enum resolvers, it even tells you its name

wild flax

Its because theres a version mismatch currently probably

between builders and djs

outer raven

@tacit crypt here’s an argument for supporting rest params if you want: Array.prototype.push takes rest params as input so people might expect these functions to do the same, since fields, options, etc are arrays of objects, and they can’t modify the arrays directly because they’re not exposed

real jetty
dawn merlin
real jetty I feel like it's some sort of bug now, because if i type `5` in the value of the...

Testing it on my end it looks like an issue with the types in general. It's caused by the fact that enum resolvers don't give specific enum type. When ts receives the type it gets ApplicationCommandOptionType but it wants ApplicationCommandOptionType.X. The only workaround I can think of is to cast the resolved type to the type you want, but at that point you're just better off importing the enums shrug

We could use mapped types to make the enum resolvers more specific, but not sure how feasible it is.

golden mortar
real jetty

I meant 10 different enums from one import actually like ComponentType,
ApplicationCommandOptionType and many more

vernal rose
vernal rose
golden mortar
real jetty I meant 10 different enums from one import actually like `ComponentType`, `Appli...

I understood a little after I posted. It's entirely a nitpick since enumresolvers still have you writing more for the same functionality. They are entirely useless;

  • types aren't correct
  • don't make transitioning from v13 easier
  • are longer than discord-api-types' imports
  • don't benefit js users... they'll either hardcode values in or use discord-api-types too

I really don't see the purpose of having them in the lib 🤷‍♂️ Honestly I think it'd be really great just to remove them entirely before they land in v14

real jetty

That's a good point, but they can be useful in some scenarios to shorten the code. For example, if we're writing a component-making command and the arguments in that command need to know which type of component we need to make, to resolve what argument they've given, we can use enumResolvers.

golden mortar

I have that in my bot (a Component factory), on a v14 branch, using only discord-api-types.

real jetty

Open source?

Nvm

golden mortar 
import {
  ComponentType,
  ButtonStyle,
  type ButtonOptions,
  type APIButtonComponent
} from 'discord-api-types/v10'

const Buttons = {
  // ...
deny (label = 'deny', id = 'deny', options: ButtonOptions = {}): APIButtonComponent {
        return {
            type: ComponentType.Button,
            style: ButtonStyle.Danger,
            custom_id: id,
            label,
            ...options
        }
    },
}

is the general gist; creates a danger button with a label & id (and optionally other parts). This codebase is from v12 but in v14 style so I would probably change it to 1 object parameter instead if I could re-do it

zenith oracle

Yeah I have something similar... Honestly enum resolvers are a bit more confusing and don't seem to help anyone. Although I don't see why they should be removed, if someone (who uses js probably, not ts) is more familiar with them instead that enums, just let them use those resolvers

golden mortar

theres no need for anyone to become accustomed to them because they only exist in v14. now's the perfect opportunity to remove them before it's a semver major change

dusky crown

Hey guys, I see there’s permissions on commands, is there any update on that on discordjs?

unique axle
dusky crown

Thanks

mighty bough

do v14 types still need fixing?

void rivet

as in? what specifically?

vernal rose
mighty bough

this doesn't work, did I make a mistake?

components: [
  new ActionRowBuilder().addComponents(
    new ButtonBuilder()
      .setLabel("Send Embed")
      .setCustomId("send_embed")
  )
]
opaque vessel

Yes

vernal rose
ember steeple

can someone tell me why there's ban and kick method on GuildMemberManager but theres no timeout method

please

haughty cypress

there is

<guildmember>.timeout()

opaque vessel

There's a ban method because there's a ban route
There's a kick method because there's a kick route
There's no timeout method because there's no timeout route. It's in the edit route

ember steeple
outer raven
visual hornet

it isn't its own route

ember steeple
ember steeple
visual hornet

you could just use the existing edit method

ember steeple
visual hornet

you don't need to let me know your own decision lol

real jetty

Hi there

This is somewhat trivial, but have you thought of adding yourself on https://github.com/discord/discord-open-source ? Entries there are used to populate the list at https://discord.com/open-source

At this time, we are accepting communities which meet the following criteria:

Your community is not Discord-focused (for example, Discord bots or modifications are not accepted).
Your community has at least 1,000 members, or the GitHub repo has at least 1,000 stars.
Your community adheres to the Discord community guidelines.

visual hornet

im pretty sure a dapi wrapper counts as discord focused

real jetty

Ahh alright then, my bad

outer raven

why is djs using experimental node features? Shouldn't we wait for them to come out in stable versions of node?

opaque vessel

I think that specific one is stable in a later Node.js version. Nothing was changed between the phase so it should be okay

golden mortar

or like is there anything the maintainers have talked about in private that would hold it up

copper laurel

No specific internal discussions thats Im aware of, probably just needs reviews

uneven blaze

Is there some kind of roadmap to the ts rewrite plans?

vernal rose
honest barn

since there's only one pr left in the 13.7 milestone which has been sitting there for two months, wouldn't it be better if d.js 13.7 was released without that pr and then include it in a 13.7.1 release?

there are a lot of new features people are waiting for like modals

unique axle

milestones are not dated and progress is not necessarily indicative of a release

ember steeple
honest barn

oh

unique axle

it's not lying, you just misinterpret milestones that have no dates

tacit crypt

ws you mean?

honest barn
void rivet

No one said that

You can say backports weren't initially planned but plans change and they are here
The milestones mean nothing, i can legit add prs to it and they'll be in the milestones. Again plans change and before that pr gets merged more prs can be made, nothing is really certain, looking at a constantly changing list of prs doesn't tell you anything about when the thing will be released

honest barn

hm makes sense

stray shell

hello, why in the stable version of discord.js the StaticImageURLOptions part has more dimensions than in the main? example: 16, 32, 56, 64, 96, 128, 256, 300, 512, 600, 1024, 2048, 4096.

unique axle
stray shell

thank you, information

bitter peak

was the oeis link really necessary lol

unique axle

yes, oeis is nice and more people should know about it firY

timber owl

is there any reason type Snowflake is defined as string rather than, say, `${bigint}`? I realize it would be semver-major to change but I'm curious if there's any motivation beyond supporting TypeScript <4.1

real jetty
timber owl

huh, good to know

thanks

vernal rose
solemn oyster
tacit crypt

Rioted is a strong word but yes. It was that before, many people complained about it because it made code require far more casts so we reverted it

solemn oyster

Honestly, a lot of things (like, the vast majority of things) didn't require cast at all. It's more an issue with third-party libraries that didn't use the type (which we cannot control at all), and with the users' own code being typed to emit strings instead of Snowflake's

opaque vessel

Being back stringified big integers :c

wild flax

@golden mortar not sure if you know more but

I can't seem to get proxying via undici to work, except for one method

lets presume this is my file

const process = require('node:process');
const { GatewayIntentBits } = require('discord-api-types/v10');
const { token, prefix, owner } = require('./auth.js');
const { Client } = require('../src');

// eslint-disable-next-line no-console
const log = (...args) => console.log(process.uptime().toFixed(3), ...args);

const client = new Client({
  intents: [GatewayIntentBits.Guilds, GatewayIntentBits.GuildMessages, GatewayIntentBits.MessageContent],
});

first I tried the obvious way doing

const process = require('node:process');
const { GatewayIntentBits } = require('discord-api-types/v10');
const { setGlobalDispatcher, ProxyAgent } = require('undici');
const { token, prefix, owner } = require('./auth.js');
const { Client } = require('../src');

setGlobalDispatcher(new ProxyAgent('http://localhost:3000'));

// eslint-disable-next-line no-console
const log = (...args) => console.log(process.uptime().toFixed(3), ...args);

const client = new Client({
  intents: [GatewayIntentBits.Guilds, GatewayIntentBits.GuildMessages, GatewayIntentBits.MessageContent],
});

which didn't work

so I tried doing

const process = require('node:process');
const { GatewayIntentBits } = require('discord-api-types/v10');
const { ProxyAgent } = require('undici');
const { token, prefix, owner } = require('./auth.js');
const { Client } = require('../src');

// eslint-disable-next-line no-console
const log = (...args) => console.log(process.uptime().toFixed(3), ...args);

const client = new Client({
  intents: [GatewayIntentBits.Guilds, GatewayIntentBits.GuildMessages, GatewayIntentBits.MessageContent],
  rest: {
    agent: new ProxyAgent('http://localhost:3000')
  }
});

which also didn't work

so I went all the way down and did

const process = require('node:process');
const { GatewayIntentBits } = require('discord-api-types/v10');
const { ProxyAgent } = require('undici');
const { token, prefix, owner } = require('./auth.js');
const { Client } = require('../src');

// eslint-disable-next-line no-console
const log = (...args) => console.log(process.uptime().toFixed(3), ...args);

const client = new Client({
  intents: [GatewayIntentBits.Guilds, GatewayIntentBits.GuildMessages, GatewayIntentBits.MessageContent],
});

client.rest.setAgent(new ProxyAgent('http://localhost:3000'));

and this seems to be the only way it works

I can't really explain why though, none of the things should be instantiated on import

and only on client creation

golden mortar

the first way (setting global dispatcher) is probably fixed in the newest undici, 2nd way is probably a bug, glad it worked in some capacity tho

i think the 2nd way should work too so ill try fixing it

wild flax

Sweats

newest undici means you need to update?

golden mortar

that's just a guess tbf, in 5.2.0 the setGlobalDispatcher was made completely global. before having 2 versions of undici (ie. using node's & undici dep somewhere else) wouldn't both work when you set a global agent

wild flax

ah

golden mortar

anyways, setting agent with RESTOptions should be fixed now

wild flax

The fix for the 2nd might actually collide with the fix for the first global dispatcher

golden mortar

How so? If no agent is provided per request and no agent is provided in the current RESTManager, it'll default to undefined. Undici will auto set it to the globalDispatcher then

it's not possible to test these changes either with jest because undici's mock system requires a global dispatcher to work lol

imo setting a global dispatcher should be discouraged especially since it can cause issues and unexpected behavior (ie. having node's fetch proxied because you set a global dispatcher in undici)

wild flax

you can easily test it with a tiny bot I put above

just put in some fake localhost

the manager will destroy and the process exit

wild flax

having the options work is good enough imo

thanks for fixing that

and have shared ratelimiting

👍

golden mortar

and now setGlobalDispatcher works, what a weird issue. Once I updated discord.js' undici it worked instantly

wild flax

I still can't seem to get this to work 🤔

'use strict';

const process = require('node:process');
const { GatewayIntentBits } = require('discord-api-types/v10');
const { setGlobalDispatcher, ProxyAgent } = require('undici');
const { token, prefix, owner } = require('./auth.js');
const { Client } = require('../src');

setGlobalDispatcher(new ProxyAgent('http://localhost:3000'));

// eslint-disable-next-line no-console
const log = (...args) => console.log(process.uptime().toFixed(3), ...args);

const client = new Client({
  intents: [GatewayIntentBits.Guilds, GatewayIntentBits.GuildMessages, GatewayIntentBits.MessageContent],
});

it connects fine

instead of crashing, like it does when I set the agent in the client rest options

undici is on 5.2.0 on both

also reinstalled with yarn

golden mortar 
(node:20284) ExperimentalWarning: The Fetch API is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
node:internal/process/promises:288
            triggerUncaughtException(err, true /* fromPromise */);
            ^

Error: connect ECONNREFUSED ::1:3000
    at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1195:16) {
  errno: -4078,
  code: 'ECONNREFUSED',
  syscall: 'connect',
  address: '::1',
  port: 3000
}

Node.js v18.0.0

it throws for me 😕

const { setGlobalDispatcher, ProxyAgent } = require('undici');
const { Client, GatewayIntentBits } = require('./packages/discord.js/src');

setGlobalDispatcher(new ProxyAgent('http://localhost:3000'));

const client = new Client({
  intents: [GatewayIntentBits.Guilds, GatewayIntentBits.GuildMessages, GatewayIntentBits.MessageContent],
});
wild flax

weird

I guess ill take your word for it

golden mortar

are you using client.login? I omitted it from my example

wild flax

Yeah

I may require undici below the client

But I doubt that’s it

Sec I’ll try your stuff

Yeah no I can't get it to work

lmao, I guess ill just take your word for it

its not like i need this use case

if it works it works

warped crater

wait, you still need a way to skip djs' ratelimiting right?

wild flax

if it returns no headers, theres no issue

warped crater

ahhh right, if it strips them its cool already

wow

end of an era

wild flax

I got the idea because I wanted to do some rest requests inside a worker with /rest, but then I thought I couldn't because the ratelimit isnt shared

but this way I can proxy inside the worker and on the main bot to the http-proxy

and have it shared

warped crater

yea I know the feeling, this is super nice

wild flax

and undici makes this really painless

while node-fetch its super painful and needs another dep

lol

warped crater

technically this was possible before with node-fetch with a hacky agent yea

wild flax

yeah, node-http-proxy-agent or something

warped crater

I looked into it before and noped out lol

hm, couldn't there have also been a client option to overwrite the API domain? (unless there is one already)

wild flax

yes but this is nicer

warped crater

yea f sure

this is the proper way

exciting

now to wait for a gateway pkg from you all :^)

wild flax

mmLol

lmao I just realized

that also solves the sharding issue have where they hit 409 on the /bot endpoint

can just tell them to install a proxy now

warped crater

owoah bless

ruby terrace
real jetty

Is there a reason why this can't be supported on api messages?

analog oyster

an APIMessage is a raw object from the api

real jetty

Like the one I showed, obviously not all of them can be reflected but some of them definitely can be.

golden mortar

lots of people don't know this but you can actually import InteractionCollector from discord.js and create your own collector. So much easier imo

dawn merlin
real jetty
vernal rose

EmbedBuilder from the builder isn't assignable in message send options. Will there any 13.7.1 release if so I can create a pr

dawn merlin

you should probably be using MessageEmbed in v13 for now

vernal rose

well maybe I'm can just cast it as most of the ppl won't use new builder with v13

analog oyster

does it not work if you use .toJSON() on it?

vernal rose
honest barn
outer raven

Is it possible to move the features part of the changelog to the top? The changelog for 13.7 is so big that dependabot cuts out most of the features so you can’t read it all

solemn oyster

You can just open the changelog, Dependabot also includes a link to the full changelog

manic cairn
analog oyster

the amount of commits in your pull request doesn't matter, they squash them when merging

manic cairn

ik but the first commit msg isnt even right at this point, im just disoriented and a dipshit today and not ready to follow up on it

clever summit

there's a new git option to use the pr title when squashing so it shouldn't matter

analog oyster

and if they really cared about the first commit msg, they can also just change it themselves when merging

lapis echo

You can literally edit what the commit message will be when using squash & merge

golden mortar
lapis echo
outer raven

dependabot shows the change log so you dont have to open a new tab

so the libraries should make their changelogs properly

manic cairn
golden mortar
dawn merlin

I'm spitballing here, but I wanna say it had something to do with the form data types? Pretty sure undici fixes that though

golden mortar

I made a pr to remove them 🤷‍♂️

dawn merlin

Does it compile without it errors?

golden mortar

it did on my side, the ci failed (just needed to import performance though, I think)

ci failed because of the other types issue 😐

is there any reason why i'm not getting these errors on my side?

dawn merlin

Are you compiling or running the tests?

Also it looks like it’s from djs not rest

golden mortar

I typically run yarn run format, yarn run lint, and then yarn run test in the root directory, so it should be catching those errors(?)

wild flax

Gotta build too

lethal echo

what a hell in changelog? backport backport backport...

I mean it's unreadable in changelog. I wanted to know quickly what are the changes, but I should click on 30 links and read what changed

solemn oyster

That's 7 links, not 30.

lethal echo

didn't know about it, thx

solemn oyster

Tip: you can also middle click links to open them in a new tab, makes opening them a lot faster 👀

outer raven

not on phone

but yeah someone thought it would be a good idea to make 7 PRs all with the same name

the issue is that its unreadable and not practical

of course

silent field

why wasn't v13.7.0 not announced? It was released on github and npm but never here.

glacial summit
lethal echo
wild flax

There’s no way to change this except to manually change it and force push, which is not an option either

So you’ll have to live with the occasional mistakes like that

long dew

the site libray is not working

im clicking on stuff and nothing happens

opaque vessel
long dew
worldly sail
opaque vessel

The typings should not include a number there. It will always be a string

tired tartan
copper laurel

It doesn't matter at all if people are still using a previous release

It will continue to work just like it did the day before the release

I don't closely follow the releases of most libraries I use - I build and deploy with a specific version that I know works for the code I wrote

tired tartan

that why 13.7.0 wasn't announced in #announcements, well good for you then ig

worldly sail
opaque vessel

Main doesn't use strings anymore, so that won't be a problem there. It should be the v13 branch, but I am currently unsure if we are making a new release for the version 13 line

worldly sail

okay, i won't pr then

oak quail
unique axle

"needs" is a bit of a strong word here

vernal rose
dawn merlin

I’m pretty sure docs doesn’t know how to represent protected methods

opaque vessel

Should all protected methods be hidden, or a badge stating they're protected?

Could probably tweak something about that

outer raven

Imo they should display as private since protected methods are private with extra steps

tacit crypt

They are most certainly not that. Protected method can be accessed by the class and whoever inherits the class. Privates cam be accessed only by the class that defines them

vernal rose
outer raven
tacit crypt

i mean even privates can, just that TS will scream unless you use [] accessing

fresh crater

hi guys

outer raven
warped crater

hello hello, I spoke a bit with @ruby terrace and was very intrigued by the idea of a @discordjs/proxy after the convo I had with crawl re twilight here a few days ago (now that undici has made it trivially easy to proxy with /rest)

loud jayBOT
warped crater

This I've started up a basic PR and would appreciate some general guidance as I work through it owoah

mostly I think what I'm interested in the most is what would be considered in/out of scope

twilight exposes metrics among other things, but that seems like something we wouldn't add to this

e.g. right now the only public function I have in mind is proxyRequests(token: string) => (req: T, res: T) => Awaitable<void> (where T are the proper types)

takes in a token - returns a middleware handler that passes things to a REST instance that makes use of that token

warped crater

working on a tiny implementation, my main concern so far is being unable to correctly forward headers and the status code without listening to the response event which gives me access to a Dispatcher.ResponseData - but that's a crazy hack

this can sorta be worked around by manually re-computing the status code and content-type, but it's one heck of a compromise

ideally there'd be a method on REST that can return a raw response, hm

golden mortar

why not just return your own Agent (maybe extending ProxyAgent) where you can get all of that info?

warped crater

hmm

could you exemplify

I can't really imagine how that'd work

warped crater

I could do that to copy over headers, status code n such, and then the body is obtained as it is right now

I'll def look into this, ty for the idea!

it's less hacky than the alternatives thats for sure meguFace

warped crater

yeahhh making an agent seems quite hacky as well 😬

@tacit crypt think you could give me some insight here if you're around?

the tl;dr is I somehow need to get a hold of the raw HTTP response that passes through REST at some point - since REST#request will only give me the body.

there's... 3 main options
a) I listen to the response event and wait for it to emit - scuffed and unsure how I could make it accurate
b) I hook an agent/dispatcher into the request that extracts the data for me - not... that bad I guess, it's just ugly and means the user can't use their own agent which is arguably an odd limitation
c) I open a PR that restructures REST a bit. the idea would be to have a method that returns the raw Response object while the existing ones (request/get/put etc call it and return the consumed body)

I'm sorta leaning towards c but it's not the easiest task either and is semver minor

tacit crypt

i mean c sounds the least jank

warped crater

notLikeCat yeah I think so too

its just that i need to modify the API for like 15 classes lmao

tacit crypt

could also just add an option to request method that returns full res

warped crater

isn't that worse

tacit crypt

Because like... we typed it as unknown anyways

warped crater

oh I guess

it doesn't make types that much more complicated

tacit crypt

well its definitely a "dont use unless u know what you're doing"

warped crater

ok yeah that's really clean hahaha

I'll just go for that then

uhhh

I guess I'll make a new PR for that

tacit crypt

cc @ruby terrace

warped crater

and rebase this one to that one

tacit crypt

since he did most of rest actually

warped crater

ckohen my beloved

motivated me to make proxy

ok yes I'll make that PR a thing

warped crater

ily

tacit crypt

but like you need to return the actual response

so people can parse the body

warped crater

yes

yup

should be p easy

uhh

if Im reading this right

this should go to RequestData right

yeah

seems right

tacit crypt

yes

warped crater

oh wait

ugh lol

SequentialRunner#runRequest doesn't.. get passed those options?

ahh

export type HandlerRequestData = Pick<InternalRequest, 'files' | 'body' | 'auth'>;

I see

well my new field needs to be passed on there

~~one more constraint @tacit crypt - added it to the jsdoc

     * <warn>When it comes to non-2xx response codes, this response instance will already be consumed</warn>

shouldn't be that big of a deal yea?~~

oh actually mb, I'm tired

in that case it never returns anyway, it throws, lol.

made no sense

tacit crypt

good morning

warped crater

hahaha

im gonnaaa

add a test for that

or not, I guess, looks like those random options aren't generally tested

wild flax

I’d rather you make a .raw method

warped crater

you.. do?

vlad just said that'd be more scuffed

wild flax

Additionally to request/get/post etc

Nah I think it’d be better

warped crater

just realized that'd be breaking though

I think

wild flax

Why

It’s an additional method

What does it break

tacit crypt
warped crater

well, I guess it depends how we go about it.

if we're looking at REST it's just a new method yeah, but for REST#raw to be able to return a raw response I need to:
a) change IHandler#queueRequest's behavior to return that, and then REST#request calls REST#raw and parses the request - technically speaking this is breaking unless you consider IHandler/SequentialHandler etc to be internal
b) somehow add methods all the way down the chain that instead return raw

tacit crypt

internal behavior is not major

warped crater

but is it internal?

if someone has their own IHandler implementation

you just broke it

because the library will now expect a raw response

rather than whatever parsed data

tacit crypt

its not like we support people supplying their own ihandler impl

warped crater

oh

tacit crypt

lol

warped crater

it seemed like that was the case to me, my apologies

okay then

so it's just a matter of making IHandler#queueRequest return Promise<Response> then

and updating everything up the chain

ruby terrace
warped crater

hahaha

warped crater
ruby terrace

because occasionally you do want to still consume the body (on 429 hits)

warped crater

oh, like, internally?

does undici not have res cloning at all

I was looking into that from my phone when I saw

loud jayBOT
ruby terrace

yeah thats not a problem anymore

lol

warped crater

well they claimed to be able to repro as of the undici commit

which is what weirded me out

and I was like

"...is that actually a clone?"

ruby terrace

because we use request instead of fetch

warped crater

(that's when emitting response)

well, alright but

looking at undici readme, you can only consume the body once

so unless that spread does some sort of magic

the undici PR broke the use case of re-consuming the body in the response event

that's besides the point regardless

429 thing :^)

ruby terrace

because its nearly impossible

warped crater

right! so, why we actually need to consume the body on 429s

isn't the data almost always identical to what you get in the headers

or is there some crazy edge case

tacit crypt

the only way a clone can be done is consuming the body before emitting a clone and then also using it

ruby terrace

I'm thinking you need to consume the body on 429s cause you're not going to send it back upstream at all

not for the data

warped crater

aaaaah, hm

aren't there cases where it would be sent upstream though

ruby terrace

speaking of which....you're gonna want an extra header that allows 429s to pass through (throw on ratelimit stuff)

warped crater

yeah like

by all means d.js will retry on 429

but eventually if it fucks up past the max retry count

it will reject w/ 429 in which case it goes upstream, no?

that should be the correct behavior

ruby terrace

yes, and you need to not consume the body in that case

warped crater

or if that header is provided, yeah

MegaThink perhaps I'm just dense but why™️

ruby terrace

because you need to send it upstream

warped crater

,,,but when would I not send something upstream

that's what's throwing me off in this conversation meguFace

like, I'm always going to consume the body

ruby terrace

In any case when the proxy is going to retry, you aren't

warped crater

right

I see

one more thing

ruby terrace

maybe 429s do pass through always though, that's a valid option

warped crater

am I clear to export parseResponse in REST

if we're gonna be adding a way to get raw responses

might as well also have a way to parse them

currently the only thing exported from the utils is

export { makeURLSearchParams } from './lib/utils/utils';
tacit crypt

sure

loud jayBOT
warped crater

@ruby terrace thoughts on how to handle invalid methods?

technically it seems like /rest doesn't actually check if the method is get/post/put/patch/delete

so I could just bypass the type and let discord throw Method Not Allowed

or should I be sending that myself - in which case it needs to be made clear to the upstream the request never made it to discord

golden mortar
solemn oyster

@warped crater can you capitalise API, please?

warped crater

everywhere?

lol

solemn oyster

I edited the comment a bit late, after @opaque vessel suggested it

warped crater

I feel like the grammar here is weird anyway

Runs a request from the api made fairly little sense to me

solemn oyster

In raw at least

warped crater

alright

opaque vessel

Thank you (':

warped crater

done

warped crater

okay well, at this point #7925 is basically ready for review - it's just waiting for #7929 at which point I'll also rebase from main to clean up the commit history

mostly wondering if there's anything else proxy should provide, or if there's any other tests I should be writing that I didn't think of owoah

tacit crypt

Test: does it work

warped crater

*unit tests meguFace

warped crater
warped crater
ruby terrace

the proxy should be theoretically transparent, so yeah that

warped crater

ty

cursive jackal
wild flax I’d rather you make a `.raw` method

Why not a request option? It can be made using rawResponse option. With the option we will able to get raw response from any request method (.request(), .get(), .post(), etc) and we will be able to set default value for the option in REST options. In my opinion the option is more flexible to use.

wild flax

because it makes more sense from a design standpoint

than to make everything an option

and make types also more complicated

cursive jackal

We can pass the option in the parseResponse function and types won't be so complicated.

ruby terrace

still way more complicated then a separate method. And, if you think about it...it is an option, you just provide that option by changing the function call itself slightly (since the other methods are really just helpers for that anyways). Furthermore, it should certainly not be a global option that can be sent on the instance, that will cause many disasters that are better avoided by not even having the ability to mess it up that way.

warped crater

rejectOnRateLimit is an option on REST itself and that's it

it can't be passed by-request

tacit crypt

also, if you need to use raw you must have some hella specific use case

warped crater
tacit crypt

idgi

warped crater

reading ckohen's message I replied to

I think he wanted a header that when provided told the proxy to reject on 429 w/o retrying

but if I'm looking at REST correctly there's no actual way to do that per-request no?

it's just a global option you pass on instantiation

tacit crypt

Hmm. That ones tough

warped crater

yeah catHmm

is there a specific reason it's not a per-req option as well

tacit crypt

can't we just

make proxy always return 429s

and let the client using the proxy handle the parsing/error throwing?

warped crater

well, the library itself exposes a middleware function that takes in a REST instance that is already instantiated

so by all means, users are welcome to do that and it's easy to accomplish

if we end up exporting a binary/docker image that just starts up a v simple proxy to fulfil most people's needs

we could default to that, yes

tacit crypt

this is more a crawl question but from my pov

my worker rest instance that connects to the proxy has its own rejectOnRatelimit
Proxy always tells me a 429 happened.
My worker instance decides to throw it or not

warped crater

yea that makes sense

you might want to retry on the worker side

we can figure out such default behavior later when we actually make those binaries, but for now

back to the original topic

could REST support per-request rejectOnRatelimit?

i actually feel like this has more diverse usecase

tacit crypt

better question is should it support that

warped crater

well i can think of why it'd be good

the age old problem

discord bumps a ratelimit to hell and we don't know, our bot starts leaking memory because the poor sequential bucket handler gets queued requests faster than it handles them because of the ratelimits

we had this happen with channel names (no, not an obnoxious clock feature)

and we had no clue what was going on

we were queueing, say 3-4 reqs per 10 minutes and the limit is 2/10min

so I could it see it being beneficial to pass in the option for long limits that you know you don't want to wait for

warped crater
solemn oyster

Yes, check the reviews

I bumped those a bit too high, they should be using the same version as 13.7.0 does, which is 0.30.0

warped crater

ah mb

I was looking at it before vlad reviewed that

warped crater

hi so

is there any reason declaration source maps are not enabled?

it would make working with the monorepo a shit load easier since VSCode "Go To Symbol" would actually take me to the definitions

warped crater

hmm, the only thing proxy can't forward rn is headers for non-2xx status codes - DiscordAPIError has most of the relevant info (status text, status message, body etc) but not headers

I was thinking of how this could be accomplished and the only way would be to have REST#raw not throw - but rather return responses even when they're not 2xx. I think this would actually be desirable behavior catHmm? cc @wild flax @tacit crypt

all though that becomes.. stupidly complicated to accomplish meguFace

dawn merlin
warped crater

it works for me in my monorepo 👀

lemme test real quick to see if it works with djs (all though I don't see why it wouldn't), and if so I might PR

while I was working on that

actually confused why that's in the repo - esp. given VSCode gives no easy option to bypass that once it's a workspace setting

well, after some research seems tsup doesn't support declaration source maps properly, so there goes that dream

golden mortar

I added that with the intent to remove it once the pr was ready but then crawl was fine with it staying in

warped crater

Is there anything stopping #7929 (REST#raw) from being merged? just needs space to approve, right?

warped crater

thanks for the merge. #7925 is now ready for review owoah

I've also added a section for current minor caveats that don't seem easily fixable but that we may or may not want to address

I'm also gonna be running some proper tests on it today to make sure it all works

wild flax

I only skimmed the code

Is this like a full binary solution

Or is this just a middleware type of thing

warped crater

unsure how we wanna deal with binaries since djs has no precedent doing that afaik

would it even be in the package folder but ignored from npm publish

wild flax

If it’s a library yeah like a middleware

warped crater

oh hey im green now

wild flax

But idk maybe we should make a separate repo

Where we make it a binary and docker image or smth

Because if it was a ready to deploy solution too, it’d be kinda nice

warped crater

yea that's what I'm thinking too

so there's like

2 exported functions that help you populate a ServerResponse from either an error or a 200 response from discord so you can make your own stuff

and then there's a proxyRequests middleware

that just calls REST#raw in a try..catch

and calls the appropriate method

and ends the response

wild flax

Yeah idk if you want to wrap this around a node http server

Lmk

I can make a repo for this

warped crater

that's probs how the default binary would work no?

express etc seems like too much just for this

wild flax

Yeah

warped crater

yea ok

yea im setting up a little test project locally rn to see if it all works

and once we merge we can get a repo going for common usecases

wild flax

There is something else but idk if it’s in node already

They did rewrite the http with some new thing

Kinda like undici

warped crater

personally what i'll be using for my bot is something that a) logs & collects analytics and b) caches since I like dealing with cache at the proxy level much to your distaste :^)

wild flax

But idk if they merged it into actual node yet

Idk if it should cache things

warped crater

yea def not the default one kek

that's mostly a me thing

I guess the first one will just proxy and nothing else on top

and then people can like

"hey I have my own custom one that does X"

and if X is popular we can

add more, boom

wild flax

sure

warped crater

@wild flax 👀

// proxy.js - this will be our binary minus the worker stuff
import { createServer } from 'node:http';
import { proxyRequests } from '@discordjs/proxy';
import { REST } from '@discordjs/rest';
import { parentPort } from 'node:worker_threads';

const api = new REST({ version: '10', rejectOnRateLimit: () => true }).setToken(process.env.DISCORD_TOKEN);

const server = createServer(proxyRequests(api));
server.listen(parseInt(process.env.PORT, 10), () => {
    parentPort.postMessage('ready');
});

// index.js
import { Worker, SHARE_ENV } from 'node:worker_threads';
import { Client, IntentsBitField } from 'discord.js';
import { ProxyAgent } from 'undici';

const client = new Client({
    intents: [IntentsBitField.Flags.Guilds],
});
client.on('ready', () => {
    console.log('ready');
    process.exit(0);
});

client.rest.setAgent(new ProxyAgent(`http://localhost:${process.env.PORT}`));

const restWorker = new Worker('./proxy.js', { env: SHARE_ENV });
restWorker.once('message', () => {
    console.log('rest worker ready');
    client.login(process.env.DISCORD_TOKEN);
});

➜ node index.js
rest worker ready
ready```

works now

I'll probs test some other stuff here and there but generally speaking at least the gateway connects kek

wild flax

Why does it use workers

monkaHmm

warped crater

idk that was just for my test

I could've just booted it as its own process lol

but I think some people might be inclined to boot it in a worker if they're just using proxy to work around the /gateway/bot ratelimit we were talking about

if their bot is a monolith where they only start one process anyway it's probs easier for them to throw rest into a worker

warped crater

alright, I'm actually done with my PR now unless changes are requested

sleek kelp

Shouldn’t the threadCreate, threadDelete and threadUpdate events be integrated into channel<events>. Then the user can check if the channel is thread and do whatever

I’m not sure if this is already implemented since I don’t follow the GitHub updates that often

copper laurel

We don't change or manipulate events, we follow the Discord API which has them separate

quiet viper

Does not account for emojiCreate and emojiDelete...

copper laurel

Huh, good point, no idea why we do that, though its kinda the opposite example

Thats one event being split out rather than multiple events being merged together

sour ferry

I have nothing to say about it, just look at the pictures.

warped crater

confused

visual hornet
rough roost

pretty sure you are supposed to parse #fff as #ffffff

visual hornet

but that's not exactly a hex string, that's a shorthand hex string. tons of things don't support it.

copper laurel

Yeah, not standard

visual hornet

hmm can't we just validate with /^#?[\da-f]{6}$/i
and if that doesn't match then throw an error

fervent walrus

cant install stuff into main branch. Need to push commits on the pr

[versions]
yarn -> 3.2.0
node -> 16.15.0

!node_modules 🫂

real jetty
warped crater

Mon's message was a general statement

unique axle

i agree, let's stop doing that firFine

ruby terrace
opaque vessel

Still voting for we should leave it as it comes from the API

fervent walrus
copper laurel

Delete node_modules and start again

fervent walrus
copper laurel This isn't a support channel

Yes but I have to commit to my pr and I cannot resolve this issue and it clearly seemed djs issue by reading the errors. This happens only on main branch.
Also there is no node_modules if you see the ss and trying to install node_modules using yarn or yarn install throws the errors from the above ss.

Idk how this aint library related.

coral cave

in the slash command option classes, you all really need to adjust the names of the addChoice and addChoices methods. They're super easy to mistake particularly if your editor has autocomplete, and the error message you get from the validation is incredibly vague - makes debugging it a nightmare

visual hornet

i think that'll be resolved along with the addField/addFields thing for embeds

coral cave

I'm happy to hear that 🙂

visual hornet

there's been a lot of discussion around their signatures and inclusion

unkempt bear

addChoice has already been removed in the main version of the builders. I believe addField will follow suit

coral cave

so in the future you'll have to add all choices as an array of string pairs, right?

unkempt bear

They take a rest param so not an array, but yes it will take APIApplicationCommandOptionChoice params

copper laurel

At the moment the latest version only takes arrays but I think we're adding back support for both