archive-library-discussion

[outer raven]

it's better now

[copper laurel]

And I feel I need to remind you that you aren't using a semver major stable release yet

[outer raven]

but it's still far from perfect

[woeful rain]

I think there are a minimum of people who use @discordjs/builders outside of discord.js

[copper laurel]

If these PRs were 14.0.1, 14.0.2, 14.0.3 because we released something broken, you might have a point

[outer raven]

I know, dw

[copper laurel]

subjective

[dawn merlin]

The point is that they can be used outside of djs.

that also means we can use them in other packages without importing the entirety of djs

[outer raven]

then they should only be used outside of djs, and not inside, because they were not made with that intent and don't have the same features and requirements

[golden mortar]

using them internally is fine imo.. but to re-export them without the api consistency is different

[woeful rain]

👍

[dawn merlin]

They’re raw classes built for extensions there’s no point in use writing code we’ve already wrote

[copper laurel]

I'm really starting to lose understanding in these arguments

[copper laurel]

But djs extends them with additional functionality to work inside, without breaking the standalone package. Problem solved, good design

[copper laurel]

They're re-exported in djs versions, if you dont want that version you use the standalone one

[outer raven]

problem solved for the user, but you lost the point of easing maintenance overload which was the initial reason to make the switch as far as I'm aware

[copper laurel]

No we dont

[dawn merlin]

In object-oriented programming, the open–closed principle states "software entities should be open for extension, but closed for modification"; that is, such an entity can allow its behaviour to be extended without modifying its source code

[outer raven]

you don't if that new PR gets merged, which adds 2 more deps

[golden mortar]

actually builders work internally without the additions, the additions are for the user only

right now the only addition is to accept camel case which djs doesn't use

[copper laurel]

Well yes, but to work "inside djs" should also mean working for the user via a consistent interface

Something Rodry was particularly vocal about lol (and I agree)

[copper laurel]

Nothing wrong with dependencies

[golden mortar]

but its not consistent fully - djs accepts both camel case & snake case but builders only accepts snake

[copper laurel]

snake

builders are snake

[golden mortar]

yes my bad

[copper laurel]

Guess what

They always did that

MessageEmbed always accepted both in its constructor

[outer raven]

they're unnecessary and only used in 1 place, I'd be fine with deps if they actually served a purpose in helping things across the library, but users usually like tend to like a package better the less deps it has

[wild flax]

They do serve a purpose

[golden mortar]

MessageEmbed was also used internally when an embed was received from the api

however it won't ever receive camelcase

[wild flax]

Users who think less deps = better are not our concern

Never were

The argument is silly

[copper laurel]

Yes it does, when a user constructs one

[golden mortar]

I was mostly referring to the api

[dawn merlin]

The library is an interface between the user and the api you can’t ignore the user part

[copper laurel]

You can when your point would be entirely wrong if you didnt though, apparently

[outer raven]

why? More deps means more things to fetch when installing them which means longer install times as well as a bigger size overall when compared to the alternative that could be integrated within djs (and is rn in main). Not to mention the simplicity of the code which is much higher in the current version than the proposed version in the PR with the new deps

[dawn merlin]

Dude

It’s 16kb

[golden mortar]

I thought it was pretty clear I was referring to the api only 🤷‍♂️

[copper laurel]

Which is only half the picture

[outer raven]

and it would be 1 or less otherwise. These ones in particular might not be an issue but if you keep this mentality and keep adding new deps they will start to build up in size

[golden mortar]

not when builders are installed with discord.js and can be used by themselves

[copper laurel]

can be yes

[wild flax]

If you really are concerned about size, add tsup to transpile the modules we use, tree-shake them, generate a source map and have djs be 1 file

[woeful rain]

Wouldn't it be preferable to add camelCase support to builders and then there would be no need to extend these classes in the lib itself?

And... https://github.com/discordjs/discord.js/blob/main/packages/discord.js/src/util/Embeds.js#L65 there is test instead of text
^ fixed by https://github.com/discordjs/discord.js/pull/7464

[copper laurel]

No, because builders are not camelCase, as a standalone item they mirror the API conventions
Within discord.js however, the extension adds an interface consistent with the rest of the library

[dawn merlin]

I want to reiterate builders is open for extension. That means providing the minimum viable idea of what a builder is in its source package.

[copper laurel]

If all you need to do to add compatibility is wrap the constructor, I really dont see how that's an issue

That seems to be the overarching issue here - people with a different opinion on what a builder is than what it actually is

[dawn merlin]

If you build logic into a base class that logic is hard to take out when extending

[copper laurel]

I personally don't like them but I still don't see the arguments made as valid ones

[dawn merlin]

I also never use builders i always use json.

But I can certainly understand why it’s setup a certain way

[golden mortar]

same but once v14 is released any change will be semver major so might as well argue about it now

[copper laurel]

But it isnt released

And isn't going to change

[visual hornet]

has duplicate properties with setters been discussed

[golden mortar]

I'm not expecting it to as that isn't how you guys do it from what i've seen

[dawn merlin]

It would be inconsistent to say the least that we’d be moving away from modular packages.

[solemn oyster]

Why are you guys so set on thinking we'll use Zod forever? lol

[golden mortar]

I'm not, but until the change is made it's still a valid point

[dawn merlin]

Aren’t you using unsafe builders anyways?

[golden mortar]

I'm using json

[dawn merlin]

But you were the one who pushed for unvalidated builders, right?

[golden mortar]

yeah

I saw something that would hurt dev ex and hopefully pushed for a better alternative 🙂

[solemn oyster]

What's this, 1984? I have a project that needs to fetch a grand total of 34000 dependencies (thanks front-end!) and barely takes any time thanks to caching in yarn v3

[outer raven]

I also have a project like that probably with less deps that takes about 4m to build from the ground up

[solemn oyster]

More deps also means less code that needs to be built in the project, which means builds are faster

[woeful rain]

And what will you use? shapeshift, myzod or nothing?

[solemn oyster]

Shapeshift

[dawn merlin]

Well yeah but at the time you I brought the alternative of json for embeds and you stated you were already used to using embed builders. Idk ig I assumed wrong.

[outer raven]

djs isnt TS yet but even when the change happens, builds on the library's side will always be faster than dependency fetching, you can't really compare the two

[solemn oyster]

Without the outliers, Shapeshift is 3rd fastest, and the util.inspect overrides makes errors very clean and very easy to read, so no more "wtf is a ZodError"

[golden mortar]

I was at the time, but once validation was added I moved from them. This is a bot from august 2020 so it definitely had some old odds and ends in it

[dawn merlin]

How often are you fetching dependencies rodry

[solemn oyster]

Furthermore stack traces are also a lot smaller since Shapeshift uses iterations over recursion

[dawn merlin]

Just out of curiosity why didn’t you move to unsafe builders

[outer raven]

on my bot? Every time I push to update it on my host, which already takes 5 minutes and is close to the size limit (for other reasons of course, but I still wanna avoid making that worse)

[golden mortar]

I moved before they were a thing

I had one place where Embeds would be built and after that I just needed time to convert the rest

[dawn merlin]

would you have used unsafe builders if they were available at the time?

[golden mortar]

I don't know tbh. Probably until I could convert everything to json

[dawn merlin]

Don’t you have puppeteer installed on your bot?

Like that’s pretty big, that’s more of the issue than djs

[outer raven]

I do yeah, that's the main issue, but still the second point stands, I don't want it to get worse
also how did you know 💀

[golden mortar]

eslint & typescript eslint is about 75% of my dependencies

[woeful rain]

Or prisma

[dawn merlin]

Eslint is a dev dep anyways, but then again this is server side software, not a website

You don’t have to worry about page load times and things of that nature

[solemn oyster]

And eslint installs lodash anyways

[dawn merlin]

Kinda the reason people are moving to SSR so they don’t have to worry about bundle size

[solemn oyster]

In fact there are so many packages using lodash, and the packages haven't seen a release in so long, chances are very high your package manager already has them in the cache

If you use yarn, which installs packages correctly, it's basically zero overhead in most cases

[dawn merlin]

Even if they didn’t, 16kb like come on

[solemn oyster]

Imagine complaining about 16kb, but not complaining about Chrome using half a GB of disk space

[outer raven]

in my case chrome is essential for a big part of my bot, and like I've explained previously, the issue isn't in the 16kb alone, it's in the "dependencies are fine" mentality that adds up space over time when it can and imo should be avoided most of the time

[dawn merlin]

I mean I don’t think anyone feels like writing another case-converter when one already exists and is battle tested

[outer raven]

your "case converter" is just an object with properties in different casing

[dawn merlin]

The art has already been perfected by people who aren’t us

[solemn oyster]

Rodry, one thing

You know what's one flaw of the current converters? Like the object mappers we have rn in main, before the lodash.snakecase PR

[dawn merlin]

I do

[outer raven]

I don't, what is it?

[solemn oyster]

They use a lot more memory

Because all properties are defined even if you pass a partial set of them

[outer raven]

more memory than a function as complex as lodash's?

[solemn oyster]

They're also incredibly deoptimised because they're megamorphic, unlike those transform functions

[dawn merlin]

Oh I was thinking because they can cause a bunch of unintended overwrites when spreading

[solemn oyster]

Right now, Embeds after getters use less memory than v13's MessageEmbed

But if we mapped every single object in Embed's constructor just like we do in djs main, they would use a lot more memory

