archive-library-discussion

[outer raven]

should it be optional or required

well I'm just gonna make the PR and make it required then if there's no response

[wild flax]

It doesn’t matter in our case really

[outer raven]

but it should follow what TS does

imo

not that I agree with their decision of making it required

but 3 maintainers gave their opinion on it so its fair to assume it's not changing soon

[wild flax]

Yeah it still won’t matter too much for us

TS says one thing, mdn another, if it works either way, I don’t put it high enough on my priority list to care

And since we don’t use the built-in signature atm it’s whatever

[outer raven]

TS and mdn agree though

mdn never said index was optional

neither did the spec explicitly

created the PR if you wanna have a look: https://github.com/discordjs/collection/pull/51

[final solar]

Hey is there a way i could have these messeges in my server from #discord-api-status

[unique axle]

host it yourself, i suppose: https://github.com/almostSouji/discord-status-webhook
i hardcoded quite a lot of things

[final solar]

Thanks

[outer raven]

Why don’t you guys make it a news channel

[unique axle]

can't edit published messages often enough

[outer raven]

Ah yeah

It’s twice per hour right

[outer raven]

Can we get a new collection release for the #reverse method and the at types fix?

[junior pumice]

hey im trying to create a PR and i was wondering where properties like the nickname are being defined...

[outer raven]

in the _patch function

[vernal adder]

https://github.com/discordjs/discord.js/blob/stable/src/structures/GuildMember.js#L68

[junior pumice]

oooooooh tyvm

what branch would i need to create the PR on

[vernal adder]

main

[junior pumice]

okay tyvm

https://github.com/discordjs/discord.js/pull/6949 i hope i did it the right way

[unique axle]

should probably follow other timestamps and assign the timestamp + have a Date getter
like premiumSince / premiumSinceTimestamp https://discord.js.org/#/docs/main/stable/class/GuildMember?scrollTo=premiumSince
to name one example

there might (i haven't checked/don't know) also be the issue of it only being in the data when the property actually updates, which you'd need to guard - else it might reset it to null, if you get another guild member update that does not provide this field

[outer raven]

why are properties even set to null to begin with on GuildMember?

shouldn't they just be set in the patch method

[outer raven]

also that doesn't seem as easy here since all those timestamps you're referring to happened in the past and this is a future timestamp, so what would the date getter be called?

[junior pumice]

so make it a getter instead of a property?

[outer raven]

no, make a getter and a property

[junior pumice]

aaaaaaaah i see okay

[outer raven]

getter - timestamp
property - date
I think ?

[opaque vessel]

Is that property even documented?

[junior pumice]

not yet

[outer raven]

not yet, no pr either

but time outs were added recently to an experiment iirc

[opaque vessel]

It won't be merged on discord.js until it is, so I'd rather you start devoting your efforts there lol

[junior pumice]

yeah i know but i wanna use it asap in djs

[outer raven]

I would recommend waiting for more info to arise, because all we have atm is that property and a red button on a member's context menu

[outer raven]

you won't until it's publicly available

[junior pumice]

[outer raven]

unless you install your fork that is

[junior pumice]

i know lol

[opaque vessel]

Well once again it will only be merged if it's even merged on the actual documentation so... you're blocked if anything

[outer raven]

he doesn't have to make the dapi docs pr tho, I did the same for the guild progress bar cuz I'm not familiar with the docs' structure

[opaque vessel]

I'm not saying he should. Just stating it won't be merged until someone does it

[junior pumice]

that wasnt the plan

i know prs take time to get reviewed and i hoped the mute feature would be documented before my pr gets reviewed

[outer raven]

yeah speaking of that, you're gonna need to add methods to mute members too

[junior pumice]

i know but idk what to name them lol

[outer raven]

disableCommunication maybe

[junior pumice]

true

thanks

[outer raven]

make it call #edit though

[junior pumice]

huh

[outer raven]

that method should be a shortcut to #edit

[junior pumice]

@outer raven do you know how i would need to handle errors say if i check if the timestamp is in the past?

[outer raven]

you'd likely not be passing a timestamp, but a time in seconds to mute the member for

just a guess tho

[junior pumice]

hmmmmmm

okay

[outer raven]

Hey @wild flax sorry for the ping but it would be nice to be able to use the new method

[wild flax]

Hi, this is Crawl. You’ve reached me after hours. Please expect a reply by EOD Monday.

[outer raven]

sure thing, thank you Crawl messaging services

[real jetty]

i suggest you throw an error when creating a webhook in a channel that has reached the maximum amount of webhooks (5 webhooks)

[junior pumice]

@outer raven i changed the stuff you commented, thanks for looking at it

[outer raven]

is this limit documented?

[outer raven]

alright, please just resolve the conversations that were addressed to make it easier to see what was done

also you missed 1

[junior pumice]

wait frick i messed something else up

[real jetty]

no

[outer raven]

then try making a PR to discord api docs

but I think the api would send an error when trying to surpass that limit, if it really does exist, right?

so why would djs need to do that

[visual hornet]

idk what you mean

[outer raven]

[opaque vessel]

The maximum is 10 in a channel

[junior pumice]

mhm

[visual hornet]

that does already give an error

[real jetty]

is it just me then? how much webhooks max can u create in the guild 🤔
because when I try to make more than 5 webhooks in a channel I get internal server error

[visual hornet]

that's definitely not the same

[outer raven]

internal server errors are usually random and related to outages

[junior pumice]

uh

how would i need to implement the stuff in the edit method?

[outer raven]

look at the current implementation

[junior pumice]

i dont quite get the commend with the checks

[outer raven]

the edit method has tons of checks, they should not be done in shortcut methods

[junior pumice]

but i havent had any checks

[outer raven]

also who's the guy that's undoing your commits and why

[outer raven]

you did

[junior pumice]

where lol

[outer raven]

the method should be 1 line and 1 line only and should be passing the seconds value to the edit method

[junior pumice]

uh

[outer raven]

with no other modifications

[junior pumice]

okay

[outer raven]

those can be done in #edit if needed, which I don't think they are

can you redo the commits that your friend undid or maybe revoke their access?

[junior pumice]

he said my pr was messy and he wanted to fix

[outer raven]

doesnt seem like he did

[junior pumice]

hmmmmmm

ill change it later

but only passing the seconds to the edit method doesnt make sense

bc the data i pass will then be sent to discord

so i would have communicationDisabledUntil and communication_disabled_until property that will then be sent to discord

or would it work like that

[rare bronze]

should be better now

git on mobile is a pita

[outer raven]

Why would anyone use git on mobile I-

[rare bronze]

I have no access to my pc and I couldn't wait

[outer raven]

You’re gonna be passing seconds not an ISO string to discord

[wild flax]

the feature isn't even documented, what is it that you can't wait for lol

[outer raven]

The pr is gonna take a while to get merged, you can wait

[rare bronze]

I only saw the fork, not the pr so I couldn't know

but whatever I merged the commits lol

[junior pumice]

@rare bronze can you push it again lol i fixed some stuff in the fork

[rare bronze]

ok now you will wait, I'm not force pushing 4 times again on mobile

[junior pumice]

okay

[wild flax]

you know you can just run eslint to fix all that stuff except the max-length

[junior pumice]

oh

crap

this is my first time

[wild flax]

I mean, I can see that lol

[junior pumice]

im trying my best

[junior pumice]

i think i "fixed" everything now i hope its fine now

[wild flax]

do you edit things in the browser? lol

literally all you have to do is npm run lint:fix

[junior pumice]

oh

crap

never used console git before

[wild flax]

thats npm though

[junior pumice]

i dont have a local copy of it tho im only in browser

[outer raven]

You really should

If you use vscode and commit through it it runs eslint for you

And shows you errors if you have the extension

[junior pumice]

yeah sorry

[outer raven]

Why do you insist in passing a date?

[junior pumice]

where

[outer raven]

For communication_disabled_until

[junior pumice]

wh-

where

file and line

the only point where i am passing a date is to the api because it requires an ISO8601 timestamp

[outer raven]

that's what I'm saying: it likely does not, but we can wait for the dapi docs pr if you wish

[junior pumice]

bruh i tested

i can even show you the error

[outer raven]

go ahead

[junior pumice]

see

[outer raven]

interesting

[junior pumice]

they might change it but for now its an ISO8601 timestamp

[outer raven]

aight then

[junior pumice]

mhm

[junior pumice]

@outer raven I think I'll just delete the PR I'm not ready to contribute yet

[outer raven]

don't give up, you should learn from the reviews

[junior pumice]