Simply because it's an object with all the properties

[outer raven]

but when you pass them to the builders class all properties get created either way

[solemn oyster]

Whereas the transformer respects the input's partiality, and doesn't transform what isn't passed

They aren't

Not after the getters PR

[dawn merlin]

Anyways what changes do people want to see out of builders? Asking for it to behave like the older builders wasn’t ever out of the picture, it’s just been an iterative process

[outer raven]

Embed#addField to not take an object

also speaking of the getters PR, surely this isn't correct, right?

[copper laurel]

Embed#addField to not exist at all when we have Embed#addFields

[outer raven]

sometimes you only need one

[copper laurel]

Which is entirely supported by addFields

[dawn merlin]

It is but it can be done better

[outer raven]

in an object form

[copper laurel]

Theres no >=2 constraint on rest params

[outer raven]

but builders doesn't support camelCase ?

[dawn merlin]

It’s getter does

It’s a js class

[outer raven]

but it's much nicer to be able to do .addField(name, value, [inline])

[copper laurel]

I disagree, I like objects

It names your params, drops the need for a specific order

[outer raven]

huh? I'm so confused now
wasn't the whole point to be just snake_case? Why do the classes exist on djs now

[outer raven]

the specific order makes sense here because name comes first, then value, both required, then inline, optional

[dawn merlin]

The classes just override the constructor to accept camelcase

[copper laurel]

Never said it didnt make sense, but I like objects anyway

What doesnt make sense is having two methods where one just calls the other one anyway

[dawn merlin]

Yeah I’ve personally found objects to be better here

[outer raven]

it does, for ease of code

[copper laurel]

Its easier to know one method adds fields to an embed

[dawn merlin]

Objects are more portable than parameters anyways

[copper laurel]

And not have to switch between two fundamentally identical methods with different interfaces

[woeful rain]

Sometimes it would be a good idea to make a global vote and not just make a decision because you like it.

I mean that in a general way, because users and their opinions are also important.

[outer raven]

[dawn merlin]

That can be used as input but it wouldn’t be the final say

Also things like that neglect the maintainers pov

[copper laurel]

Theres a team of core maintainers, there's a large team of contributors, its one of the best open source projects I've seen, and these channels exist because discussion is welcomed

But its not a democracy

You can discuss, the core maintainers dont have to agree or adopt suggestions if they dont want to

[outer raven]

it's not the best open source project if outside suggestions are alienated by core maintainers almost constantly

[woeful rain]

It's important to listen users too (you can't always do that), but with the addField thing, for example, I think it's perfectly fine as Rodry said.

Certainly, as I have already said, you cannot take these votes everywhere, but you also need to take users opinions and ask them.

[copper laurel]

Sorry you feel that way

I think we have a very vocal minority going against certain changes, the majority of them are very well received

[dawn merlin]

It is open source because you can fork it if you feel otherwise

[outer raven]

I've seen countless amounts of people saying they're scared of submitting feedback to discord.js for that same reason, so maybe sometimes that "vocal minority" might not be as much of a minority in reality

[outer raven]

observations like these prove my point

[dawn merlin]

I totally agree, I’m open to opinions. I just want to make to it’s a change that works for more than one person

[copper laurel]

I could make exactly the same claims in reverse though

Every time we get a complaint about something we changed people refuse to listen to the reasons for doing do, be it performance, consistency, easy of maintenance, whatever the reason might be

Theyre just mad that it changed

[dawn merlin]

I didn’t mean that as a way of dismissing you I’m just saying the essence of open source is that’s it’s not exclusively owned

[copper laurel]

If it were up to users we'd probably still be on v8 syntax

Because any major change would be voted against

[woeful rain]

I understand that, that's why I said you can't always do a global vote.

[outer raven]

some do, that's true and those don't really have a point most of the time. I think that because of those users, usually the first response to any suggestion is very hostile and poorly thought, because they assume it's gonna be one of those users and that it's not worth even thinking about it. The whole energy around PR reviews also isn't the best when compared to other repos I've seen (smaller than discord.js, I'll give you that), but there's a lot of room for improvement if they wanted to

[copper laurel]

What "energy" is that? Its the same as here, differing opinions. There's a lot of poor quality issues/comments/reviews too though

[wild flax]

Never heard of something like public votes on something in the OSS space

Yeah sure, someone prs something

And there’s a discussion why they think it should be that way, but votes..? 🤪

[outer raven]

not votes, but maybe PRs like this shouldn't be merged

[dawn merlin]

which PR is that

[wild flax]

I mean I don’t wanna be super political, but you know that most topics lately in politics go the same trend

But still get applied and go through

Case in point if we only listened to users we wouldn’t be able to move forward at all

[outer raven]

Embed builders switch

[dawn merlin]

it's kinda inconsistent tho, bc my components PR only got one thumbs down

[outer raven]

the only difference is that in politics, people are elected. I think you would be able to move forward even if PRs have a high dislike count as long as they also have a high like count but I mean... that one is 15 to 1, it barely has any likes at all

[outer raven]

yours was already halfway through the process so it's understandable and Embeds are more widely used too

[dawn merlin]

Also keep in mind this is before unsafe builders were ever considered

[wild flax]

Yeah but tbh it’s as rodry said, this isn’t exactly a democracy

We have our own design decisions that we take and move forward with

And sadly 99% of our user base doesn’t understand half of the things we do

[outer raven]

it doesn't have to be as linear as "if it has more dislikes than likes then close", but for that specific scenario, maybe it should've been reconsidered or more opinions should've been heard

[outer raven]

you shouldn't assume your user base is stupid, even if some seem to be

[wild flax]

They only build bots and barely succeed in doing so, I don’t heavily trust the opinion of a beginner to make a good design decision for the long run

[outer raven]

many of the dislikes on that PR aren't from beginners

[wild flax]

This doesn’t have anything to do with stupidity but inexperience

[outer raven]

I see what you mean but then again, only 1 person liked it

there's both sides of the same coin

[wild flax]

It’s not like everyone uses that feature

Do you see any of us maintainers likes on it? You could easily add another 7 likes then

[outer raven]

I do sometimes yeah

[wild flax]

Then it’s 8:15, much more closer

[outer raven]

but you're also slightly biased, it's good to hear external opinions

[wild flax]

And we can disagree with those

Some of those arguments sound like we just do whatever lol

[copper laurel]

As much as I do tend to argue in this channel, I hugely prefer this discussion where people often make a case for/against a change rather than just object to it because they don't want to have to rewrite code

Or change their code style slightly

Which is how a lot of people react by 👎 on a breaking PR

[wild flax]

Discussions like this just further deter me from engaging in others because it’s always the same outcome anyway because of the above mentioned

You can explain as much as you want, all the good reasons, make sure you get to 1:1 parity as much as you can and still get some dumb feedback because now it’s suddenly “unnecessary” and “hacky” and “a maintenance mess”, yet we don’t think it is any of this at all lol, and for us it should probably matter the most if something is harder to maintain than before

[outer raven]

if you make an argument and the other side ignores it and continues saying stuff you've already explained it's fine to assume they just don't like it because it's breaking but not every breaking change gets negative feedback, so all I'm saying is that sometimes it's worth looking slightly more into the cause of the outrage

[tacit crypt]

our past record of breaking changes begs to differ with that

[wild flax]

Yeah I don’t understand the current outrage sorry

As much as I try to read into it

The embeds will have pretty much 1:1 parity with a single place to be maintained

[oak quail]

is it known/intended that rest stable docs dont work? (failed to fetch docs file from github)

and https://discord.js.org/#/docs/rest/main/typedef/RESTOptions does not have the defaults

[opaque vessel]

REST doesn't have a stable release, so yeah, intended

[oak quail]

https://github.com/discordjs/discord.js/releases/tag/%40discordjs%2Frest%400.3.0 is not marked as a prerelease

[oak quail]

looks like https://discord.js.org/#/docs/rest/@discordjs%2Frest@0.3.0/general/welcome also doesnt work

so there seems to be no way to view the docs for the release version

and ive already encountered a situation where the release version differs from main

[ruby terrace]

you mean files :)

but yeah, 0.3.0 was released before monorepo IIRC, so the docs build for it isn't in the right place

[mighty sequoia]

interaction?.message?.interaction?.commandName on SelectMenuInteraction as interaction works but commandName isn't documented

[visual hornet]

i mean why wouldn't it work? it's not going to error, just return undefined or some other value; what exactly is the issue, what's interaction, what does commandName give

[mighty sequoia]

the commandName for the interaction

theres no commandName on the SelectMenuInteraction object

[visual hornet]

and what's interaction? an instance of SelectMenuInteraction? in that case that should be normal

[mighty sequoia]

yeah

[visual hornet]

interaction !== interaction.message.interaction

[mighty sequoia]

I don't understand why thats normal

Yeah. I'm aware

hence why I have to use interaction.message.interaction to dig deeper for the commandName

[visual hornet]

the interaction and the interaction of the message aren't the same, commandName is well documented https://discord.js.org/#/docs/main/stable/typedef/MessageInteraction

[visual hornet]

well, you're going away from whatever problem you may have had then

[mighty sequoia]

how?

look I'm not going to debate semantics back and forth, I'm just reporting a problem

[visual hornet]

interaction has little to no influence on interaction.message.interaction

if this is what you had to "dig deeper", what was the original issue

[copper laurel]

Yeah hold up, lets not argue this one

I think the only issue here is that MessageInteraction#commandName should be nullable

right?

Because it wont be defined unless the interactionType has a name

[copper laurel]

Nah Im confused sorry, I dont understand what the issue is

[mighty sequoia]

https://i.imgur.com/1O1J3X1.png

should have commandName in the dropdown

I went into debugger and it is there

[copper laurel]

What's the type of message?.interaction here though?

[copper laurel]

I dont see APIMessageInteraction anywhere in the codebase

[mighty sequoia]

I'm on ^13.6.0

and I installed a few hours ago

[copper laurel]

Okay, yeah. That's because it's not a property of both types, so Intellisense wont display it

If you assert the SelectMenuInteraction as being from a cached guild, either by casting as SelectMenuInteraction<"cached"> or typeguarding using interaction.inCachedGuild() it should be detected

[ornate topaz]

PSA:
Intellisense is not documentation. It displays most of the stuff found there, but don't forget that it isn't a full index of everything, but stuff that fits your current variable, especially in TypeScript.

[outer raven]

Don't know if you're aware yet but the most recent dev release is deprecated

[outer raven]

@tacit crypt since you approved, what's your opinion on this suggestion? https://github.com/discordjs/discord.js/pull/7320#discussion_r790155072

[tacit crypt]

The move channels one? Its very..wack, setting positions has always been an annoying thing... I can sorta see the benefit for a shortcut but personally I'm neutral. Idk what the others think tho

[opaque vessel]

Feels like you could create a base guild channel manager with those methods

Put set positions and fetch active threads on it

Have the guild channel manager and category children one extend it

But I don't really like where that is going so

[outer raven]

The reason I didn't add it straight away is because it serves no purpose really and it can lead to misleading behavior since you could move a channel that isnt in the category to it

and I don't think that's the point of that manager, it's to manage channels inside that category only

[outer raven]

that wouldn't work because the methods aren't the same

wouldn't help much

[opaque vessel]

If the methods aren't the same then don't do it at all

[outer raven]

don't do what

[solemn oyster]

I'm personally against it as well, that method isn't even aware of its children

Wrong channel, @indigo bison.

[indigo bison]

whats the right channel?

[solemn oyster]

None, I don't think we allow those things due to some issues in the past

[outer raven]

yeah alright, will resolve then

[opaque vessel]

Seems the stuff in that pr is documented as GuildChannel, should there be a new JSDoc for that without category channels?

For example, the create method in that new manager @returns {Promise<GuildChannel>} which includes a category channel, but that's not possible

[dry totem]

I don't really understand how discordjs work

I wanna edit this event https://github.com/discordjs/discord.js/blob/main/packages/discord.js/src/client/actions/GuildScheduledEventDelete.js

But it don't show me when it is triggered

[solemn oyster]

What do you want to edit from it?

[dry totem]

The problem with this event is that it only fire when we delete the event before it start

But it is not triggered when we press the button "Finish Event"

When it has been started

[opaque vessel]

That's as-is from Discord, it's not a problem with us

[solemn oyster]

You can verify your findings by using a debugger on line 8 of that action, for example in VSCode: https://code.visualstudio.com/docs/editor/debugging. With this, you can check if we're omitting messages or some sort due to a check failing

[outer raven]

When you do that you’re not deleting the event, you’re finishing it

[dry totem]

Is there any event for that ?

[solemn oyster]

GuildScheduledEventUpdate (aka the same, but Update instead of Delete)

[outer raven]

Ehh I could, is it worth it tho?

What would we call it

[opaque vessel]

NonCategoryGuildBasedChannel, just like the types shrugs

I think it's called that

[solemn oyster]

Do both classes take the same type arguments, @outer raven?

I assume they do, but I want to make sure

Well, I guess CI will tell us

[outer raven]

they do, I tested it

also don't ask me why it says app, I typed apply suggestions from code review lol

[outer raven]

that would require a class too

because the holds of that manager is GuildChannel which is also technically incorrect

if you wanna be 100% accurate that is

[solemn oyster]

@outer raven definitely not doing a Kyra LGTM moment but... line 7 in the manager isn't right, it doesn't store their cache

[outer raven]

Should I say holds instead?

[solemn oyster]

computes?

Or just don't mention it at all

I think that entire thing needs rewording if we want to mention both things

[outer raven]

I just copied the style from other managers

[solemn oyster]

Because by itself, it doesn't compute the cache when the class is created, only when the getter is called

[outer raven]

Nitpicking at it’s finest

Ill just remove the cache part

Done

[outer raven]

Could the test folders be renamed to tests? Having the tests show up at the very top of a PR isn’t very useful IMO, ideally you’d wanna see the changes first

[wild flax]

If tests are being changed it should be reviewed too

It doesn’t matter

And tests and mocks are a sort-of standard folder name for those

Thanks discord

[outer raven]

Not sure what standard that is because I’ve never seen any library name them with underscores. Also I didn’t suggest removing the folder, just renaming…?

I’m saying you can’t possibly review a test if you don’t know what’s changed

[wild flax]

Seen them plenty out there

[outer raven]

Probably not in good libraries then because this way you have to keep going back and forth in the code to understand the tests

[solemn oyster]

That's because it's Jest's default, lmao

It's a weird Python-like convention, I have no idea why it's adopted in the first place

[wintry pendant]

I agree, there is no point looking at the tests until you know what actually warranted the change

[outer raven]

can't commit because of these errors even though I didn't touch those files, is there an known issue about this?

[dawn merlin]

move into the root package and run yarn build then yarn install and see if that works

[outer raven]

apparently just running yarn fixes it but I've done this 5 times today without deleting node_modules once so idk why this keeps happening

[outer raven]

yarn v3's link command seems to be very complicated and isn't working for me properly and I also cannot find any documentation online. Could you guys make some sort of document explaining how to test a PR locally now? I used to use yarn v1's link just fine but now I'm stuck

[copper laurel]

npm link works for me

[outer raven]

but it doesn't work with a yarn project does it

[wild flax]

If you checkout a pr you need to make sure to run yarn install again or yarn build additionally

Because links or deps could have changed

[outer raven]

I did, my issue is in the link command

I have no clue how it works and everything I try errors

[wild flax]

Just link npm link usually

[outer raven]

tried now and its saying the deps aren't installed even though they are

[wild flax]

You just run link in the first directory you want to link. And then again in the one you want to link it to

[outer raven]

if I do that it tells me this

that was the v1 syntax

[wild flax]

You could probably also just try npm link

You wouldn’t have to use yarn to link I guess

[outer raven]

I did ^^

[wild flax]

From yarns documentation it’s just link + destination module/folder

[outer raven]

do I run that on my project with the destination being the path to my djs clone?

[wild flax]

Yes

[outer raven]

the issue is that I'm using yarn v1 globally and the only project that uses v3 is discord.js therefore I can't link that way

[wild flax]

That is indeed a problem

[outer raven]

and I don't plan on switching because the new version doesn't seem any better to me than v1

would it be impossible in that case?

[wild flax]

Could be

But it is better

Just use the node-linker mode instead of pnp

Now it’s basically yarn v1 but faster and more competent

[outer raven]

what's that?

[copper laurel]

Yeah it did when I was testing v14

[outer raven]

well it didn't for me cuz all dependencies appear as if they're not installed

[copper laurel]

Sorry for a dumb question but you are linking from the package dir and not the root right?

[outer raven]

I am yeah

[wild flax]

For npm link you go into the discord.js sub package

Run npm link

And then your project

Run the same command

[outer raven]

the typings file is correct but full of errors

[manic cairn]

same tho, yarn 2/3 utterly sucks i wasted half a day trying to link djs dev branch, and it would link ok but every yarn install/add it would try to install djs deps to the remote folder's nested node_modules (despite --flat??) and then stop itself and fail
and npm module resolution chokes on those workspace: protocol deps. just an unfun mess. totally solvable but ugh.

[copper laurel]

The contributing guide says to run yarn --immutable which hasnt failed me and I have no idea how to use yarn, I always use npm

Im literally just following one instruction

[wild flax]

@outer raven if you feel like it, you can take over https://github.com/discordjs/discord.js/pull/7448

since you moved it already

[outer raven]

Do you wanna go forward with that though? I don’t really see a point since the name is always required when creating a channel and nothing else

[wild flax]

Yeah probably if I'm honest

Theres rarely a case where you want a channel and no other options to it

[outer raven]

True but usually the options object in djs is for optional stuff except some rare cases

[wild flax]

this would be one

[outer raven]

I could take a look at other methods like that and move all of them in one PR then

[analog oyster]

I understand moving the name into the options object, but not reason, because the name pertains to the channel/whatever while the reason will only be used for audit logs and is not related to the channel/whatever options

(Look at the edit() methods in that pr)

[wild flax]

No, I think thats ok

[analog oyster]

Now that I'm looking at it there are other methods that are already doing that, so I guess for consistency reasons the other ones should be changed too

[opaque vessel]

I'd vote for the others should be changed for consistency too

[outer raven]

Psst @solemn oyster webhook handlers?

[outer raven]

Changed to which?

[opaque vessel]

Including reason in the JSON params

[outer raven]

Ah yeah alright

[solemn oyster]

Yes, what about it?

[outer raven]

Isn’t it websocket?

[solemn oyster]

There, fixed

Both start with "web" so I was mixing the two

[outer raven]

Lmao yeah I figured

Happens to the best of us

[solemn oyster]