:(

[outer raven]

I tried to explain what's missing in the reviews, if you have questions you can ask there

but I don't think you should close it

[junior pumice]

ill update it

wanna give me the error tho?

im very bad at making short explaining errors

[crisp eagle]

Did anybody got that error ?

ClientUser ??= require('../../../structures/ClientUser');
              SyntaxError: Unexpected token '??='

I never saw that ??= expression

This is a error thrown from the package's code

[slate nacelle]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Logical_nullish_assignment

You need to update your node version, we require 16.6 or up.

[crisp eagle]

Thank you ! 🙂

[vivid field]

shouldn't that be a top-level import anyway, now that User can't be extended anymore?

[slate nacelle]

Possible

[junior pumice]

i've changed everything you suggested - could you take a look again?

[outer raven]

Will do, but please use vscode and not the browser next time

[junior pumice]

👀

does browser work too?

[outer raven]

Very badly

[junior pumice]

[outer raven]

You have eslint errors that will be fixed if you install vscode and commit with git

[junior pumice]

uggh

okay

but i cant do it today i gtg

[junior pumice]

Yo @ruby terrace can I like temp remove the PR until this is final bc as you said it doesn't throw an error with dates in the past (so I think they'll change that in the future) I'll make it ready once they release it

[real jetty]

You should be able to just make it a draft PR

[junior pumice]

Wait really? Where

[ruby terrace]

you could either remove or mark as draft, as draft some people will still review, but its more of a glance over than an actual review

click the convert to draft here

[junior pumice]

aaaaaaaah thanks

[real jetty]

I would like to know if it's possible to retrieve raw data from GuildChannel instance ? If not, I think it can be really useful sometimes if we can access raw data from object instance

[ruby terrace]

that's in the plan for v14!

[real jetty]

😭 I will keep my non-optimize methods until v14 😂

[outer raven]

wait it is?

is there an open issue about it?

[loud jay]

#6567 in discordjs/discord.js by kyranet opened <t:1630328859:R>
Roadmap: raw data storage and state changes

[solemn oyster]

@outer raven ^

[outer raven]

ooohh I remember reading that but didn't read it fully, thanks!

[solemn oyster]

np ^^

[loud jay]

#6958 in discordjs/discord.js by suneettipirneni opened <t:1636305505:R>
Remove *Data types from Discord.js types

[copper laurel]

Going to continue this discussion here since I don't like long comment chains on Github

I agree that it makes sense to use discord-api-types when the return value is actually an API type

If we have to write a translation utility type, then I think we're going down the path of an over-reliance and shoehorning in what is essentially an incorrect type where it doesn't belong

The issues is exacerbated by the fact that /builders produce snake_case JSON. If they're builders for discord.js, then to align with Rodry's assertion that camelCase is the standard for JS, then thats what /builders should be exporting as JSON

discord.js compatible objects

[ruby terrace]

I think the main sticking point is going to be slash commands, components, and anything else in builders. Since builders outputs API ready data, ...

yea

the question becomes whether builders is really an augmentation module for discord.js or if its supposed to be standalone

[dawn merlin]

The issue is that builders is meant to be used with rest also

[copper laurel]

We need to take a step back and review architectural decisions here before implementing more shit to sit in the middle and tie it all together

Then it needs two separate output functions

toSnakeCaseJSON() toCamelCaseJSON()

boom done

[dawn merlin]

I suggested this and even made a PR but it was shot down

[copper laurel]

lol

If it's standalone, then discord.js shouldnt support them

Gotta get the fuck off the fence and pick a side here

[ruby terrace]

probably because neither one of those functions would get called automatically with JSON.stringify()

[copper laurel]

Anyway, well aware my opinions are probably also controversial but here they are

[dawn merlin]

The reasoning given was that they didn’t want djs to be a hard dep for builders

[ruby terrace]

(I saw that the first time) I don't understand how it would be a hard dep though

[copper laurel]

Yeah, I'm not here to call out any specific decisions as being wrong, but we are definitely running into some incompatibilities as a result

api-types, rest and builders all align with snake_case support, great, but then discord.js is the odd one out that should drop camelCase

Or at least theres an argument to be made for that

[ruby terrace]

I think application commands in particular put us in this situation because of their unique deployment methods

[dawn merlin]

Idk I would ask Kaira and vlad as for the reasoning

[copper laurel]

ehhhh its not that unique

[ruby terrace]

unique in that its really designed to be done outside of an active connection

[copper laurel]

Well yeah, its an API deployment that doesn't require a gateway connection

But thats also true of

• creating a channel
• creating a role
• create an application command
• sending a message

All can be done without a gateway

discord.js just happens to be a gateway library

And thats fine, but maybe we need to look at why we're trying to build this complex bridge between two different approaches

[ruby terrace]

I think there was never really much of a point to being REST only, and if you were, you didn't have any equivalent to builders

because you sure weren't going to install discord.js and use the structures to create raw REST data

[copper laurel]

absolutely agree, so why are we trying to now

[ruby terrace]

because we're trying to encourage not deploying application commands through djs (wait...so, don't accept API style). I think that's actually the solution, but then we're kinda duplicating the code in builders for components? Or maybe it already is, can't remember off the top of my head

so builders should output API ready data only, discord.js shouldn't accept that

[dawn merlin]

It didn’t until relatively recently

Mainly because people complained they couldn’t use builders directly with djs

[ruby terrace]