We talk about webhooks more than websockets tho

[outer raven]

True lol

[outer raven]

@slate nacelle since you commented on https://github.com/discordjs/discord.js/pull/7472#issuecomment-1041330271, do you know how to fix the error? For some reason, PartialGroupDMChannel doesn't have partial set to true so I can't replicate the test. Could the type of partial be changed to this is PartialDMChannel | PartialGroupDMChannel?

also for some reason Channel#partial is typed as strictly false when it can be any boolean

[zenith oracle]

it is false because partials structures have their own type, with partial strictly true

[slate nacelle]

Isn't that test to ensure that the partial dm channel is included there?
I'm not really following what's going on in the PR and said test though.

[outer raven]

I thought that AnyChannel was supposed to be all the channel types so that it can then be extracted from to create other types which is why I made the PR but that test specifically is checking for the type of lastMessageId in PartialDMChannel which isnt the same on PartialGroupDMChannel

[slate nacelle]

PartialGroupDMChannel has no text capabilities.

The issue(?) there is that TextBasedChannel can't be a PartialDMChannel when not included.

[outer raven]

So should I keep that interface and just add groupdm?

[slate nacelle]

I think so

[outer raven]

Alright, will do

[outer raven]

Since more PRs keep appearing to v13 are we allowed to make PRs to backport semver minor changes before the next v13 release?

[wild flax]

Sure

[outer raven]

oh great, that's nice to know

to the v13 branch, not stable right?

[wild flax]

Yes

[outer raven]

alright

[opaque vessel]

I had these on a list that should probably be backported

• https://github.com/discordjs/discord.js/pull/7386
• https://github.com/discordjs/discord.js/pull/7294

[outer raven]

I believe I saw a few others too, I'll take a look

[charred mural]

Is it normal that Message.crosspost() RateLimitData (when using rejectOnRateLimit) returns 10500ms timeout instead of actual timeout (which is up to 1h)?

[copper laurel]

Is it actually one hour though? Or is 10500 accurate

[charred mural]

When you normally hit rate limit and click Publish on Discord manually, it tells you for how long you can't publish (1h if immediate), RateLimitData will return 10500 regardless, I don't know where it gets that number from

[copper laurel]

It should be from the headers

[charred mural]

I tried manually sending crosspost request and headers return actual timeout in data.retry_after

Though it's in seconds there for some reason instead of miliseconds

Oddly enough, when crosspost request is not rejected (but queued instead), it automatically publishes that message after the correct period has passed

[copper laurel]

The correct period being one hour?

[charred mural]

It's 1h since last successful publish (since you can publish 10 max per hour per channel)

So if you try to publish another message 30 mins after the rate limit on that channel is already triggered, it will delay that one for 30 mins

[copper laurel]

Thats not how a rate limit should work to my knowledge

10 per hour, but if the last one is at 59 minutes you shouldnt have to wait a full hour for the next publish

[charred mural]

I mean, I meant that above, just used 30 mins as an example

It won't wait 1h, but 30 mins in my example

Anyhow, I still don't get it where it gets 10500ms in RateLimitData from

[wild flax]

I could lie but

There might be 2 limits in place

1/10s and then 10/1h

[charred mural]

It's possible, they definitely changed the timeouts with v9 because with previous API versions it returned retry_after in ms, with v9 it returns in seconds instead

Could be they changed more stuff with that

[wild flax]

10500s is also like 3h or smth

So not sure what’s up with that

[charred mural]

It's 10500ms, not s

So 10,5s

[wild flax]

Yeah just thinking out loud

[charred mural]

It even surprised me why they would return retry_after in seconds all of sudden (when sending crosspost request manually)

When I manually send the request, 2021.647s is about 33 mins

timeout in RateLimitData (in ms)

Manual request would be correct as that's also what client would show me

[void rivet]

Okay i too am confused now
How come MessageAttachment is still MessageAttachment and not just Attachment?

Like the docs don't refer to it as message attachment
And with attachment options
I'd argue that's not really attached to a Message

[dawn merlin]

Good point

It can probably be renamed, and additionally converted to a builder

[void rivet]

Yeah now it makes a lot of sense to add to builders

[golden mortar]

or it could just be left alone

[dawn merlin]

But they can be used with slash command builders if they’re in the builders package

[golden mortar]

you can add to builders without removing from djs

[void rivet]

It goes out the pattern too, like this is probably the only case where the Message prefix isn't being dropped right?

[golden mortar]

then you don't need to add UnsafeAttachment or whatever as well

[void rivet]

We've done it already with the others
Why not with this? Why exactly should we leave this one alone?

[dawn merlin]

Then we’re maintaining two classes with duplicated code

[golden mortar]

you have to maintain 2 classes regardless and i don't think attachment needs much maintenance

also would builders' attachment support Buffer/node streams?

when I made the undici pr the data resolver stuff was some of the most annoying to deal with lol

[dawn merlin]

an extension of a class with most of its implementation already defined is much different than maintaining 2 of mostly the same class from the ground up

[golden mortar]

I can't check earlier commits but since the monorepo change 1 change has been made to MessageAttachment which is a jsdoc change

it's not really a structure that constantly changes

idk why im arguing because the unsafeattachment would be good anyways

im just wondering if it would support djs features

[dawn merlin]

That’s beyond the point we want to be able to use it multiple packages without duplicated already written code. For example an HTTP interactions library.

[dawn merlin]

I’ll have to investigate but if it can it should

[golden mortar]

also please support Blob and other new things please 🤞

[dawn merlin]

Is blob in node now?

[golden mortar]

const { Blob } = require('buffer')

[dawn merlin]

Ah ok that must be new bc I swear that wasn’t present before

[golden mortar]

since v15, backported to 14.18

it's very useful when using fetch, ie ```js
const r = await fetch('something');
const blob = await r.blob();

new MessageAttachment(blob, 'name.something')

[dawn merlin]

Well yeah considering that standard fetch uses a blob

Will the undici merge add Formdata to the node scope?

[golden mortar]

eventually but not in v17.xx afaik

it's not in core yet

https://github.com/nodejs/node/pull/41956

[dawn merlin]

Epic

[golden mortar]

it might be worth supporting blob like classes too, if someone is using node-fetch for example

${blob} === '[object Blob]' should be good enough to check if a class is a Blob

[dawn merlin]

We’re still using node-fetch so

[golden mortar]

ah right, mostly talking about v3 since they use a dependency for blobs

idk what v2 does, I assume Response.blob doesn't exist

[wild flax]

I think it does

[golden mortar]

oh cool, they have their own blob implementation

[dawn merlin]

So does that mean we can use node Blob without worry?

Well for our use cases at least

[golden mortar]

yes - I implemented in my original undici pr actually

[dawn merlin]

I also took a jab at mocking with undici, and oh boy it’s a mess

[golden mortar]

I was hoping you made a little more progress than I did, but it definitely doesn't have the best api available

[dawn merlin]

The ideal situation would be for nock to abstract their primitive interceptors to support both http stacks

But that’s a large refactor for them

[golden mortar]

I'm not holding my breath for nock support tbh. I think over time (and maybe some more interest?) the undici mocking will get better, but it isn't there quite yet

also with my latest pr the timing test in /rest failed again

[dawn merlin]

It’s a flaky test if it happens I usually push an empty commit

[opaque vessel]

Can't you re-run the action

[golden mortar]

I forgot I could push an empty commit

maybe it's better to remove those tests?

[dawn merlin]

Only admins on gh can

Iirc

[copper laurel]

You can push an empty commit to trigger it again

[ruby terrace]

it'll be flagged in v16 as far as I'm aware

[ruby terrace]

we aren't using performance on all the tests yet, maybe we should fix that

[ruby terrace]

Took a quick look at this, we don't ever send the retry-after timeout in the rate limit data for some reason
Looks like this is being treated as a sublimit (which it kinda is I guess) and Discord returns normal ratelimit headers for the normal limit + retry after. Explains the weird queueing working properly behavior you were having too.

[analog oyster]

the test that failed on khafra's pr is already using performance

[golden mortar]

i believe a pr was made to use performance on those tests as they were flaky which is why i brought it up again

[dawn merlin]

Also @golden mortar if you want you can try to take a stab at refactoring the message attachment builder

[golden mortar]

I don't have much free time until the weekend but I'll look into doing so 👍

[golden mortar]

@dawn merlin how should I handle files uploaded by people? should I just add a property to the class?

[dawn merlin]

wdym uploaded like local file paths?

[golden mortar]

if someone has a Buffer/etc they want to upload

as it's not part of the attachment object but djs combines both functionalities

[dawn merlin]

oh, well it's multipart form data I think it should ideally toJSON would generate that multiform data so the actual data should be in the class. Or at least have a reference to the data, and resolve its buffer in toJSON

[honest barn]

since #7490 (raw message data) is a thing, could the message constructor be changed to public?

it can be really useful for extending the message class in a non library breaking way, and as easy to do as just new ExtendedMessage(client, message.data)

[dawn merlin]

I don't see any issues with that 🤷 but idk what the others think

[golden mortar]

I don't know if this would work as Attachment also needs to send the actual attachment's data in toJSON, unless you mean to combine both of them? I'll see what I can think of as I work on it

[dawn merlin]

If you look at MessagePayload#resolveFile you can see how it resolves the payload

[golden mortar]

I plan on returning a blob when resolved so it's spec compliant (formdata only accepts blobs and strings)

[dawn merlin]

ah

[golden mortar]

idk how node-fetch's form-data works tbh

[dawn merlin]

so more of toData rather than toJSON?

[golden mortar]

that could work, yeah

toJSON for the APIAttachment, toData for the attachment data

[dawn merlin]

read my mind

[golden mortar]

does anyone know the max description + filename length for attachments?

[analog oyster]

filename is 1024

description is also 1024

[golden mortar]

thanks 🙏

[golden mortar]

using UnsafeEmbed or Embed on latest dev doesn't work, seems as toJSON is never called on the embed class

doesn't happen on 14.0.0-dev.1645056357.dee27db, just latest

[dawn merlin]

like it's never called by djs?

or calling toJSON doesn't do anything?

I think I see the issue with unsafe embeds but regular embeds should work

[golden mortar]

neither worked for me, I didn't play around with it much

it was on djs' side

maybe I didn't use Embed? I don't know

[dawn merlin]

Well the error is that it's checking instanceof Embed but UnsafeEmbed doesn't satisfy that

[golden mortar]

luckily Embed is a subclass of UnsafeEmbed so it should be an easy fix then

[dawn merlin]

it should be isJSONEncodable(embed)

[sullen marlin]

Shouldn't there be more ClientOptions alike failIfNotExists? It'd be interesting for the exception layer to be more customizable by the consumer

[copper laurel]

Thats an API option

Its not something discord.js custom implements

[sullen marlin]

oh, didnt knew that. Thanks!

[copper laurel]

We do have a little bit of similar stuff in the RESTOptions for v14
https://discord.js.org/#/docs/rest/main/typedef/RESTOptions

e.g. the rejectOnRateLimit option

Which makes rate limited promises throw instead of queuing

So I think generally yeah we'd be open to more things of that style if its not disruptive to how djs needs to work

[charred mural]

Coming back to this...
#archive-library-discussion message

I've noticed that the rateLimit event also fires with the same 10500ms timeout instead of full timeout

[stoic olive]

I wish if Discord add feature that if the message button expire time was ended up, it replies with ephemeral message "This button is expired"

[wild flax]

Buttons don’t expire

[stoic olive]

I mean collectors

[wild flax]

That doesn’t make the button expire

Only our collector

[dawn merlin]

Usually when a collector expires I remove the button from the message so it can’t be clicked anymore

[stoic olive]

Similar to what I've done, but like uh.. forget about it

[wintry pendant]

whats the reasoning for typing getters as readonly instead of getters?

[zenith oracle]

Wdym? Getters are read-only...

[wintry pendant]

right but why don't you make them actually getters

[slate nacelle]

That notation didn't exist when the typings were made and there was no reason to switch is what I think.
Is there some advantage of switching?

[analog oyster]

correctness, i suppose. denoting them with get tells us that it will return an evaluated expression, which isn't necessarily the case with readonly

[outer raven]

There may be a readonly property out there that isn't a getter but maybe those are typed differently so if that works you could probably change it ig

and I just did the test and seems like some properties are typed wrong and having them typed as actual getters will tell you that

[vernal rose]

When I was adding commit, I saw git hook is running format test in all the files, which takes a few seconds. So I'm asking why u don't use lint-staged and only lint the files which were modified instead of checking all files.

[wild flax]

Won’t work in our setup

[ruby terrace]

Same problem, was going to be a simple fix until I realized there’s a bigger issue at hand here

[charred mural]

I see. If there is some other way I can help you, let me know

[wintry pendant]

I will make a pr then

is this undocumented for a reason?

[slate nacelle]

I don't think so.

[outer raven]

Any reason why Partials is an enum? It's only used as strings so I think it could perfectly be an array? Would also make it easier for us to specify all partials

[dawn merlin]

It’s pretty inconsistent to accept the gateway intent bits as an enum but not partials

[outer raven]

no it's not because those two aren't related at all. Intents is something from Discord, they only accept them as flags, while Partials are something from discord.js and is only used as strings, the numbers don't mean anything (and if you provide numbers it will error out)

[dawn merlin]

Yeah but we’ve moved away from using strings as representing a subset of values. Just because the api doesn’t use partials data doesn’t mean it’s not inconsistent with the rest of our library api

[outer raven]

but there's no such thing as a partial flag and there doesn't have to be. You moved away from strings because discord uses numbers. If partials are made up, there's no reason to make them hard to use

[dawn merlin]

They’re not hard to use at all

[outer raven]

makes input much longer and there's no way to add them all at once

[wild flax]

We moved away from strings because we didn’t want strings

Just to correct that statement

[outer raven]

well that's not true yet, lots of things still use strings on the library, but you're not benefiting from using an enum in partials at all

other bitfields are resolved from an array of flags into a single number and partials are still used as an array

so checking if an array includes a number or a string is the exact same thing

• the numbers have no meaning, 0 could be User or Channel

[dawn merlin]

That’s why you don’t use magic numbers

[outer raven]

exactly, that's why you don't use magic numbers

[dawn merlin]

Yes hence enums

[outer raven]

did you read what i just said or

you're not benefiting from using enums in this particular case at all

[dawn merlin]

so checking if an array includes a number or a string is the exact same thing

It’s the same thing it sounds like?

And the magic number argument still doesn’t make sense

[outer raven]

the only reason you are moving away from strings in other places is because discord doesn't use strings, they use numbers

so you have to transform them

discord is agnostic to the existence of partials, so you can use whatever you want

[dawn merlin]

Yeah, and we chose to use enums

It was deliberate

[wild flax]

Idk why you decide that for us rodry

I don’t want strings in the lib

I will say it till people get it

[outer raven]

but why don't you want them

[wild flax]

No matter if it’s OUR Interface or discords

[outer raven]

isn't it for the hassle of converting them back and forward

partials isnt even a class

its just an enum

it's impractical to use

[wild flax]

Because if you use 90% of enums to interact with discord, and then 10% strings, you might as well have a consistent interface

It’s basic api design

[outer raven]

right now you don't have a consistent interface

[wild flax]

Because 14 is in development

We do iterative programming

[dawn merlin]

Also the only reason we use strings is because some bitfields have bigint values which can’t be used as keys in objects

[wild flax]

Not “all at once”

[outer raven]

there was a PR to convert them "all at once" but alright, I'll do sth else then

[dawn merlin]

Which PR was that?

[outer raven]

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

[dawn merlin]

So the PR before the other 4-5 prs changing things to enums?

[wintry pendant]

that pr needed like 5 more to fix all the stuff it broke

[dawn merlin]

Even then it was expected that pr would have bugs and we were ok with that since it’s in dev anyways

Otherwise it would’ve been stuck in rebase hell

[outer raven]

Seems like we can no longer provide an object to SelectMenuComponent#addOptions or setOptions, can this be fixed?

[dawn merlin]

Should’ve been fixed by:

[loud jay]

#7466 in discordjs/discord.js by suneettipirneni merged <t:1645142675:R>
feat: add missing v13 component methods

[outer raven]

nope, it isn't

actually seems like that was fixed but it isn't in the latest dev release for djs

[dawn merlin]

Should be, the latest version has the same git hash

[outer raven]

discord.js has these versions in its dependency list and for some reason they don't resolve to the latest dev release

I currently have 0.13.0-dev.1644797188.0dfdb2c installed

[solemn oyster]

Probably due to cache

[outer raven]

how could I update them then?

[wild flax]

You’ll have to use resolutions

[outer raven]

how?

[wild flax]

Not supported by npm I think

But yarn has it

[outer raven]

I'm using yarn v1 on my project so it should work?

[solemn oyster]

Yes

[wild flax]

Yes

[analog oyster]

https://classic.yarnpkg.com/lang/en/docs/selective-version-resolutions/

[outer raven]

alright that worked, thank you!

also seems like we can no longer pass action rows in the form of an object when sending messages? The error is bigger than that but I can only show that much at once

and creating an action row instance doesn't work either

[zenith oracle]

from what I understand of that error it's saying that you can pass an object but components can't contain a SelectMenuComponent class object but should contain a plain object

same for the second error

I'm using raw objects and haven't encountered any problem so far

[outer raven]

hmm yeah seems like the types need fixing

[zenith oracle]

does it actually allows mixed objects though?

[outer raven]

no reason why it shouldn't

[zenith oracle]

so yeah ig

[dawn merlin]

I’m pretty sure it doesn’t if you want to mix then use .toJSON

[outer raven]

it does because it calls this on every component

[dawn merlin]

Which line calls it in djs again?

[outer raven]

in djs?

this is all done in builders

[dawn merlin]

Well it’s serialized in djs

[outer raven]

it only transforms casing

which in the case of action rows doesn't even do anything

[analog oyster]

this is raw -> builders

not the opposite

[outer raven]

huh?

[analog oyster]

you need to call .toJSON() when sending builders classes

[outer raven]

it checks if the component is already an instance of the component class

no you don't

not in djs

[analog oyster]

ah i see, but you sent a screenshot of an unrelated thing

[outer raven]

no it's not unrelated because when you call the constructor it calls this function

[analog oyster]

@dawn merlin

oh wait, i misread what you said initially

[tacit crypt]

it has overrides I think? Because yes consistency baby

[outer raven]

so the types are wrong right?

im not sure how to fix them because I don't know where the issue lies

[analog oyster]

yes

calling toJSON() fixes the issue temporarily, as siris mentioned
but you shouldnt need to