yeah, cause people were complaining (guess we shouldn't have listened)

[dawn merlin]

Is that sarcastic or serious? couldn’t tell lol

[ruby terrace]

I haven't used builders, but what does it have right now?

application commands, components, embeds?

[dawn merlin]

Just app commands

No components and embeds (yet?)

[ruby terrace]

oh, no components, but embeds and string formatters

[dawn merlin]

I thought embeds were in a PR?

[opaque vessel]

They were released in 0.6.0

[dawn merlin]

Ah ok

I was thinking of message button

[copper laurel]

I think our component guides use builders?

idk

I might be getting mixed up

Oh yeah I never fixed my buttons PR lol

[outer raven]

Lemme just give my two cents on the matter of compatibility with djs and the other packages
Someone up there said that discord.js is the odd one out in using camelCase but that’s not necessarily true. Let me explain
Builders is supposed to be a utility package that helps you create complex object with just a few functions and makes sure they’re all correct before sending to Discord. These functions are written in camel case and, since it’s a package made to interact with discord, it produces api-compatible objects. So the package itself uses camelCase for the front-end, and produces snake_case outputs, which is fine
What I think is that we shouldn’t explicitly document support for snake_case properties in djs, however, if someone wants to pass them they can, only thing preventing them will be TS errors. Under the hood we can still support snake_case for compatibility with builders and mention that compatibility, but never explicitly mention snake_case support, as we wanna stay away from that. This might be harder than it seems on the TS side, but it’s definitely doable.
We can easily implement this without needing a big change, just a friendly, under the hood thing to be compatible with the other packages as they’re all associated with discordjs

[copper laurel]

In no way do I think we should be providing secret hidden support for snake_case properties, and have to implement a TS compatibility layer for it, which inherently WILL be documented in some sense because it will show up in Intellisense

Its also awful for code maintenance to have to be handling two different possible formats of input values to a constructor

I'm firmly against trying to support two different standards

[outer raven]

Then how would builders be supported in djs? Would it simply… not?

[ruby terrace]

I think builders should be for people using REST directly or stuff via outgoing webhooks

[copper laurel]

Yeah, I dont think it should be tbh

[outer raven]

Actually I have another idea

Why don’t you add a setting to all builders that lets you choose whether you want api or djs compatible objects?

[dawn merlin]

Smh I made a pr and it wasn’t accepted

[copper laurel]

I'd be okay with a way to define the version of output from the builder, yes

[outer raven]

Link?

Never saw that tbf

[copper laurel]

The builder is the third party. It should be made compatible with discord.js, not the other way around

[undone ravine]

why not just a method on the builder toAPIObject() or smth

[outer raven]

Yeah that’s totally fair

[ruby terrace]

yeah, that's not terrible

[dawn merlin]

https://github.com/discordjs/builders/pull/39

[outer raven]

And it’s the best of both worlds, since builders has already been used in guides with rest and djs

[outer raven]

That’s slightly different though, I’m suggesting a boolean at the very top of the builder that determines the types for everything going forward

[undone ravine]

that's a mess

[dawn merlin]

It’s not

[undone ravine]

internally

[outer raven]

Why?

[ruby terrace]

that's different

[undone ravine]

the method makes the most sense to me

[outer raven]

I can see both being doable

[undone ravine]

because builders would constantly have to do checks against multiple versions of the same property

[ruby terrace]

and the main issue there is the importing of types from djs tbh, when it should export its own types as it doesn't necessarily have to be used with djs

[dawn merlin]

Yeah I’m not for this

[copper laurel]

nah just in the toJSON()

It can store them however it likes internally, then toJSON checks which output setting was configured and spits it out accordingly

[outer raven]

Yeah that could work

[copper laurel]

That does make it harder to type the output though

[undone ravine]

that makes sense, works the same way i was suggesting a separate method basically

[copper laurel]

Yeah

Im fine with either

[outer raven]

Curious how @solemn oyster mentioned that djs should be made compatible with builders and not the other way around, when we’re talking about the exact opposite here

[undone ravine]

although i do think it makes more sense to have two separate methods for it

[ruby terrace]

separate method is more painful because you need to call it manually, but easier typings wise

[dawn merlin]

The Boolean method just feels like extra work, just be explicit about what you want with a method

[undone ravine]

if for some reason you wanted to be able to output both formats

[copper laurel]

If thats the case, then discord.js should be snake_case imo lol

[undone ravine]

a boolean switch would be annoying to toggle

[outer raven]

How did we get here

[undone ravine]

or maintain a separate builder instance

[copper laurel]

djs supporting builders / api types

as input

Which are snake_case

[undone ravine]

this right here

[outer raven]

Ah about kyra’s comment

Yeah I don’t think that’s the way to go tbf

[copper laurel]

Yeah im not saying I like that solution

[outer raven]

Djs is the main lib, all the other ones are @discordjs/*

[ruby terrace]

just a reminder that you almost never explicitly call .toJSON()

[dawn merlin]

It's certainly used in the guide for deploying commands

I don't use builders that often, but I don't recall just passing builder objects as-is to /rest or djs

[ruby terrace]

you should be able to just fine for either one given they call JSON.stringify() which calls .toJSON for you

[dawn merlin]

the issue I have with that is that .toJSON isn't spec compliant on builders, it's more like a unique method

realistically it should return a JSON-fied version of itself, not the snake case dapi compatible version

[ruby terrace]

spec says nothing about that, just

If the value has a toJSON() method, it's responsible to define what data will be serialized.

[dawn merlin]

Seems you're right

however for something to be implicitly validated by using JSON.stringify seems iffy to me

[wild flax]

We already have some compatibility for registering commands in djs with snake_case no?

[dawn merlin]

yeah, you can today

[wild flax]

Then we should expand that to the whole lib

And let camelCase take priority if both are supplied

[dawn merlin]

Does this mean have all methods that accept data to also accept the djs data, ie foo(data: FooData | APIFooData)?

[copper laurel]

I really feel like that's such a poor interface though, to be supporting two entirely different formats of input object

[wild flax]

Probably not for creation

Because those are private anyway, at least most of them

No point for a transformation there

[dawn merlin]

This would align with other feature requests that have been brought up around v14. However my main issue is the duplicated non-simple types for djs and dapi-types. It feels like everytime a new feature comes out one PR is made to add the feature, then another one is made to make it align to the way it's enforced in dapi types. Why? because the person making the PR has probably never even used dapi types

[copper laurel]

Why are we aligning to dapi types though

[dawn merlin]

Mainly for type-safety

[copper laurel]

But those arent discord.js types

Why are we aligning specifically to those types

Why is there this perceived need to be supporting snake_case (except where appropriate on discord.js outputs)

[dawn merlin]

What I mean by align is that we take the way they enforce props and apply it to djs, the naming is still the same

the autocomplete typing PR is an example of this

it keeps the djs type, but modifies it to have the same level of type-safety as the dapi version

[copper laurel]

See, I don't see this as duplicated. It's two different types for two different libraries

Maybe in this case it is because these types happen to all be one-word, at least that I can see, so there's no difference between snake and camel

I personally only see it as duplicated if we write it twice and then support both

[dawn merlin]

I'm on the other side of the spectrum, my rationale is that if we already have the types written out and defined in various different ways in dapi-types, and the only, only difference is that one is camelCase and one is in snake_case, then why not make a type that can interop between them both rather than writing (essentially) the same thing in camelCase. But I can respect your opinion that there's a need for the alternative of ditching dapi out of djs entirely

[wild flax]

ditching api-types will def create duplicate types then

because we use raw api return types already

[copper laurel]

I don't think we should ditch it entirely. It makes complete sense to use when the type in question is actually an API type. But yeah, I think writing a compatibility layer is the right solution to the wrong problem statement - using discord-api-types when we shouldnt actually be using them

[wild flax]

honestly, sometimes I would even want to allow Record<string, any>, so you could just supply anything and we don't have to explicitly "support" it, but you can work with it

[copper laurel]

I think it would be a pretty confusing look for discord.js's types to be Camelise<OtherLibType>

[dawn merlin]

even defining a type alias would be a major improvement export type ApplicationCommandData = Camelize<APICommand> and it wouldn't be viewable by intellisense

[copper laurel]

However if the requirement is to go ahead and support both formats, then APIType | Camelise<APIType> is probably the nicest way to achieve that, Id agree

yeah fair enough, using it to define the renamed types wouldn't be as bad

[dawn merlin]

I do disagree with API | DjsType mainly because it offloads all the work on the function to figure out how to process snake_case vs camelCase

[wild flax]

thats fine though

[copper laurel]

Why is that fine? Isn't that kinda awful for code maintenance to be doing shit like this.propName = data.propName ?? data.prop_name

if('propName' in data || 'prop_name' in data)

[wild flax]

not really and we already kinda do it

[copper laurel]

Yeah and I hate it

Almost makes me want a supporting utility function to actually convert the object too, not just the type

[wild flax]

sure

[copper laurel]

camelise(unknownInputData)

Then we know what the format is for the rest of the method

[dawn merlin]

the issue with the even a util method is that even if it's already camelCase/snakeCase it'll try to convert everything anyways because it'll most likely be used as a catch-all. If you pass in snake_case, does it get sent directly as snake_case, or does it get converted to camelCase and then back to snake case when it's sent?

[copper laurel]

¯_(ツ)_/¯

I'm just brainstorming

[wild flax]

again, this won't matter, and we do it already

it could also be the other way around

and snake_case everything and work with that data from then on

because like I said, I wouldn't accept camelCase for creating instances mostly anyway

its data that comes strictly from discord in that case

its more about methods, like the edit method on everything almost

[outer raven]

We already do this but it’s much more painful to maintain as you have to keep remembering what properties accept both types and then check for both when they do. Wouldn’t it be much easier to make builders compatible with djs instead of the other way around? Afaik builders is the only reason why we support snake_case

[tacit crypt]

Absolutely disagree on that part..

[ruby terrace]

If it were standalone, the djs shouldn't support them (I agree with that, think about other 3rd party packages), I think you're saying it shouldn't be standalone yes?

[tacit crypt]

I'm saying we should support it.

[ruby terrace]

so its not standalone

[tacit crypt]

It is

[copper laurel]

Why is it discord.js's responsibility to change to support a dependency that doesn't provide a djs compatible output?

It outputs snake_case

[wild flax]

because we make it so

and I honestly don't think this:

the issue with the even a util method is that even if it's already camelCase/snakeCase it'll try to convert everything anyways because it'll most likely be used as a catch-all. If you pass in snake_case, does it get sent directly as snake_case, or does it get converted to camelCase and then back to snake case when it's sent?
is an issue with a util function

we already KINDA do this

[copper laurel]

So much work was done in v13 to reduce inconsistencies and now you're talking about supporting two entirely different casings for inputs objects

[wild flax]

we get snake_case from the API, we transform it manually, and when we send it off we transform again

so it literally will make 0 difference if we do it everywhere

lol

[copper laurel]

A util function is kinda like the... least awful approach

[tacit crypt]

Yes, Monbrew, how does that matter? Builders, from the getgo, are a utility class to emit compatible api data. You're not forced to use it but you can. Hell, we could make our types take in the full builder instance and it'd make sense, no?

[copper laurel]

It makes a difference when it's something the user inputs, not just API data

[wild flax]

no even then it won't matter

[copper laurel]

If it takes the builder instance and can call a toCamelCaseJSON yeah I guess

[wild flax]

it doesnt matter if your IDE shows:

maxValue
max_value
minValue
min_value

as autocompletes for types

doesn't even hurt anyone

[copper laurel]

It's a fucking mess

[wild flax]

its not

[tacit crypt]

And tbf, if we go with the raw data proposal, we'll always have raw data somewhere so like where's the issue

[wild flax]

we dont write code to "please" whatever IDE output you get when you press 2 buttons

[copper laurel]

I don't mind moving to raw data, but I think half-assing it and continuing to support camelCase is pointless at that point. It serves no purpose other than an appeal to tradition / backwards compatibility

[wild flax]

what?

[visual hornet]

am i allowed to weigh in on this or...?

[wild flax]

no that makes no sense

we have to keep camelcasing

[copper laurel]

Why

[ruby terrace]

I think I'd rather have a dedicated method that deals with pre formatted api data, since we don't need to run checks or transform anything

[copper laurel]

Because it's JS standard or something?

[wild flax]

we can't just tell people to mainly do

Guild.edit({ system_message_channel: '123123123' });

yes

lmao

[visual hornet]

why not just support both?

[wild flax]

it would also mean internally all our shit is snake_cased

its literally abstracted away, you will never see it

[ruby terrace]

*and yes, we can pass the normal method formatted data to this

[wild flax]

all youll see is:

const opts = camelize(options);

thats it

[ruby terrace]

we'd need to fix our camel casing (which we mostly did) in a few places still though

[tacit crypt]

That's sortof what we're doing right now and that seems to be the core of this "issue"?

[copper laurel]

I still don't really understand why your okay with introducing user-facing inconsistencies in the data formats accepted after so much work was done to remove inconsistencies from the lib

[visual hornet]

i mean if data that doesn't fit is sent to the api, the api just ignores it right?
like if you send iconURL and icon_url together, wouldn't the former just be discarded

[copper laurel]

But I seem to be the minority opinion there lol

The former would probably be preferred there

At least that seems to be the general consensus, user format > API format

[visual hornet]

im thinking more of just providing support for both within the same methods, like having duplicate keys, one in snake and one in camel?
instead of seperating them as the discussion here seems to be about

[copper laurel]

That wouldn't be typesafe

[visual hornet]

or would that be too clunky

[ruby terrace]

that's the main pain point actually

type safety goes out the window as soon as we do that

[visual hornet]

aight well i have nothing to contribute other than that
good luck i guess

[ruby terrace]

we'd have to create unions of all the camel case stuff or all the snake case stuff

[copper laurel]

I just think foo(data: CamelCaseData | SnakeCaseData) is shit.

[visual hornet]

what about just appending the snake case stuff to the original camel case stuff though

instead of having a seperate type?

[wild flax]

in this case it will just be: APIMember | CamelcasePropertiesDeep<APIMember>

[copper laurel]

Regardless of how CamelCaseData is defined, utility type or otherwise

[wild flax]

but I guess

[copper laurel]

Everything would have to be optional, not typesafe at all

[visual hornet]

alright

[copper laurel]

Unless at the function signature level we did wrap both into a single union

[tacit crypt]

If your only issue is editors showing both camel and snake case.. first off why do you care so much, secondly that can be solved by overloading the function

[copper laurel]

type FooData = SnakeCaseFooData | Camelize<SnakeCaseFooData>

foo(data: FooData)

[copper laurel]

It's not just about how editors show it, that's just an example of a symptom of the inconsistencies

[tacit crypt]

I literally fail to see where the inconsistency is in us supporting both api data and camelCase that we process

[copper laurel]

You don't see why having two entirely different object formats for users to choose between is an inconsistency? Am I mistaken and this is a common thing for libraries to do?

Like I said it seems I'm in the minority here it seems so it doesn't matter anyway, looks like this is the approach we'll take

Also not a maintainer, I'm not meaning to overstate like I have some important vote

[tacit crypt]

Considering that between our documentations always showing camelCase, our utility functions like setXYZ, I don't see why implementing support for snake_cased data from utilities like builders is a bad thing 😕

[copper laurel]

So is the intention not to document that support?

[tacit crypt]

It's already not documented, no?

[copper laurel]

Or document it as supporting the Builder instance?

[tacit crypt]

Well, past application commands I guess

[copper laurel]

Well it's also not supported right now I thought, hence this discussion

[tacit crypt]

Its technically supported in one place, which is application commands

Which also happens to be the only structure that has a builder rn

[wild flax]

okay hear me out

we expose the camelize function

[copper laurel]

I feel like by offering snake_case support across the board, while probably a good thing, actually makes it the superior option because it's then better supported than camelCase across the rest of discord.js modules, like rest and builders

So if people want consistent objects in their code they're going to use snake_case

[wild flax]

and people could do:

Guild.edit({ ...camelize({ system_message_channel: '1231123123' }) })

[ruby terrace]

embeds have a builder

[tacit crypt]

That has an issue already tho, because api data takes the id not the "channel" like we do

[copper laurel]

Yeah which is somewhere we had to implement dual support because they're created by users and the API

re: embeds

[tacit crypt]

embeds also doesn't have fields that are affected by this issue

[ruby terrace]

don't they?

[copper laurel]

author.icon_url?

Stuff like that?

[tacit crypt]

I keep forgetting its not just proxy_url

[copper laurel]

The top level doesn't iirc, only nested

[ruby terrace]

yeah, footer, and author only

[dawn merlin]

This is probably the best compromise, the original reason I made the ticket was to remove duplicated type declarations, and this seems to address that. It also allows for raw data anyways. We could even generalize the type to a “APIResolvable<T>” where the type represents a union of the api type as well as the camel cased api type.

[outer raven]

I read the entire conversation and I actually like the solution that siris proposed just now in the comment but I don’t think I saw anyone explain why we couldn’t make builders compatible with djs instead of the other way around? Still seems like the most logical option to me so I’d like to know why that was discarded

[dawn merlin]

it doesn't matter anyways if Djs is moving to accepting dapi types or camelcased dapi types in the methods

If that happens there’s no need for another method in builders

Well, since ApplicationCommandManager already allows this, there’s really no point in it

[outer raven]

but in order to do that, all the code in all methods has to be duplicated to support both versions, whereas if builders supports it, we simply need to tell users to use that method instead

[dawn merlin]

#archive-library-discussion message

[outer raven]

but if we do it like it's done in the application command manager, we have to write duplicated code everywhere

unless we make a general function that converts all snake_case to camelCase

i saw that as well but thought it was for types only

[dawn merlin]

That’s what was suggested

[outer raven]

hmm ok

still would prefer that to be done on the other deps instead of the main lib, but that's me

[dawn merlin]

Fundamentally I don’t think builders should have to change for djs, it should be the other way around. After all, some of the major use cases of it don’t require djs at all

[outer raven]

but builders is called @discordjs/builders meaning it's a sub-dependency of discord.js. Discord.js doesn't depend on builders and neither does builders depend on djs, but if they need to be compatible then the sub dependency should be made compatible with the one that was already there to begin with

[dawn merlin]

I’m pretty sure the only reason it’s scoped by discordjs is because the npm package name “builders” was taken. I don’t think has to do with how the package is meant to be used.

[outer raven]

it's still the most recent one and it's the utility package that doesn't work by itself, so it should be compatible with all existing alternatives to it, and not the other way around

[dawn merlin]

It works without djs so why should it change to work with it? If it’s only usable with djs than that’s different, but that’s not the case

[outer raven]

but it doesn't work on its own because it doesn't interact with the api, so it should be made compatible with the two packages that do interact with it

[wild flax]

The scope means nothing. Same for collection. It just means it’s “from us” nothing more

discord-api-types would be scoped too if it wasn’t for vlad having it released before we moved it to us

[wild flax]

It kinda does, since you can just use it fine with the /rest package

Which doesn’t accept camelCase at all

[ornate topaz]

but then doesn't it just produce output for the /rest?

similarly to how you can use its output in d.js?

[wild flax]

Not really

You can use it with anything

/rest is just a glorified http wrapper I guess

So i should have said you can use it with any fetch stuff

[ornate topaz]

well, my point is that builders doens't have /rest integrated, doesn't it

[wild flax]

No because it’s agnostic

[ornate topaz]

it's just a builder, you throw stuff at it, take whatever it spits out and use whenever

[wild flax]

Yup

[ornate topaz]

so you could justify more making builders be able to spit out different formats than adapt d.js to use builders output

[wild flax]

Yeah but meh

I’d rather adjust djs

It’s just a Util function

And the user has to use it themselves

[wild flax]

Like this

[ruby terrace]

oh, you want to just give users a function to camelize (that we'll probably use ourselves for received data occasionally), not modify the actual methods. I like that

[wild flax]

Yeah

[outer raven]

that's what I'm saying: you need another package to do something with the builders output, and builders should be compatible with all the available packages. It already is fully compatible with rest, so it could be made compatible with discord.js way more easily than discord.js can be made compatible with builders IMO

[wild flax]

Disagree

I like to compare it to voice

Voice doesn’t have the djs adapter

Djs has the voice adapter for itself

[outer raven]

but that's because you need information that is stored in djs in order to create the adapter right?

[wild flax]

You could just pass that

[outer raven]

I mean it sort of relies on the shard status to be safer, but in the case of voice, it's only one function and one manager that was created once and never had to be touched again, whereas for this builders stuff, from what I understand, we'd have to build in support for it on every method we make going forward

[ornate topaz]

not if you make a converter for builders output in the djs

assuming it's going to be even a tiny bit less hardcoded than fully hardcoded, it shouldn't need updating every single time, i'd imagine

[outer raven]

you'd still have to run the function on every method, unless you tell users to run the function themselves, which I'd prefer

so we wouldn't support snake_case, but we'd let people easily convert

[ruby terrace]

yeah, that's what changed my mind

users do it, not lib

we just offer the means

[outer raven]

Yeah that sounds better then, didn't notice that part mb

would this be considered semver major?

[wild flax]

Nope

That’s the cool part

[outer raven]

I mean it's technically breaking since it was documented

[wild flax]

Only for slash commands no?

This applies to everything

[outer raven]

Yeah but since it applies to everything it would break snake_case support for slash commands, unless you keep that and remove later on in v14 which could also work ofc

[dawn merlin]

I'm confused now, are we still leaning towards APIObject | Camelized<APIObject> + camelize(object) or just camelize(object)?

[wild flax]

only Camelized<APIObject> + camelize(object)

[dawn merlin]

ok sweet

[copper laurel]

a camelize function is good

I do like that

actually I think it was originally my idea, but making the users do it as a middleware is 👍

Allows discord.js to have a standard interface convention while providing support for raw API data

[opaque vessel]

@dawn merlin pokes is it just me or an interaction's guild id is no longer nullable via TypeScript?
(mentioning you cause you're like the typings guy at this point lol)

import { Client, Constants } from "discord.js";

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

client.on(Constants.Events.INTERACTION_CREATE, interaction => {
  const a = interaction.guildId;
  a.slice(1); // Expecting an error, but there was none
});

(And yes I'm running strict mode)

[dawn merlin]

Uhh looks like a bug to me

[opaque vessel]

Just to confirm, are you able to reproduce it?

[dawn merlin]

yeah

[opaque vessel]

Rip

I'll file a issue for it, thank you for confirming

[outer raven]

Why not fix it straight away

[copper laurel]

People dont always know how to correctly fix the issues they find

Idk if Jiralite does or doesnt, but in general it can't be expected all the time. Thats why issues exist

[dawn merlin]

thank you for this, turns out the bug was affecting a bunch of different areas because of a small mistake in CacheTypeReducer

[opaque vessel]

Hot

[wild flax]

@dawn merlin no offense, but you should probably, really wait for real maintainer feedback on things like deps and all 😅

I'm not a huge fan of copying types from another package just to "save" a dep

Just because someone said so

[dawn merlin]

Ok sorry about that, I’ll try not to do that from now on

[wild flax]

Your arguments were solid, and I don't see why we should concern ourselves with any of that

It just makes our types even more complex, and reliant on us updating the definition of it too

[dawn merlin]

Understood, and completely agree

[wild flax]

I thought using type-fest is a good idea (thats the sindre package right?)

[dawn merlin]

Yeah

[wild flax]

Since it also potentially provides us with other utilities, should we need it, instead of us coming up with those too

[outer raven]

Well then it needs to be a dep not a dev dep

I just felt like using a package for one of its features didn’t make much sense but thats fine

[tired valve]

what you feel is not relevant, maybe don't superimpose that much in the future

[outer raven]

I didn’t impose anything, I literally asked if we really needed the dep and he just went ahead and removed it. I suggest removing it, didn’t force anyone to do anything

Afaik suggestions are still welcome

[dawn merlin]

If it’s a dev dep then these types won’t even work at all because the package wouldn’t get installed. We can simply copy the type from them and add it ourselves

this is far from a suggestion, it's a "it needs to be done this way"

[outer raven]

that's an explanation to my opinion. The only "it has to be done this way" part in that sentence is that it has to be a dep and not a dev dep if you do wanna add it, that's all

[outer raven]

Is it a bug that we cache ephemeral messages? Was trying to edit the first message on a channel's cache and I kept getting errors because it was ephemeral

[opaque vessel]

Hihi @dawn merlin, if I'm not mistaken, I think https://github.com/discordjs/discord.js/pull/6965 is opened at https://github.com/discordjs/discord.js/pull/6949 already?

[dawn merlin]

you are correct!

[opaque vessel]

(:

[outer raven]

The docs say that GuildMember#user is nullable but the typings say it isn't, which one is correct?

[dawn merlin]

The field user won't be included in the member object attached to MESSAGE_CREATE and MESSAGE_UPDATE gateway events.

[outer raven]

so the typings should be updated to make it nullable only on those events right

[dawn merlin]

i suppose so

unless it's guaranteed to be cached

[slate nacelle]

Those events have an author.

It's probably nullable in the docs due to being a getter or something. -> Can only be null if you mess with cache.

[outer raven]

it's not a getter

[slate nacelle]

From this PR: https://github.com/discordjs/discord.js/pull/6066
I have no idea why this was changed.
Was partially reverted in https://github.com/discordjs/discord.js/pull/6840

So good to revert to non-nullable I guess.

[outer raven]

why was this done too if content is never null?

[opaque vessel]

Partial message content will be

[outer raven]

when do u get partial message content?

[slate nacelle]

Through a message update for an uncached message.

[outer raven]

ah aight

then ill PR the fix

I mean, should the whole user property be nullable on every scenario?

If not then I don't know how to only make it nullable for those events

[dawn merlin]

can't you change the type returned by those events?

[outer raven]

the type is message, idk how I'd change the type of the user property in the member object inside the message

[dawn merlin]

Can't you extend the message ommitting the user property and redeclare the property in the new type?

interface AnotherMessage extends Omit<Message, 'user'> {
  user: ...
}

[outer raven]

it's not the message, it's the message's member

[dawn merlin]

yeah but doesn't the event return a message type?

[outer raven]

yea but you need to change the type of the user property in the message#member

message doesn't have a user

[dawn merlin]

interface AnotherMessage extends Omit<Message, 'member'> {
  member: Message['member'] & { user: User };
}

Maybe this then?

[outer raven]

well if you were to use that on those 2 events then it'd be User | null, but yeah

[dawn merlin]

wait but if Message.member.user's type is User | null and you intersect it with User, then the resulting type should be User

[outer raven]

no but the type of GuildMember#user should be User, except on those two events where it can also be null

[dawn merlin]

oh so I had it backwards

[outer raven]

gonna make the PR?

[dawn merlin]

? I thought you were gonna do it lol

[outer raven]

you made the type i thought you wanted to kek

[outer night]

you both are free to make PRs for the same issue, and if both of you go for it maintainers will just choose the one that made better implementation decisions over the other

[dawn merlin]

I thought it was first come first serve, I’ve seen situations where one pr is denied solely on the fact one already existed?

[copper laurel]

If the implementation is the same yeah, first come

If someone tries to PR snipe with garbage, they'll probably just keep the better one

[wild flax]

I was looking at some implementations for the aforementioned toCamel feature

I was thinking it might be worth it to bring a dep into this, instead of maintaining a somewhat complicated code + typings ourselves

The one that mostly sparked my eye was: https://github.com/RossWilliams/ts-case-convert/blob/main/src/caseConvert.ts

it covers both of our cases, converting back and forth while also having test coverage

The other option I found was a rather popular package like: https://www.npmjs.com/package/change-case but based on GH issues its been missing a proper release for over a year now

I'm just not a fan of bringing in more potential burden into the lib that technically shouldn't even concern us

[dawn merlin]

One thing I was looking at was the highly popular humps.js library, as it stands right now its typings outputs an “any” type for its camelizing function. I’m most likely going to make a PR to its DT repo to use the type-fest types as a return type

[wild flax]

I mean we don’t need too big of a lib

[dawn merlin]

In terms of package size humps is actually the smallest compared to ts-case-convert and change-case

[wild flax]

Ah interesting

[dawn merlin]

So CamelCasedPropertiesDeep doesn't work with mixed case types which humps accepts, for our use cases we'll never have to worry about that anyways, imo we should use whatever lib we want + sindre's types and then do a module augmentation ourselves on the lib's return types.

[wild flax]

Does ts-Case-convert not expose its types?

I can’t check rn

But then we could just use them too I guess?

[dawn merlin]

Yeah it does we can just use that lib directly

[wild flax]

Sweet

That’d be best case scenario then

And if there’s issues, we could just send a pr or 2 to them too

Since it’s a very specific lib too, so it’s not like we need to hammer through some bureaucracy because it does too much

I’m sure we’ll get someone who tries to copy the dep and make a pr and say “one less dep, here you go” but we can just deny those, wouldn’t be the first time

Lol

I just don’t wanna maintain more code that doesn’t really concern us or rather shouldn’t be maintained by us unnecessarily

[outer raven]

What would you need to maintain?

If it works atm there’s no reason for it to break later on

[wild flax]

It’s just not beneficial

Look at the code I posted above from the ts lib

Shoving all that into a Util file and let it rot there

At this point you might as well just install the dep lol

[dawn merlin]

It’s not just that it “won’t” break, it also needs to be updated when any new ts type features come out. The TS devs have shown interest in expanding the template literal types

[outer raven]

Yeah the one you linked seems fine since it doesn’t case converting only, but the one siris suggested doesnt seem right imo because we’re only using a very small portion of it, and some people, including myself, really care about package sizes

[wild flax]

but why?

[outer raven]

But that’d only need to be done on a major release, which would probably come with other breaking changes

[wild flax]

Yeah but it shouldn't be our priority or task to upkeep such a util function as a "discord" lib

if theres bigger issues at hand in that case

[outer raven]

My bot is currently 430mb ish because of dependencies mainly and it cannot cross the 500mb because of the host I use. Crossing that would mean having to find a new one which would be a pain. This is my case tho, idk about others

[wild flax]

it just fragments our focus

You could look into bundling?

tsup for example could very well be used to bundle your node_modules too into a single minified file with a sourcemap

so you also get proper line counts in the source files in case an error occurs

[outer raven]

I could look into that but most of that size is a dependency that is on the host only which is needed for a specific package (puppeteer) to work

[dawn merlin]

The lib is like 2kb

[outer raven]

Anyway that’s not really the topic of this channel, point is that yeah that could reduce size but it wouldn’t be a lot and I think it’s still possible to avoid deps when we can

[outer raven]

mb*

[dawn merlin]

No the camelcase lib

The size difference is negligible

[outer raven]

I’m talking about type-fest which is 155kb (ik, not a lot, but still)

[dawn merlin]

What is the djs’s size?

[wild flax]

I would really like you look into bundling your application honestly

you can ignore puppeteer, but youd drastically reduce the size of your application

I can maybe write up a guide for the guide page for this

But we really shouldn't be "too" careful here

[outer raven]

I'd appreciate that, but I'll look into tsup like you suggested

[wild flax]

I can maybe also look into bundling d.js itself, so our sourcefiles are minified

since they can take up quite some space

[dawn merlin]

I agree it’s a server module not a browser module

[wild flax]

i wouldnt bundle node_modules though

since as a lib that is a bit bad

[outer raven]

wouldn't that make it harder to navigate? Also there were some issues with using fs recently I didn't really look into those too much I just saw a PR about it

[wild flax]

Harder to navigate how?

Nah it was injecting an "import" into a cjs file, nothing to do with fs or path

[tacit crypt]

Well duh, it comes bundled with an instance of chromium..

[outer raven]

like when we get errors that link to a djs line instead of an error message, I usually open it to see where it came from and then check other files, if it's all in 1 it could be harder to do this

[tacit crypt]

Please no.. it's something I dislike heavily when we did in sapphire, it makes exploring the code locally a nightmare without opening up the repository

[outer raven]

^^ this

[outer raven]

yep that's the issue kek

[wild flax]

well that shouldn't be a main concern, sorry lol

[tacit crypt]

The stack traces aren't an issue, source maps deal with that

[dawn merlin]

I disagree with bundling stuff built for servers

[wild flax]

if im on a server env, I don't go looking into my node_modules to fix something

[tacit crypt]

But like our lib is pretty small either way so I don't see how we'd benefit from minifying it further

[wild flax]

hmmm, 1.19mb unpacked isn't that small

lets be real

when it could be 500kb

[outer raven]

what if you're in a dev env

[tacit crypt]

Don't bundlers minify our source either way? You want small, use a bundler?

[wild flax]

Sure, that'd be on the user then

[tacit crypt]

I don't see why we should minify our source, I'm sorry

[dawn merlin]

But why does this matter on the server? For websites that’s a concerning number, but 1.19 should be palatable

[wild flax]

I mean at the end of the day, once we switch to TS, we will bundle and minify anyway...

[tacit crypt]

We're what.

[wild flax]

So I am not sure how keeping that equilibrium is important

[tacit crypt]

Why are we minifying .w.

[visual hornet]

well didn't voice already get minified

[wild flax]

Well the output .mjs and .cjs gives is a lot of crap anyway

So traversing that is already painful

Even without minification

[wild flax]

Yeah our voice lib is only around 450kb

[tacit crypt]

Also don't forget our code does fs calls now so careful with minifications and bundling..

[wild flax]

Well that will obviously change

[outer raven]

can it change easily?

without hardcoding all exports again

[wild flax]

nothing wrong with hardcoding them

[outer raven]

its a pain to maintain

[wild flax]

we switched to the fs approach because at the time I thought it was a somewhat good idea, but I don't really think so anymore honestly

since we had to introduce this webpack hack

its ok for now

[tacit crypt]

I mean yeah but they're also touched once a century soo

[wild flax]

but I would rather not keep it for the long term

[outer raven]

actions yeah, but I've personally always thought we should do this for index.js

[wild flax]

no

esp not in the future

we won't be exposing a lot of private stuff

[tacit crypt]

I still strongly dislike the idea that we'll minify/bundle our sources in the future... There's like..no reason to outside of this size..

[dawn merlin]

Quick question, are we going to use deno for the TS rewrite with npm exports?

[wild flax]

deno*?

[dawn merlin]

Yeah

[wild flax]

Not sure I want to commit to deno and node at the same time

[tacit crypt]

I doubt our lib will be deno compatible?

[wild flax]

Deno usage is very low

would also mean refactoring -modules, /voice and /collection

Thats... a lot of work lol

[dawn merlin]

Tru

[tacit crypt]

I mean for -types it was dumb easy to do, but for d.js.. the path changes ALONE would be a chore, so I doubt it'll happen soon

I mean collections should be deno compatible tbh

I don't think we'd need to change anything?

[outer raven]

you can add those to an array, it's much easier to specify the ones you don't want than the ones you do

[wild flax]

I'd rather use a multi index.ts approach

and call export * from 'x/'

so youd only need to add the export to the specific sub-index.ts

and it will expose itself

[outer raven]

yeah but you still gotta go there and manually add the line

[wild flax]

thats fine

[outer raven]

whereas with an fs read you simply don't have to worry about that

[wild flax]

yeah but I don't see the benefit here

How often are you gonna add new classes/files and have to add an export

[outer raven]

every time a new discord feature comes out

[dawn merlin]

Isn’t that what most TS libs do tho

They don’t expose everything by default

[wild flax]

yeah, using fs to export your files is quite the anti-pattern

[outer raven]

I'm not sure what most TS libs do tbh, but we're still js and im proposing this for the js lib still

[wild flax]

And I’m not comfortable enough to pioneer a different way here lol

[dawn merlin]

I always see the reexport pattern used in ts libs

[wild flax]

Relying on IO shouldn’t be something we do in the lib

Just my 2 cents

Save those cpu cycles

[tacit crypt]

I'm not comfortable reading all our directories to do an index.js/ts/mjs/tjs/whatever file either

[dawn merlin]

Like this https://github.com/discordjs/discord-api-types/blob/main/v9.ts

[outer raven]

but on discord-api-types you have all the v9 stuff in 1 file, so you only export 1 file

[dawn merlin]

That would be the same for nested index.ts’s they reexport and then the root index.ts re-exports them again

[outer raven]

but in djs you have multiple files so even if you have multiple indexes, in the sub indexes you'd have to be require()'ing all of them

[dawn merlin]

Ideally every directory has an index.ts, all you would need to do in the root index.ts file would be reexporting each directory (on the same level).

[outer raven]

yeah that's fine, but it doesn't solve the issue, just spreads it across multiple files

"issue" imo ofc

[wild flax]

I honestly don't wanna get too hung up on this whole thing

I am pretty dead set on this

Sorry

[tacit crypt]

We're not using fs to generate an index.anything file

[dawn merlin]

Reexports?

[wild flax]

Multi-index file approach

[dawn merlin]

Ah ok

[outer raven]

and using fs on index?

[wild flax]

Nope

No fs in the lib

[dawn merlin]

What if there’s files/symbols you don’t want to include in your exports?

[outer raven]

you put them in an array, like was done for actions

was at some point, idk if it still is

yeah literally https://github.com/discordjs/discord.js/blob/main/src/util/ModuleImporter.js

[wild flax]

Yeah I def wanna move away from that

[outer raven]

:/ aight

[slate nacelle]

I'm +1 on that

[dawn merlin]

I wasn’t aware of this, but this is … gross

[wild flax]

tip: don't look at it

helps me through the day

[outer raven]

was added in https://github.com/discordjs/discord.js/pull/6709 but why? And why was it merged if it's so bad

[wild flax]

Because we I merged the fs pr, which I shouldn't have in hindsight

but 20/20

If anything the less complex code #6102 made things a lot more complex

But hey, I was young and dumb

[outer raven]

_how is it complex tho _

[dawn merlin]

Live and learn

[strange igloo]

people often think less lines = faster/cleaner/better/etc

[wild flax]

I'm saying because of that PR, we had to introduce a more complex method for webpack, since we broke support for it

[visual hornet]

golfing in a nutshell

[wild flax]

So "less complex" ended up being "way more complex"

[outer raven]

but wasn't support for webpack removed in v12?

[wild flax]

only web builds were removed

not using webpack on the end-user

[outer raven]

I see

[dawn merlin]

So like the dom lib essentially?

[wild flax]

No no, we had specific builds so you can run your bot on a website lol

[dawn merlin]

Hmm ok

[wild flax]

Or the chrome console or whatever

But since discord doesnt support that anymore, we removed those builds

Doesn't mean we should break bundling overall

[solemn oyster]

@tender field are you able to continue working on https://github.com/discordjs/discord.js/pull/6647? You have requested changes from Space

[slate nacelle]

I think that's the wrong PR

[solemn oyster]

Fixed 😅

[wild flax]

can you rebase this @dawn merlin https://github.com/discordjs/discord.js/pull/6950

[dawn merlin]

Will do

[outer raven]

and probably 6959?

oh i gotta do some too

[dawn merlin]

It’d be amazing if GitHub notified you whenever your pr has new merge conflicts

[outer raven]

you can use an extension

it doesn't notify you, but it shows you on the pr list

for example this has a merge conflict and it has the icon

[tender field]

yes i'll do it asap! thanks for reminding me, i forgot about it 😅

[tender field]

uh-oh i think i failed my rebase

can some gitmaster help me fix my rebase fail here please ? https://github.com/discordjs/discord.js/pull/6647

[wild flax]

that looks rather messy

you might be better off just remaking it 😅

[tender field]

ah shoot 😅 well thanks, i'll redo it.

[wild flax]

@opaque vessel your PR will be the last blocker for a 13.4 release 👍

so whenever you get to updating it with the feedback, we can move forward with that

[outer raven]

Could try if you invite me to the repo

[tender field]

invitation sent ✅ thanks for your time!

[outer raven]

Aight will take a look

[tender field]

thanks ! i'll be back in half an hour fyi

[outer raven]

Fixed ^^

[opaque vessel]

Ooh okay, just done that - allowed null to be passed which'll clear the author object (:

Also @tacit crypt, I removed the default value on the first parameter (which was an empty object). This shouldn't be optional because calling .setAuthor() with no parameters will yield an error when undefined is checked to be a non-empty string.

That should be all

[burnt cradle]

Is the plan to port stuff like MessageButton and MessageSelectMenu to builders?

And if so, how far does that extend? Would it include something like MessageAttachment?

[copper laurel]

I did start a Button PR but I didnt like the design and never went back to it

Thing is, components basically already are builders in discord.js

[burnt cradle]

Yeah that's why I'm asking

[copper laurel]

I dont really see a need for Attachments to be one

Has like 3 properties

[tender field]

i pushed commits that added tests and resolved Space's comment, ready for review 😄

[outer raven]

https://github.com/discordjs/discord.js/pull/6887/files#r736720877 does anyone have a better suggestion for this description? @opaque vessel I know you usually like docs stuff so maybe you have an idea?

tbh I think it's good as it is. It mentions the API and client names just like we do in other properties

in fact, maybe it should only mention the client name (since the api name can be deduced by the property name) which has also been done on others like GuildMember#premiumSince

ops @tacit crypt?

[lethal storm]

Hey library people. Please let me know if this is intended:

With the bot having the "View" permissions in channel 1, I am able to access interaction.channel (as a ThreadChannel) when a command is used in channel 2 (the thread).

If the bot is denied the "View" permission is channel 1 and a command is used in channel 2 (the thread), interaction.channel returns null. The <interaction> itself is a basic object format.

interaction.channel is always available in channel 1. Permission doesn't matter. Is there a reason why the thread acts differently?

[dawn merlin]

what method are you using to check viewability?

[lethal storm]

No checks in code currently if that's what you mean

But Slash commands don't care about channel perms, no?

It seems to be cache-related. I can get it it work, but then if I then deny all perms and restart the bot, the interaction.channel returns null again

This is all beyond my skill level, but might it be because the bot isn't actually inside the thread?

[opaque vessel]

In the client, this is the description. So, maybe this:

Whether this guild has its premium (Boost) progress bar enabled

Just one word in a bracket extra

[outer raven]

with a lower case b but yes that sounds great, will change

[opaque vessel]

I don't think you need to state its whereabouts in the description

At Discord we tend to capitalise the word Boost everywhere

Because it's a part of "Server Boost"

[outer raven]

not really

and the rest of the sentence is uncapitalized so it doesn't matter

[opaque vessel]

When I said "At Discord" I didn't mean discord.js, I mean literally at Discord

[outer raven]

well but we follow the d.js standard

[opaque vessel]

True, though there's my suggestion nonetheless (:

[outer raven]

yep, committed :D

[lethal storm]

Not sure if you saw this @dawn merlin

[dawn merlin]

yeah I did, haven't fully looked into it yet

[lethal storm]

Alright

To my un-smart self, it seems like weird behaviour

[real jetty]

djs-modules/core will be using djs-modules/rest and djs-modules/ws instead of internally implementing it for the most part, right?

[wild flax]

core is a module itself

discord.js will end up using those

[real jetty]

wait, what will core do? I assumed it was going to be the djs ts rewrite

[wild flax]

core just houses functions or classes that multiple modules use

[real jetty]

Ah i see

[tender surge]

Hi, do you think it would be possible to add some type hint of the expected interaction type in command data interfaces?

export interface UserApplicationCommandData extends BaseApplicationCommandData {
  type: 'USER' | ApplicationCommandTypes.USER;
  __interaction: ContextMenuInteraction; // Like this
}

The issue I'm running in to is that the union type causes issues when attempting to extract the correct properties because you can't get the properties by only referencing either value of the union, and providing both tends to become verbose, after some workaround I managed to reference either union member to get the correct ApplicationCommandData properties but now I essentially have no way to statically determine what interaction type I'm dealing with based on the inferred type

[dawn merlin]

Hi, are you able provide a code sample of this issue? I'm just having a hard time trying to visualize it

[tender surge]

Well simply said, it would be nice to be able to do execute(interaction: ApplicationCommandData[T]["__interaction"])

Where T is where I reference ApplicationCommandData["type"]

[dawn merlin]

so bascially in your command handler you want the specific interaction type to be inferred?

[tender surge]

Exactly

Currently it's not possible because ContextMenuInteraction is completely separated

[dawn merlin]

I can share some code I use in mine to get it to work

[tender surge]

I'm scared now but I'm interested 😅

[dawn merlin]

export type CommandRunner<T extends Interaction> = ({ interaction }: { interaction: T; }) => Promise<void> | void;

export type CommandBase<T extends BaseCommandInteraction> = ApplicationCommandData & {
    run: CommandRunner<T>;
};

export type ContextMenuCommand = CommandBase<ContextMenuInteraction<'cached'>> & (UserApplicationCommandData | MessageApplicationCommandData);

export type SlashCommand = CommandBase<CommandInteraction<'cached'>> & ChatInputApplicationCommandData;

export type Command = ContextMenuCommand | SlashCommand;


// Usage:
const command: Command = {
  type: 'USER',
  async run({ interaction }) {
    interaction // Inferred to `ContextMenuInteraction`
  }
}

basically the command kind is inferred based on the type passed into Command

[tender surge]

Why is ContextMenuInteraction different from the others? Oh wait I see now

This is what I managed to create

import type { ApplicationCommandData } from "discord.js"

interface LegacyCommandData {
  type: "LEGACY"
  name: string
  description: string
}

type InteractionCommandData = ApplicationCommandData | LegacyCommandData

type InteractionCommandProps<T extends InteractionCommandData["type"]> =
  InteractionCommandData extends infer U
    ? U extends unknown
      ? T extends InteractionCommandData["type"]
        ? U
        : never
      : never
    : never

type Command<T extends InteractionCommandData["type"]> =
  InteractionCommandProps<T>
```In this case it would be solved with `InteractionCommandProps<T> & Record<"execute", (interaction: InteractionCommandProps<T>["__interaction"])>`

[dawn merlin]

You don't need a conditional type to get this to work. A union of ChatInputCommandData | UserApplicationCommandData | MessageApplicationCommandData is a discriminated union. It's discriminated on the type prop. As as long as the type prop is provided typescript can figure out which constituent to use. So adding a branded type to the interface wouldn't add much value.

[tender surge]

I see, ty for the example, honestly the guide screams for a TS version now but that's more of a #archive-guide-discussion thing 😅

It's too hard to navigate the types

[dawn merlin]

I agree lol, someday

[real jetty]

RESTManager class does not have JSDoc comments?

[outer raven]

It doesn’t indeed

[dawn merlin]

This is intentional

[real jetty]

I see

[oak quail]

this is intentionally how Discord works, it isn't a library thing

Discord gives users data for all non-thread channels in the guild, but thread data is locked to users who can access the parent channel (if the thread is private, membership in the thread or the Manage Threads permission is also needed)

the only channel data Discord currently sends in interactions is the channel id (which you can access in d.js with interaction.channelId); interaction.channel is loaded from cache

[real jetty]

Is there an official place on djs's github to work on the ts rewrite? Can't tell if im missing something

[dawn merlin]

Well it hasn’t started so not really, but the modules it uses will be under -modules

[real jetty]

ah, alright. thank ya

And is the rest module as powerful as the one djs is currently using? Like does it respect ratelimits everywhere it can and such like djs does?

[dawn merlin]

Yeah it keeps track of bucket hashes so it can keep track of rate limits, it’s almost if not as powerful as the djs rest client

[real jetty]

Alright sounds good, thank ya once again!

[lethal storm]

Thanks, I guess that makes sense

[outer raven]

Well, since the only open PR on collection was closed, can we get a release

[opaque vessel]

https://github.com/discordjs/discord.js/pull/6968's upstream pull request was intentionally closed

[visual hornet]

Why does Util.discordSort sort by id if the rawPosition is the same?
It seems to maybe have some issues, as channels with the same rawPosition have the position in the wrong order
2 channels with the same rawPosition of 0 will have the first channel's position as 1 then 0, for example

the rawPosition is linear with some repeats, but where the rawPosition has repeats, the position goes backwards

rawPos 0 0 1 2 3 3 4 4 5 6
pos    1 0 2 3 5 4 7 6 8 9

[outer raven]

Positions are quite a problematic thing so I think that was done as a way to be able to tell them apart. Since Discord doesn’t send the proper position fields, it’s hard to know where stuff is right

[visual hornet]

well it's consistently wrong when the rawPosition is the same, so maybe the a and b could be swapped when checking ids to make it correct?

[outer raven]

that would fix your use case specifically, but break others where that position is correct

I don't think you can have a 100% success rate

[visual hornet]

with the other scenarios where the rawPosition is incorrect and the position corrects it, what's going on there?
the channels i have here are from cloning

which makes the new channel higher up since it's replacing the position of the cloned channel

[outer raven]

when you create a channel, discord messes up positions and sets two channels to the same pos, which is why there's rawPosition and position, so just like you had a scenario where the order was reversed, others might have a scenario where it is correct

[visual hornet]

pain

[wild flax]

I think threads are incorrect either way

They sort alphabetically I think in the client

Which is an accident

[visual hornet]

so, it's an issue upstream that's attempted to be fixed with the position getter?

[wild flax]

Our discord sort should be exactly what the client does

[visual hornet]

well, it clearly isn't, but i guess that's not really an option

[wild flax]

You mean it isn’t for channels either?

[visual hornet]

nope, this is the result im getting

[visual hornet]

the repeated rawPos seems to sort by id ascending, but the discordSort method actually seems to sorts descending
@outer raven can you reproduce a scenario where rawPosition is the same and the position is correct between 2 channels? i can't seem to get any cases where it's correct with create either

[outer raven]

I can try but it's really unreliable

what type of channels are you working with?

[visual hornet]

TextChannels
GUILD_TEXT

[wanton forge]

Is the <ClientUser>.setPresence() thing buggy for activities? Because there were multiple people in #djs-help-v14 now already whos code didnt work with it, but it worked with <ClientUser>.setActivity()

[wild flax]

@trim quartz can u just copy the client code for positions on channels and threads and give it to us

[outer raven]

setActivity calls setPresence so there's no reason for it not to work

[wanton forge]

well, why wouldnt it work then? I have no idea tbh because it works with the same code for my bots but it seems not to work for theirs (and setActivity works for all of us)

[outer raven]

could be an issue in their code, I'm not sure
do you have a reproducible code sample?

[wanton forge]

no.. unfortunately not.. i was just wondering because their exact code worked for me..

[vernal adder]

Can possibly be unknown rate limits if they set at start-up

[visual hornet]

well i'll just draft a pull request for now, if there's more inconsistencies then... idrk
and im not touching threads, they don't seem to even have a position

[outer raven]

Does the api not give a position for threads?

[tacit crypt]

iirc no because threads aren't always present

[wispy ingot]

Hey, (not sure if its good channel, sorry if its not), does anybody know if the new Discord event scheduling feature will be included into the Discord API? And following question is - if it will be, will it be implemented to discord.js library?

[visual hornet]

the first one isn't really a question to djs, but if it's added to the api it'll be added to djs
it probably will be added though

[real jetty]

for discord api implementation you should see in the discord api server, #useful-servers
Afterwards, anything open to bots does end up on djs, someone just needs to make the PR

[loud jay]

#6493 in discordjs/discord.js by iShibi created <t:1629573546:R> (review required)
feat: add support for GuildEvent
📥 npm i iShibi/discord.js#feat-guild-event

[wispy ingot]

okay, thank you for the info, have a nice day

[frigid sierra]

Will discord.js include a thing for server events? Like create, edit, delete, etc...

[remote wasp]

yes

[visual hornet]

was just discussed lol

[outer raven]

discord.js supports everything the API does, so those questions always have an obvious answer.
Thing is the API doesn’t currently support events yet, so we’ll have to wait until next week

[outer raven]

@real jetty

that's what I mean by "the API doesn't currently support events yet"

[real jetty]

Ok? I'm aware as stated at the start of this conversation. Not sure why it's being repeated

[outer raven]

then why the reaction

[tacit crypt]

Misleading phrasing

[outer raven]

not really

you can't use events with bots

and self-botting isn't allowed

[tacit crypt]

Ok, then let me be clearer. Your phrasing was terrible

The API does support it but bots cannot use it

[outer raven]

yeah so the part that concerns discord.js supporting events isn't supported yet

[real jetty]

is mostly because "why repeat things already said to just be less precise"

[copper laurel]

Isn't this exactly why it is, in fact, not an "obvious answer" as you put it? Because the API endpoints for this feature are not yet available to bots, thus it's a Discord feature discord.js is not yet able to support

[tacit crypt]

wh

[dawn merlin]

@opaque vessel for issue #6905, could ThreadChannel#parentId be used?

[opaque vessel]

Sorry, can you elaborate?

[dawn merlin]

On this part of the issue ticket:

However, this condition is always false. this.channelId is the id of a ThreadChannel, but the deleted channel is a GuildChannel. Therefore, the interaction collector is not stopped.

Couldn't you use the parent id against the deleted guild channel id?

[opaque vessel]

I'm kind of lost sorry

The parent id of the deleted guild channel would always be a category channel (if one), and this.channelId is the id of a thread channel

[dawn merlin]

oh I see

nvm that then

[opaque vessel]

This issue could be partially resolved if one were to not store ids and instead store the structures. But, that would remove the ability to use the interaction collector on guilds the client is not in

[dawn merlin]

another alternative would be to find a way to directly associate collectors with their respective channels, some kind of tree-like structure. When a parent node is eliminated it's children should be as well

I think the infrastructure issue has to deal with the fact that threads are more than one layer deep

[opaque vessel]

The thing is, both solutions will not work in guilds the client is not in. The client won't detect if the thread is deleted or if the parent channel is deleted

There will always be a possible leak because discord.js supports those types of guilds for the collectors

The only real fix is to require a time, then it won't be discord.js's fault (':

Then comes the handling of the thread channel shenanigans

[dawn merlin]

so this leak affects collectors in guild channels too?

[opaque vessel]

If you mean a discord.js structure, then currently, yes. But that's fixable

It's just not fixable for guilds the client isn't in

[dawn merlin]

ah ok

[opaque vessel]

So you can see the real pressing issue here >:

[dawn merlin]

hmm maybe it's worth it to throw an error if you're not in the guild and create limitless collector

after all collectors are djs constructs

[opaque vessel]

How would you detect that in the typings o,o

[dawn merlin]

I mean you wouldn't, lol, it's possible but painful

[opaque vessel]

Ya, but this specific collector is alienated from the others due to its unique ability to work on fancy guilds

I think the/a fix would ultimately be for the next major version though

[dawn merlin]

For sure

[outer magnet]

Idk but a suggestion that the docs give a note saying CommandInteraction#guild and CommandInteraction#guildId will return null if the client user isn't in the guild / used the command in dms

[opaque vessel]

Isn't that... obvious?

[visual hornet]

well, Message would have to have that as well? and also Typing? and then it would have to be on the base Interaction to let it be shown for the other interactions?

yeah i don't think it's really worth documenting, as jiralite said, it's kinda obvious

[outer magnet]

hmm okay then

[visual hornet]

well since noone has brought up anything conflicting with this, ill just mark my pr ready for review, since im free to respond to any feedback or comments now

[copper garden]

Has it been tested against guild roles?

[visual hornet]

oh shit, it hasn't, i forgot about those.

well there's where the positions are correct as is, since the positions are reversed, so ids are sorted descending instead
so i guess there'd have to be some extra option or check for that?

discordSort is only being used for those 2 (channels and roles) though, so that should be everything left
so this can be resolved but i don't know what would be the best way to go about it.

[analog oyster]

you could add an option for the sorting order ¯_(ツ)_/¯

[visual hornet]

i did mention that but im not sure what the best way to go about it would be

[visual hornet]

by the way, is there a specific reason parseInt is used instead of, say, Number in discordSort?

[opaque vessel]

Maybe it should be BigInt(a.id) - BigInt(b.id). That code is from several years ago... won't have to slice

[visual hornet]

alright ill include that in my pr

[tacit crypt]

technically Number(BigInt(a.id) - BigInt(b.id))

[opaque vessel]

Noo that won't work

[tacit crypt]

Except it will

And sort expects numbers not bigints

[visual hornet]

was typing a question, got answered before i finished

[opaque vessel]

Number(BigInt("858851038236901377")) - Number(BigInt("858851038236901376")); evaluates to 0 and that's not true

[tacit crypt]

Not

what

I showed

Number( BigInt(a.id) - BigInt(b.id) )

[visual hornet]

make them strings

the ids.