[dawn merlin]

@outer raven do you want me to fix the types or are you making a PR?

[outer raven]

I'm not sure how to fix them so feel free to

[dawn merlin]

Alright

[dawn merlin]

@outer raven I don't think I can do the PR without breaking the existing json types

btw this isn't the modals pr

[outer raven]

why not? Isn't it just adding the classes out of the possible types?

[dawn merlin]

because the component classes and json interfaces share the same properties, which means that if we union them the type constraints for them become loose. They become loose because builders hold partial data, while json objects don't

so basically we lose all of the required prop type checking for JSON

unfortunately if you want to use builders and don't care about the JSON types loosing you can use module augmentation

[outer raven]

well tbf component types have gotten so complex that i dont rly understand them anymore but if you add a class to a union type wouldnt typescript be able to check if you're providing an instance of that class or an object and cast the type accordingly?

I mean this was possible in v13 so it definitely is possible now, just needs some adjusting

[dawn merlin]

If I'm reading the v13 branch properly it suffers for the same loose typing downfall

I rarely mix json and builders so I probably never noticed

[copper laurel]

Yeah when I did components originally the types were very loose

Siris was the one who improved them so I'd trust him lol

[outer raven]

By “improving them” they became quite complex and often cause more issues than they solve
Iirc in v13 you do get errors for missing props when using an object in a component’s constructor, so that shouldn’t be an issue

[dawn merlin]

You’re arguing about them being too complex while also suggesting to make them more complex by adding a union

[outer raven]

A union isn’t more complex? It’s just another option for what you can pass in the constructor

[dawn merlin]

It’s not, you have to modify the *Data type itself

[vernal rose]

ig after https://github.com/discordjs/discord.js/pull/7197 CommandInteraction docs don't have options property. Is it's intentional?

[opaque vessel]

Yes

[zenith oracle]

Not sure if I did something wrong or this is already known but with the last dev version I'm not able to send an embed using Embed builder as it gets rejected by Discord saying that a description is required (even if I added one of course)
The json data sent is in the screenshot and the Embed doesn't seem to be correctly converted to json...
A reproducible sample is quite easy like the following:

interaction
  .reply({
    embeds: [new Embed().setDescription("test")],
  })

[outer raven]

oh the data

that should be an easy fix

[zenith oracle]

Should I open an issue or it's not needed

[analog oyster]

cannot replicate

[zenith oracle]

[analog oyster]

discord.js@14.0.0-dev.1645142857.f7257f0

[zenith oracle]

Same

[analog oyster]

did you import Embed from discord.js or builders?

i imported from discord.js

[zenith oracle]

builders

[analog oyster]

let me see

[zenith oracle]

Oh wait a moment

[analog oyster]

works still

@discordjs/builders@0.13.0-dev.1645142857.f7257f0

[zenith oracle]

Uh, strange

[analog oyster]

whats your builders version

[zenith oracle]

Same, let me send a screen of the dependencies

[analog oyster]

yarn why @discordjs/builders or npm ls @discordjs/builders

[zenith oracle]

let me try to reinstall

[analog oyster]

this is what i used ```js
const { Client } = require('discord.js')
const { Embed } = require('@discordjs/builders');

const client = new Client({ intents: 1 });

client.on('ready', () => console.log('ready'));

client.on('interactionCreate', async interaction => {
if (interaction.isChatInputCommand()) {
await interaction.reply({
embeds: [new Embed().setDescription("test")],
})
}
})

client.login();

[zenith oracle]

I'm deferring the reply actually but I tried it with reply and still

Reinstalled and used a normal reply, error is still there, I'm really confused now 😂

[analog oyster]

what does your src/structures/MessagePayload.js line 196 look like

discord.js folder in node_modules

btw, are you sure you're not using unsafeembeds? because there was a bug with them not being serialized and the fix is not on @dev yet

[zenith oracle]

yeah I'm using a normal Embed

[zenith oracle]

embeds: this.options.embeds?.map(embed =>
        embed instanceof Embed ? embed.toJSON() : this.target.client.options.jsonTransformer(embed),
      ),

oh wait

I think this should be fixed with the introduction of isJSONEncodable

just not published I guess

[analog oyster]

same as mine

in theory isJSONEncodable would only fix the issues with UnsafeEmbed ¯_(ツ)_/¯

i guess just wait for another dev release and see

[zenith oracle]

Well, this is weird ahahah

[zenith oracle]

Yeah but the fact that the embed is nested in data can only happen if the toJSON method is not called, which happens only if embed instanceof Embed is false, and it shouldn't actually, but isJSONEncodable checks for the existence of toJSON directly without using instanceof so I guess it should work.
Still not sure why it isn't instanceof Embed but... not such a big problem actually

[woeful rain]

I know, you don't like eta questions but... When will the new version of the djs be released (v13)? I see that everything (probably) is about done on the modals PR.

[copper laurel]

We don't do ETAs and everything is not done

[woeful rain]

Okay

[golden mortar]

I'm guessing one of u is using djs' Embed and another is using builders? Then the instanceof Embed would fail for one of you (whichever one djs uses). Either way problem is fixed next dev release

[analog oyster]

it worked with discord.js' and /builders embed for me

[golden mortar]

I think I had issues with builders as well although I didn't look into it much because siris made a pr almost instantly to fix it

[zenith oracle]

Yeah it was weird because the djs' code seems to import Embed from /builders, which I was using...

However should be fixed now

[upper echo]

Hey guys, was wondering if anyone could give me a run down on guild.members.fetch() and just how we are retrieving info at scale in general in discord.js before I dive into the code myself? I just ran that function in a very larger server and was shocked by how performant it was

major shout out to the contributors

[visual hornet]

it's event based, it tells discord that it wants members and then discord sends the members as gateway events in 1k member chunks until either it's done or it times out

[upper echo]

yeah it blasted through 100k with very little effort

[upper echo]

thanks for the high level explanation!

how large approx is each of those member objects?

[visual hornet]

the event in question is GUILD_MEMBERS_CHUNKS / guildMembersChunk if you want to listen to it btw

[upper echo]

tysm, so appreciate these breakdowns cuz it saves so much time while I am looking through it myself

[visual hornet]

i don't really know off the top of my head, but the full structure that discord sends is available on the discord docs if you want to see how much data it's sending
https://discord.com/developers/docs/resources/guild#guild-member-object

[upper echo]

yeah just put together something and shipped it really fast, now need to bridge the gap between understanding how this lib works to ensure it's performant

[copper laurel]

If you're fetching all members of a large guild, its never going to be particularly performant. However yeah as the others have said, rather than use a REST API to do this, discord.js sends a opcode to the gateway requesting members

Discord then sends back several guildMembersChunk events which the library collects until it has them all, matching guild.memberCount

[vernal rose]

ig this isn't intentional. is it? it should be ActionRow right?

[vernal rose]

index.test-d.ts

ig expect error is there only for empty object not for wrong class name.

[analog oyster]

yes, it should be ActionRow

[quiet viper]

Are any kind of ratelimit issues known, which causes a local limit on a specific route/resource?
Did not found any issue on github and others can not reproduce it.

Ik, wrong channel, its not a Discord bug, raw requests works with status 200

The new rest manager has the same issue...

|| #944920036471111680 ||

[fast jacinth]

Does anyone know if the next version will have the ability to add a banner to guild event creation?

[outer raven]

that's not possible in discord assuming you mean creating a guild with a banner because you need boosts to do that

[analog oyster]

the dev version has it

if you talking about guild scheduled events

[fast jacinth]

yep, awesome thank you

[opaque vessel]

@outer raven how are you managing https://github.com/discordjs/discord.js/pull/7529? What code gave you a union of all channels?

[outer raven]

like I said in the description, an options object with no type specified

turns out the test with an empty object was seen as an undefined value somehow

so TS didnt pick up on that

if you specify one of the properties you get the bug

[opaque vessel]

I'm not able to reproduce that

I'm not sure how that comes to be because the type is required in options, so that overload should be skipped and head on over to the last one, right?

And wouldn't these tests pick it up? O_o

[outer raven]

well this is awkward, because I can't reproduce what just happened to be 10 minutes ago

I think I know what happened here give me a second

yep, so I had a channel with an incorrect type set as the parent, which messed with the types

guess I'll close that because that should never happen in normal conditions, my bad

[opaque vessel]

Is okay [:

[vernal rose]

@dawn merlin

function copyProperties(target, source) {
    for (let key of Reflect.ownKeys(source)) {
        if (key !== "constructor" && key !== "prototype" && key !== "name") {
            let desc = Object.getOwnPropertyDescriptor(source, key);
            Object.defineProperty(target, key, desc);
        }
    }
}

https://github.com/discordjs/discord.js/issues/7517#issuecomment-1048262707

will this work?

[dawn merlin]

Sure but would it copy the setters? Because the setters shouldn’t be copied

[vernal rose]

lemme test all i didn't tested it

[dawn merlin]

Also have you looked at InteractionResponses.applyToClass? You might be able to take some inspiration from there

[vernal rose]

lemme check it bc ig above one isn't working as expected

[vernal rose]

I was testing with few functions and found this one helpful but it includes creating instances but I can't find any other good way to do this i'll try later

function cloneClass(target, source) {
  for (let key in source) {
    if (source.hasOwnProperty(key)) {
      target[key] = source[key];
    }
  }
  return target.constructor;
}

const B = cloneClass(new A(), {});
console.log(new B().b);

copying properties and gatters only

edit: above approach was wrong

[dawn merlin]

ok

[vernal rose]

In your case ig something like this will work

[dawn merlin]

nice, if you don't mind you can make a PR. Probably easier since its ur idea

[vernal rose]

okay sure. I'll try some other if I can find any simple alternative or i'll use this one

[vernal rose]

@dawn merlin I've found another workaround which is working fine. But how can I update typings? Should I create a new interface for components coming from api (ButtonInteraction.component)?

[outer raven]

@dawn merlin did you test the code sample you sent on https://github.com/discordjs/discord.js/pull/7531 on main?

[vernal rose]

how can I fix them?

[outer raven]

if the CI doesn't error you can ignore that

run yarn build to see if it completes

[vernal rose]

yeah that's why I was ignoring but as no was reporting so I thought these errors are coming to only me

[vernal rose]

lemme try

[outer raven]

they're not, but they don't error on dev builds because yarn resolves the other packages to their latest dev versions

[vernal rose]

oh btw build worked and now only rest have error

[outer raven]

did you run build on the root directory?

[vernal rose]

yeah

[outer raven]

well then im not sure in that case, but as long as you can run the build script it should be all fine

[dawn merlin]

@vernal rose have you tried looking to see if mixins work:https://www.typescriptlang.org/docs/handbook/mixins.html

[vernal rose]

tbh I don't know how mixin works. I'll check it and will see if I can use it.

[digital sun]

Found a bug when following the official discord js guide and I had to hack to get to work. Sub commands are not assigning the type when using SlashCommandBuilder.addSubcommand and the API throws this error options[0].type: This field is required

What I had to do to get it to accept commands with sub commands was to manually add it:

or (const file of commandFiles) {
        const command = require(`./commands/${file}`);

        if (command.data.name === 'assign') {
            command.data.options[0] = {...command.data.options[0], type: ApplicationCommandOptionTypes.SUB_COMMAND}
            commandModules.push(command.data);
        }
    }
    commands.set(commandModules)

[copper garden]

You have to call .toJSON() on the builder before using it in the api call

[digital sun]

ahh

think that was missed in the guide walkthru

[copper garden]

It's in the guide as shown above

[digital sun]

ooo

well thanks!

[opaque vessel]

Hello @void rivet, do you think you can backport https://github.com/discordjs/discord.js/pull/7436 to the v13 branch? [:

[void rivet]

i'll look into it, sure

[opaque vessel]

[: thank yous

[void rivet]

so what, this is for 13.7?

[opaque vessel]

Yea, the backport

https://github.com/discordjs/discord.js/pull/7337/files this looks to be the other half of this feature :thinking:

[void rivet]

yeah i saw that making the pr, ig it was an oversight or even wasn't documented when it was made

[opaque vessel]

Yea probably

[void rivet]

https://github.com/discordjs/discord.js/pull/7549 👍 done

[steep zodiac]

Howdy
Can anyone confirm `npm i discord.js" works ?
I can install other modules, but with discord.js it throws an error, so I wanted to know if the issue comes from me

[analog oyster]

yes, it works

[steep zodiac]

Uh ...
Weird but thanks

I use discord.js for a few months and I never had this issue, strange

In the log file it ends up telling

"Invalid response body while trying to fetch https://registry.npm.org/@types%2Fnode: EIO: i/o error, read

[analog oyster]

npm.org?

[steep zodiac]

Idk, it's the error I get when I try to install discord.js
Never got this before so it's weird

I searched for this package on the npmjs registry, it exists, but I cannot install it with "npm i @types/node"

I was about to check djs packages.jsos dependencieson github

[analog oyster]

not an issue with discord.js

its trying to download @types/node from a random website instead of registry.npmjs.com

[steep zodiac]

Any explanation ? 😭

I tried "npm i https://registry.npmjs.com/@types/node", but I got a lot of "checksum failure" 🤔

[raven juniper]

this is not a support channel, this is for the development of the d.js library

[steep zodiac]

Oh sorry
I tough it should be discussed here because it's not really related to the use of the library and I just wanted to know if I'm alone

So it's one of #djs-help-v14 channels to go further ?

[analog oyster]

no, #useful-servers #869652447881207858

[steep zodiac]

Alright
I will stick to this server, so #other-js-questions, thanks ^^

[opaque vessel]

Because you can only edit the logged in user's voice state, not any other one, so I don't think it would belong there
@outer raven I don't wanna say this on GitHub but this isn't true...

[outer raven]

then why was there this check to begin with?

[opaque vessel]

Why are you just looking at one method

You've grouped up a method for only the client user and everyone else into one method for just the client user, removing functionality

If we could only edit the voice state for the client user... why would this class exist in a cache lol

[outer raven]

calm down alright

[opaque vessel]

I still think they should be added to the manager so you can edit an uncached user's suppressed state

[void rivet]

Speaking of this
What really plays into whether or not a method should or shouldn't be on the manager?

Like let's say messagereactionmanager.fetchUser

[opaque vessel]

If the route belongs to something there, it should be in the manager

[outer raven]

i mean i made a PR to add every method to it's manager where applicable

if there is a manager it should have it

[opaque vessel]

This is on the ReactionUserManager

[void rivet]

Is that pr still up?

[void rivet]

Yeah but you need a specific reaction

[outer raven]

been merged already

[outer raven]

could be on both

[outer raven]

editing an uncached voice state will never be possible, should this still be on the manager?

(you always need to access the voice state's channel)

[opaque vessel]

You can just disable voice state caching so that seems very possible to me

[outer raven]

But if you do you won’t be able to edit them

It’s not possible because you lose the channel link

[opaque vessel]

What do you mean

[outer raven]

when you send the request you need to send the id of the channel the voice state belongs to. If the voice state isn't cached you can't know what channel it's on

[opaque vessel]

Why is that a problem

[outer raven]

what

how is it not a problem

[opaque vessel]

Why do you think we need it cached

[outer raven]

I literally just explained what

[opaque vessel]

I don't understand the confusion sorry o,o

Have the user provide the channel id?

[outer raven]

that's not gonna happen obviously

the voice state is already on a channel

and this method requires you to be on a stage channel

[opaque vessel]

I don't think I can see the issue still

[outer raven]

well I'm not gonna add it for now because of that, if you want you can PR it yourself later on

[opaque vessel]

I just coded it into the manager and it works fine... I still don't know what the issue is

[outer raven]

with the channel as a parameter, which I don't wanna add

otherwise you'd need to use resolve and not resolveId

[opaque vessel]

You should add it to expose the raw functionality

[opaque vessel]

Then do that and do the cache checks where there is definitely a channel

[outer raven]

if you use resolve then you can't use it with uncached voice states

[opaque vessel]

Yes so do that where there is a cached voice state

[outer raven]

no because then you'd be moving the user too which is not the point

[outer raven]

so whats the point?

if the manager method only works when cached

[opaque vessel]

I genuinely don't understand the confusion here

Just move the method to the manager, anything that requires a cache check keep it in the voice state and keep things raw in the manager

[outer raven]

it's not a cache check

you literally need the whole class to make the request

it's not a check

and im not gonna add the channel as a parameter

because moving users is a method for the guildmember class and its manager

[opaque vessel]

You lost me again, why are we talking about moving members o,o

[outer raven]

if you provide the channel id as a parameter you're adding the possibility to move the member

which is not what this method is about

it's simply about editing those 2 props

[opaque vessel]

Providing the channel is a requirement to edit a voice state

Voice states don't change a member's channel

[outer raven]

yes they do

[opaque vessel]

They don't

[outer raven]

if you're on channel A then edit the voice state as channel B you moved the member

bro

[opaque vessel]

That's not from the voice state's endpoint

That's editing a guild member

There is only two things you can do with a voice state: edit a suppressed state & set your request to speak

Everything else you see in there goes to the guild member endpoint

[outer raven]

then that's even worse: you're providing a parameter that can only be 1 thing and cannot be used for anything else

the error if you provide a different channel ID is "Unknown Voice State" which would sound like a library error

[opaque vessel]

Their problem lol

[outer raven]

yeah no I don't think that should go in the library

if you do, go ahead and make it

[mighty sequoia]

sorry to bring this up again, but this doesn't work for dms since they aren't cached guilds,

[copper laurel]

Yeah this still isn't about library development either way

isCommand would still work

[mighty sequoia]

it doesn't

how would it not be about library dev if this is something missing in the lib

[copper laurel]

Because nothing is missing, as per the original discussion

These are typeguards

[mighty sequoia]

is a problem with the typeguards not a problem with the lib?

[copper laurel]

What do you believe is missing?

[mighty sequoia]

a way to get the original commandName of an interaction in dms?

[copper laurel]

The workaround would be to cast it directly to SelectMenuInteraction<'raw'>

though to typeguard it properly, I guess an .inDM() could be added

[outer raven]

Why can’t you just access Interaction#commandName

[opaque vessel]

It's not a command

[outer raven]

context menus are commands so they have a command name

[dawn merlin]

What does this have to do with context menus?

[outer raven]

Nvm

But there should be a typeguard for asserting something cannot be the api type but can also be null

Which I think would help here

[fervent walrus]

any reason why onClose is not being invoked which result in shard not resuming after a zombie connection[4009]?

Ive been trying to debug WebSocketShard.js to find why the shard is alive but isnt responsive ie zombie connection due to 4009. I am failing to understand why it wont invoke onClose method. when 4009 occurs randomly
But when I try to recreate a zombie connection it does invoke onClose which later on reconnects from the WebSocketManager.js

[void rivet]

https://github.com/discordjs/discord.js/pull/7384 shouldn't this be backported as well?

okay then making a pr

[dawn merlin]

7576 can be backported as well

[loud jay]

#7576 in discordjs/discord.js by suneettipirneni opened <t:1646018618:R> (review required)
feat(StageInstance): Add support for associated guild event
📥 npm i suneettipirneni/discord.js#discord.js/feat/stage-instance-events

[loud jay]

#7384 in discordjs/discord.js by fowlerro merged <t:1644664130:R>
fix(guildmember): check if member has administrator permission

[fervent walrus]

I am trying to fix websockets https://github.com/discordjs/discord.js/issues/7450
Can someone help?

[alpine bough]

is there any way to fix it?

[vernal rose]

No I had created an issue in eslint/import you can check here

https://github.com/import-js/eslint-plugin-import/issues/2354

[alpine bough]

thanks!

[outer raven]

Yeah there is that isn’t an issue with the plugin 🤔

[mighty sequoia]

I don't think It's possible to coerce an AnyChannel to a Textchannel that can have threads in it, since isText() allows for NewsChannel too, so I still can't use the threads object?

[outer raven]

a typeguard could be made but honestly that sounds like a bit of an edge case

you're better off just doing if ('threads' in channel)

but it could def be made and quite easily too

[mighty sequoia]

how so?

this seems like a reasonable case

[mighty sequoia]

cant since anychannel has threads in it

[outer raven]

Huh?

AnyChannel is a union of types

If you do that you narrow it down to the channels that can have threads

[outer raven]

Make a PR, see how it goes

I also don’t know what you’d call a method like that tbh

[mighty sequoia]

.hasThreads

[outer raven]

No that’s misleading

[mighty sequoia]

.canHaveThreads

[outer raven]

Ye that’s more like it

[mighty sequoia]

or add .isNews() and then you can filter that out

[outer raven]

That is a thing my guy

[mighty sequoia]

no its not…

check the docs

[rustic bough]

Documentation suggestion from @void rivet:
<:_:874573924988518500> Channel#isNews()
Indicates whether this channel is a [NewsChannel](<https://discord.js.org/#/docs/discord.js/main/class/NewsChannel>).

[vernal atlas]

what channel types can have threads

[mighty sequoia]

TextChannel thats it

[inland lotus]

.isThreadBased()? or does that sound like its a thread channel not a channel that has threads

[mighty sequoia]

its on main not on release

[analog oyster]

news channels can have threads too

[vernal atlas]

sounds like a thread channel

[analog oyster]

sounds like a forum channel

[vernal atlas]

canHaveThreads sounds best tbh

[analog oyster]

while not misleading, thats not a very good name. but i cant think of anything better

[vernal atlas]

yeah its not a very pretty name but cant think of anything else either

[mighty sequoia]

just use the canHas one

[mighty sequoia]

Unrelated, but this seems like with djs too:
is there a reason that ```js
const channel = await client.channels.fetch(config.THREAD_HELP_CHANNEL)
if (channel === null) return true
if (channel.isText()) {
const { threads } = await channel.threads.fetchActive()
for (const [, thread] of threads) {
const msg = await thread.fetchStarterMessage()

throws at the `const msg = await ...`.:

DiscordAPIError: Unknown Message
at RequestHandler.execute (C:\Users\12156\Documents\git\prismarine-bot\node_modules\discord.js\src\rest\RequestHandler.js:350:13)
at processTicksAndRejections (node:internal/process/task_queues:96:5)
at async RequestHandler.push (C:\Users\12156\Documents\git\prismarine-bot\node_modules\discord.js\src\rest\RequestHandler.js:51:14)
at async MessageManager._fetchId (C:\Users\12156\Documents\git\prismarine-bot\node_modules\discord.js\src\managers\MessageManager.js:219:18)
at async userAlreadyHasOpenThread (C:\Users\12156\Documents\git\prismarine-bot\src\threads\index.js:14:23)
at async Client.<anonymous> (C:\Users\12156\Documents\git\prismarine-bot\src\threads\index.js:29:59) {
method: 'get',
path: '/channels/----/messages/----',
code: 10008,
httpStatus: 404,
requestData: { json: undefined, files: [] }
}

[vernal stratus]

is the builders package being expanded on still?

[fallow crater]

How stable is v14 right now? I know it's still in dev so there can be breaking changes, but is it worth already using it in prod 😅?

[copper laurel]

It isn't stable

We can and still are changing it at will

[outer raven]

Yes its on dev because it was a breaking change, it won’t be on v13

[outer raven]

Some things don’t work at all atm so I wouldn’t recommend it just yet

[outer raven]

The reason is probably that the message was deleted

[mighty sequoia]

It isn’t and I checked, besides that shouldn’t cause a crash anyway that should just return null

[outer raven]

If it tries to fetch an unknown message it should def cause a crash

[mighty sequoia]

that's not desirable behavior at all, it should just return null

[unique axle]

.fetch* is making an API call and subsequently returns a promise
if anything goes wrong (the API errors) that will cause the promise to reject
that is desirable behaviour.
handle your promise rejections gracefully and if you really don't want it, wrap it in a function that resolves the promise with null instead

[mighty sequoia]

I see okay

[echo ginkgo]

Just a quick question as I'm looking to use discord.js for a project at polytech, is the node module free to use for non-commercial projects like this, and are there any licenses I need to make note of and comply with?

[burnt cradle]

it has an apache 2.0 license

[void rivet]

could https://github.com/discordjs/discord.js/pull/7292/ be backported as well?

[vernal rose]

issues like https://github.com/discordjs/discord.js/issues/7192 can be closed right? there are no response since 2 month. If anyone still have the issue they can create a new issue.

[outer raven]

I mean that issue should've been closed when it was made because it's not an issue lol

[analog oyster]

shouldnt the /builders addChannelTypes() accept rest parameters instead of an array?
https://github.com/discordjs/discord.js/blob/53defb82e36108468e35077b887ee28b811891ab/packages/builders/src/interactions/slashCommands/mixins/ApplicationCommandOptionChannelTypesMixin.ts#L51

[opaque vessel]

I think so if we wanna stay consistent? @dawn merlin^

[dawn merlin]

Yeah

Speaking of, I need to do everyone’s favorite consistency PR today

oh nvm almeida did it for me

[analog oyster]

[real jetty]

I think I found a bug? this method is already called internally on toJSON and has no docstrings https://github.com/discordjs/discord.js/blob/main/packages/builders/src/interactions/slashCommands/mixins/ApplicationCommandOptionBase.ts

[outer raven]

done https://github.com/discordjs/discord.js/pull/7591

[opaque vessel]

It's not hidden because it's not a private method

[golden mortar]

can we readd addField?

[unique axle]

no, for all the reasons listed here: https://github.com/discordjs/discord.js/pull/7533

[outer raven]

I tried to keep it relevant but guess they didn't feel like it

[real jetty]

Shouldn't it be though?

It seems like an internal method considering its already called on toJSON and i don't see why any user would want to run it twice

[opaque vessel]

Can only either be private or protected

[real jetty]

Could you do some jsdoc fuckery where you put @ private? Or would ts not prioritize that?

[opaque vessel]

Putting @private on it would work

[analog oyster]

the method is already set as protected (in ts, it has no jsdoc)

[golden mortar]

That PR fails to mention the burden of moving from v13 to v14 or developer experience completely, and it got closed too fast for any real argument to be made for or against it. I don't even care much about bringing back the v13 syntax, I'm against removing it entirely

It's a simple utility method that has existed for probably well over 5 years now that has (a) no maintenance burden, it was a 1 line utility; (b) had no memory footprint/ performance implications whatsoever; (c) is widely used (unlike a certain equals method recently added 🤭 ); and (4) is another deterrent from switching over from v13 -> v14.

In the original PR, I hardly see any argument for removing it, if we distinguish opinions from actual arguments. I didn't actually expect the addField removal PR to be accepted so I never got to make these comments on it.

• "redundancy" from advaith's argument: If the goal is to remove redundancy then imo addFields should be removed instead of addField, which is probably used a lot more. You replied that addFields "makes much more sense, you can add multiple, but adding one is fine too.". Now you can probably see the issue... it all comes down to an opinion. In my opinion, adding 1 field using addFields doesn't make sense, whereas adding multiple fields with addField does make more sense. Therefore I don't consider this point valid...
• advaith also brought up another point: popularity. A quick search on github shows that addField is used 444,000 times. On the other hand, addFields is only used 85,000 times (https://github.com/search?l=JavaScript&q=addField(&type=Code vs. https://github.com/search?l=JavaScript&q=addFields(&type=Code). In other words, addField is used 5x more than addFields. I searched through a few pages of addField and every usage was from djs.

p.s.: im not gonna reply to this or really view it as I already made my point and have nothing much else to say, plus you probably couldn't change my mind anyways 🤷‍♂️

[outer raven]

I don’t understand how or why you prefer to add multiple fields by calling addField every time as opposed to simply passing all of them as arguments to addFields but ok 🤔
Imagine trying to add 25 unique fields manually through that method… wouldn’t be fun at all

[dawn merlin]

Looking at the search results I can confidently say that there's much better ways to do this: