archive-library-discussion

[outer raven]

but prs are being reviewed daily and then there's some that are 20 days old that haven't gotten an update since

[wild flax]

most likely because they are very low in the priority list

we usually don't look at PRs if they arent on a milestone thats soon to be released

[outer raven]

Well they were on the 13.3 milestone

Talking about https://github.com/discordjs/discord.js/pull/6782 btw

You moved this to 13.4 just now, I was just wondering why

[wild flax]

Because in the timeframe we wanted to release 13.3 there was no resolution

And this PR comes with a huge perf hit

[outer raven]

I understand, but the initial concerns were replied to and then nothing else was said, I believe a solution could’ve been worked out or a decision could’ve been made (since the only alternative to that PR without the performance hit would be semver major)

[wild flax]

IMO it was pretty clear that the perf hit is pretty much a no-go

so yeah

[opaque vessel]

Hihi, I'm unsure if https://github.com/discordjs/discord.js/issues/6905 actually has a pull request per its tags :thinking:

[wild flax]

does not, is mislabeled

[opaque vessel]

Thank you for correcting it (':

[outer raven]

Well then if no one can come up with an alternative it should be closed? Tbh I’d rather if it wasn’t but if it’s the only option then it’s better than leaving it hanging

[wild flax]

the perf and memory hit is not acceptable honestly

for such a niche fix

like

the fix would be for <20% of people probably

but affect 100% of people

[outer raven]

Thing is the current behavior is not following discord’s behavior, and it has been said in this channel that it is not ideal

So instead of removing userupdate events completely (or almost completely) it would be better to have them both work

I understand that the performance hit is not acceptable, but if feedback was given maybe other solutions could be found

[wild flax]

I honestly think over overestimate the ability for us to give feedback

certain things cant be magically fixed with some sort of solution honestly

[outer raven]

Yeah I know you don’t have the answers for everything, but I thought someone might do

Either way saying nothing at all doesn’t seem right, specially for a “first” time contributor

[wild flax]

If theres nothing to say, im unsure what to elaborate on

[outer raven]

Close maybe? Idk I think it can’t stay open forever right

But if its closed it should be explained to the author maybe

[outer raven]

Apparently the collection repo says the latest release was 0.2.4 even though there's a 0.3.2 tag, could someone update this? (maybe even release a new version for the new method)

[outer raven]

Any reason why the return type of MessageEmbed#toJSON is set to unknown? Pretty sure this could be safely set to APIEmbed right?

[opaque vessel]

I think it's because nearly all others type toJSON() the same. It should be fine

[outer raven]

other toJSON methods don't have interfaces but this one does, so it should be used imo

unknown types are annoying to work with

and the docs even document it that way

[dawn merlin]

It might have been set before dapi-types were integrated into djs

[outer raven]

I don't think so, it was just done to all because the other toJSON methods can't have the API types

so we'd have to be making tons of interfaces

unless there is a way to make an interface out of a class in TS

[dawn merlin]

Interfaces can extend declared classes

[outer raven]

we'd still have to declare them all though, I'm talking about like a type or something that turns a class into an interface

[tacit crypt]

wrong

iirc

because it has our camelCased properties in footer/author

[outer raven]

it doesn't, I tested

didn't test with all fields, but the ones I did did have snake_case

will test with all actually

[tacit crypt]

From an embed you made or you got from the api?

[outer raven]

from an embed in a message that I called toJSON on

[tacit crypt]

Ah

We made toJSON act like the old _apITransform

[outer raven]

apparently yeah

[tacit crypt]

then yes that type could be corrected

[outer raven]

oh wait there is an issue

some props are snake cased some are camelCased

[tacit crypt]

toJSON returns api data

[outer raven]

nope, not for image for example

[tacit crypt]

then that should be fixed

wait

it doesn't even matter

proxyURL cannot be set by the end user

and the API ignores it

[outer raven]

so is it fine to keep it like this?

I mean others like author have their icon_url typed in snake_case so guess thats fine

[tacit crypt]

I think you can safely type it as APIEmbed, tho it will mean that proxy_url won't be set

[outer raven]

if it doesn't get set by us nor the API I guess it's fine right

[tacit crypt]

the API sets it derp, but it ignores it if we provide it

which is what I said

[outer raven]

aight then that's fine as it is

[dawn merlin]

@tacit crypt is this what you're suggesting in the builders PR?

    public setAutocomplete(autocomplete: false): Omit<this, 'setAutocomplete'>;
    public setAutocomplete(autocomplete: boolean): Omit<this, 'addChoice' | 'addChoices' | 'setAutocomplete'>

[tacit crypt]

.....no

Stop Omitting anything if autocomplete is false

and definitely don't omit setAutocomplete for a general boolean set

[dawn merlin]

So have the overload return this? and remove the setAutocomplete omission from the general boolean method?

[tacit crypt]

The overload should return the original class

without omissions

[dawn merlin]

ohh ok, now I see my misunderstanding, this is mutated whenever an Omit is returned, it's not static

hmm, the only workaround I can think of for this would be to do something similar to SharedSlashCommandOptions<ShouldOmitSubcommandFunctions = true>, and add a SharedSlashCommandOptions<ShouldOmitSubcommandFunctions = true, ShouldOmitChoiceFunctions = false>

[tacit crypt]

that won't help at all

the omit is done already

Just return the original class

or just don't omit it if this makes it so complex

[dawn merlin]

but how do you return the original class without a polymorphic this? If you change an overload to return the original class without using this it's an error:

do you mean return this & { addChoices(), addChoice() }

[tacit crypt]

you shouldn't add it there..

In the setAutocomplete base class

this & Pick<CommandOptionBaseWithAutoCompleteOrWhateverItIsCalled, 'addChoice' | 'addChoices'>

[dawn merlin]

Ok, so this?

public setAutocomplete(autocomplete: false): this & Pick<ApplicationCommandOptionWithChoicesBase, 'addChoice' | 'addChoices'>;
public setAutocomplete(autocomplete: boolean): Omit<this, 'addChoice' | 'addChoices'> { ... }

[tacit crypt]

yes

Probably, test it

make sure everything builds and works

[dawn merlin]

got it to work with conditional types:

public setAutocomplete<A extends boolean>(
  autocomplete: A,
): If<
  A,
  Omit<this, 'addChoice' | 'addChoices'>,
  this & Pick<ApplicationCommandOptionWithChoicesBase<T>, 'addChoice' | 'addChoices'>
>;
public setAutocomplete(autocomplete: boolean): Omit<this, 'addChoice' | 'addChoices'> { ... }

[tacit crypt]

That's..why

That's overkill

Makes the second overload useless

[dawn merlin]

Because if you use just the top overload, you have to cast the return to any ie return this as any

this is a TS type limitation, ref: https://github.com/microsoft/TypeScript/issues/33912

[dawn merlin]

This doesn't work because I get this error:

The call would have succeeded against this implementation, but implementation signatures of overloads are not externally visible.

Even though everything is exported

[burnt cradle]

couldnt I just do ```ts
public setAutocomplete<T extends boolean>(
autocomplete: T,
): T extends true ? Omit<this, 'addChoice' | 'addChoices'> : this { ... }

[dawn merlin]

You can but the issue is that you have to cast the return type as any

Return this as any

[burnt cradle]

its not making me

I can use it fine without casting

[dawn merlin]

hmm you're right, I could've sworn it didn't let me before

[wild flax]

can anyone test running npx tsc typings/tests.ts --noEmit

not sure if its TS thats being unreasonable here (again)

[ruby terrace]

uh....this seems weird

[dawn merlin]

I'm erroring out, but the strange thing is that #6913 doesn't?

[wild flax]

different file

[ruby terrace]

Does the tsconfig somehow not include tests.ts?

[dawn merlin]

oh I see

[wild flax]

yeah I found it by accident

[ruby terrace]

uh, why is skipLibCheck enabled again?

[dawn merlin]

I wanna say that there is a misconfiguration somewhere, because the typings work when djs is used in another project, and within tests.ts my intellisense is accurate

[wild flax]

it was never disabled

🤔

yeah but I dunno if I wanna trust the editor over the cli

[ruby terrace]

The editor doesn't show errors because those files aren't being included properly it seems

[wild flax]

also eslint errors

have to add them to an eslintignore file too

[ruby terrace]

ts will still error over eslint, but yeah, we'd need the typescript parser for eslint to not error

[ruby terrace]

skipLibCheck is false by default and wasn't in the tsconfig until 2 days ago

[dawn merlin]

ok I included the typings dir and the editor now shows the corresponding errors, these are legitimate errors

[ruby terrace]

although this was previously enabled

[opaque vessel]

I see it in the git blame for 2 years :thinking:

[ruby terrace]

it keeps changing

for context why that's a problem: #archive-offtopic message

That reaction from space feels appropriate

[wild flax]

feel free to disable it again

it seems that its defaulting to true in the common tsconfig we use

[outer raven]

it's actually true by default if you run tsc --init

[ruby terrace]

by default I mean when its read as undefined in the file

[outer raven]

ah then yeah

[dawn merlin]

it's strange that when you run just npx tsc in the root directory it successfully compiles test.ts because it spits out test.js file. When you do npx tsc typings/test.ts it fails

[ruby terrace]

@wild flax OH, i forgot this...tiny little detail

https://www.typescriptlang.org/docs/handbook/tsconfig-json.html#using-tsconfigjson-or-jsconfigjson

also it appears vsc does not fully execute tsc since doing so with no files does generate the files prop

[dawn merlin]

It still doesn’t compile with those options for me

[rocky latch]

Would it be okay if I add a setting in the client options then?
So peeps can opt-in to this feature.

[rocky latch]

Cause I agree it would affect everyone, hence why I think making it an option is better

(if you have a better name for the option, let me know xD)

[dawn merlin]

so I figured out the TS errors, it'll compile properly if you pass in --strict

that was also the cause of the 4.5 beta errors

[tacit crypt]

That makes..a sliver of sense?

I've had and seen several projects where strict mode was necessary

[dawn merlin]

I'm confused as to why it doesn't use the tsconfig.json

strict is set to true there

Well actually now that I think of it I know why, if you compile a single file it's obviously not going to the tsconfig in the root dir.

So actually npx tsc -p . typings/tests.ts is probably the best way to do it

[wild flax]

but if we add the files array?

does that solve it 🤔

its like super annoying

I also tried like

include: ["typings/**/*"]

but then I don't think it picks up the test.ts file

not sure

[dawn merlin]

the issue is that when you run tests.ts individually, it's not using the tsconfig at all

thats why npx tsc compiles successfully and npx tsc typings/tests.ts fails

[wild flax]

so there was never an issue to begin with 🤔

and the PR that lints the typings its pointless

is what you are saying

is there no debug option for tsc to see which inputs it tries?

im sure there has to

so we can see what happens if you run npx tsc right now

without any changes

[dawn merlin]

#6913? yeah that PR isn't going to change anything

this is correct to being with "test:typescript": "tsc --noEmit"

[wild flax]

ok so we dont need to change anything then

sounds good

how come those changes slipped through then 🤔

[dawn merlin]

I lied, the changes slipped through because of skipLibCheck, index.d.ts is just straight up being ignored if skipLibCheck is enabled

#6913, is trying to manually check that file

[wild flax]

right so all we need to do is disable the lib checks 👍

disable the skipLibCheck oops

[dawn merlin]

or if skipLibChecks causes issues for users, we can simply only use the CLI options for the GH workflow, so npx tsc --skipLibCheck false

bc I think someone said earlier it caused compiler errors when disabled for some users

[burnt cradle]

@dawn merlin sorry but at what point would this not have addChoice when it should

[dawn merlin]

#archive-library-discussion message

Basically if you omit things from this, you mutate its type

[burnt cradle]

yeah but why is that a bad thing here

The only omitting that's happening is when it should happen

[ruby terrace]

that doesn't work, you cannot use a tsconfig when passing a file / path

[dawn merlin]

Yeah I just realized that but running tsc without any explicit source files and disabling lib checks is how to get it working

[ruby terrace]

yeah

the weirdest part is that (after disabling skipLibCheck) adding all ts files manually using the files property causes those errors to show back up

in tsconfig

[dawn merlin]

If you set the autocomplete to false and then set it to true, you’ve lost some properties in your original “this” type

[dawn merlin]

I’m pretty sure it’s just ignoring tsconfig entirely, that’s why the js file is outputted in the same directory instead of dist/

[burnt cradle]

yeah, as you should, because you shouldn't be able to addChoice if autocomplete is true

though I see now the issue if you set to true then false

[ruby terrace]

when I edit the tsconfig to include the files, vscode gives me ts errors in editor

[dawn merlin]

But if you set autocomplete to false afterwards (although somewhat unnecessary) it should still have the addChoice and addChoices props

[dawn merlin]

Same here

[burnt cradle]

yeah ok I get it now sorry about the hassle

[dawn merlin]

No worries

[dawn merlin]

does your tsconfig have strict enabled?

actually removing include entirely removes the false positive errors, but still keeps the true positive errors in the editor

[ruby terrace]

oml I'm stupid, I was using files instead of include, and files doesn't match globs, so vsc was using the default tsconfig

[dawn merlin]

do you know if it's possible to disable skipLibCheck without the side-effects previously mentioned?

[ruby terrace]

Just only disable skip lib check

And that doesn’t even affect the test file

[dawn merlin]

so disable skip lib check and keep skipdefaultlibcheck enabled?

[ruby terrace]

Yes

[rocky latch]

@wild flax Could I have your thoughts on that please, thanks in advance 🙂

[opaque vessel]

Does discord.js use ?type as nullable or nullish? I believe it should be just nullable as intended use from JSDoc? I see some documented as ?type but actual values may include undefined, hence the question

[slate nacelle]

I think we use it for both, I don't know if that's what we want to do though.

[opaque vessel]

I plead for documenting undefined instead, people like me do if (X === null) and expect it to work as such, and not account for undefined

[wild flax]

Doesn't seem feasible

Instead you should just do == undefined

which covers both cases

theres nothing our wrapper I think where undefined or null is really being distinguished

it both means theres either no initial value, or theres no value being sent explicitly

[opaque vessel]

I'm not sure it's for me to change my code style because discord.js doesn't take JSDoc's nullable as... nullable?

To answer if values are being distinguished, an example is on the user class where the recent properties there are undefined if not fetched, null if fetched and not existing and a value otherwise. Or, the message references where it's typed as type | undefined but documented as ?type so it should be null here, not undefined. This is more about being consistent than anything, no?

[wild flax]

It'd be a lot to go in and change all the types now

If you are up for it, please feel free, but I don't think its in any of the maintainers interest to do that work themselves right now

[wild flax]

No thoughts yet, and I won't be deciding that alone either

[opaque vessel]

Alright, thank you for your thoughts

[rocky latch]

Yea I understand that, just wanted to see if you think that would be a better option if in the end you guys decide to add it, should I commit the option or nah?

[wild flax]

You should def put it in the PR

but maybe not commit yet

[rocky latch]

[tacit crypt]

iirc in JSDoc, ?type is type | null, and [name] = possibly undefined

[outer raven]

[name] is optional, not undefined tho

[vernal adder]

Isn't that the same thing?

[analog oyster]

no

[vernal adder]

optional = value or undefined, no?

[outer raven]

not at all

[vernal adder]

What is it then?

[outer raven]

https://www.typescriptlang.org/tsconfig#exactOptionalPropertyTypes

[vernal adder]

So... it's either the value(s) or undefined?

[outer raven]

but optional is not the same as undefined

[visual hornet]

wait wasn't this about jsdoc

[outer raven]

yeah but the explanation is there

[vernal adder]

"possibly undefined" = optional to me

[outer raven]

did you read the link?

[visual hornet]

so it's value(s) or nonpresent

the latter just turning into undefined when you access it, the only difference would be how in behaves, no?

[outer raven]

yeah

[wild flax]

Which makes no sense for the documentation

So yes, [name] means optional and undefined

In JS if you pass nothing as a value and try to access it, it ends up being undefined

[dawn merlin]

Should we enable noEmit in the djs tsconfig?

[outer raven]

but then you can have properties in an object set to undefined, which is different from them not being set at all

[wild flax]

Nah I like to keep that separate

[opaque vessel]

https://github.com/discordjs/discord.js/pull/6838
:thinking: am I missing something or how come Message#inGuild() doesn't make Message#guild non-nullable?

[vernal atlas]

because #guild is a getter

bot leaves the guild or doesnt have guilds intent, no guild

or just for any reason the guild isnt cached

[opaque vessel]

If the bot leaves the guild, then the typeguard would be false?
discord.js requires the GUILDS intent

or just for any reason the guild isnt cached
Only achievable through altering the cache which is unsupported behaviour

[vernal atlas]

• the typeguard only checks if the message was sent with a guild_id field - which would stick even after the bot leaves because its cached

• fair enough

• haven't used djs in a while, wanted to include any other reason it wouldnt be cached

[opaque vessel]

Rip, maybe Message#inCachedGuild() soon™️ then lol

[vernal atlas]

message.guild !== null when

[sullen idol]

We need MessageEmbed#video

[dawn merlin]

It should, it was passing the type tests so idk what’s up

[opaque vessel]

I read the pull request again and saw that it changed from checking .guild (cached) to .guildId (may or may not be cached) so... ya. An example of where it may not be cached is when you leave the guild then decide it's a great idea to check the cached messages you have there. I think it's better left alone now /:

[dawn merlin]

I was going based on what crawl said earlier: #archive-library-discussion message

Also if you leave a guild I think it’s best that messages from that guild get wiped from the cache. Mainly because of the issue being described; you can’t property represent their current state

[opaque vessel]

I was thinking that too!

[dawn merlin]

I just realized that we’re doing this anyways because when a bot leaves the guild we delete the guild from the cache thus all the messages are also deleted

[opaque vessel]

Oh huh. Wow. Great. So we can assure that the guild is not possibly null then? :D

[dawn merlin]

Yup

[opaque vessel]

Happy days (:

Hold on, on the guildDelete event, the cached guild is passed through, right? Isn't it accessible there?

[tacit crypt]

The previously cached guild is passed through*

[opaque vessel]

But it's sill possible to check the previously cached guild's cached messages via that right?

Too much thinking

[tacit crypt]

I mean I GUESS you can go deletedGuild.channels.first().messages.first(), yeah?

[dawn merlin]

The only other option I see here would to set all of the messages guildids to null, but idk about that

[opaque vessel]

The other option is .inCachedGuild() I guess o,o

[dawn merlin]

But that method relies on guild ids not guilds, so if you use it in the delete handler, it’ll give a false positive

I’m pretty sure guild ids don’t get removed from messages even if the guild itself is removed from the cache

[opaque vessel]

(Just to confirm the method I typed out above would rely on cache and not the id)

[dawn merlin]

Oh well we discussed that on the PR Vlad suggested that we use the id instead of the getter for performance. And in 95% percent of cases this is accurate

[opaque vessel]

Yeah, oh well

[dawn merlin]

I would just strip all messages out of the previously cached guild on the delete event

I don’t really see the use cases in giving users the old guild messages from a guild that they’re no longer part of

[dawn merlin]

@ruby terrace was it ever verified if min/max and choices are mutually exclusive?

[ruby terrace]

no

they are not currently but I believe they should be

[dawn merlin]

Sadness

On the topic on regressions, the typescript devs have confirmed that the errors appearing for us on 4.5.0 but not on 4.4.4 are in fact regressions from 4.4.4: https://github.com/microsoft/TypeScript/issues/46624

[wild flax]

[opaque vessel]

Latest GitHub Actions seems to be queued for >= 25 minutes... outage? :thinking:

[outer raven]

nothing reported on github's website at least, could've been a one-time error

maybe rerun?

[real jetty]

Is there a plan to include the jsdoc comments inside the typings so they appear in intellisense?

Talking about this btw

[outer raven]

Yeah on the ts rewrite

Currently not really feasible

[real jetty]

is there any plan for the ts rewrite or is it just "sometime before 2030"? i saw djs-next got removed

[outer raven]

the djs-next repo got renamed to discordjs-modules, not sure whats being done about that but its not gonna be v14 for sure

[vernal adder]

There is no set date, but I believe the focus will be moving towards the rewrite in the coming months

[outer raven]

hope so

[ruby terrace]

working on drop ins for rest and ws first, rest I'm hoping to get into v14, ws may be a v15 thing or potentially semver minor on 14 depending on implementation. Once those are migrated over, and we're not stepping on ourselves, it should be easier to push for the main lib

[real jetty]

Probably not because voice was rewritten in TS, which djs doesn't have docgen support for

[wild flax]

I think it mainly has to do with the fact that its a pure function

which is something we should, and can add support for

will happen

[burnt cradle]

is there a point in having babel as a dep in builders after babel-jest got removed?

[digital sun]

Is there a reason the API includes bots in memberCount and no easy way to get the accurate count without bots without fetching the entire Guilds member list?

[slate nacelle]

The reason is probably that there is no need for such for their clients.

[wild flax]

Jest now directly uses Babel when a Babel config is found, so yes

[burnt cradle]

Couldn't ts-jest just be used instead?

[wild flax]

no ts-jest is trash

[burnt cradle]

fair

[solemn oyster]

Just to clarify it further, the only advantage of ts-jest is typechecking, but in reality, that's unneeded since typechecking is already done by our build step.

Babel's config is a lot faster thanks to it stripping the TypeScript types so it can be run as plain JavaScript.

[digital sun]

is there a bug with the 'remove' call back on a messageReactionCollector? Its never called and digging through the code im not seeing this.emit('remove', ...args); being called anywhere in Collector.js like i do with collect or dispose and end

[unique axle]

you need to provide the dispose option for removal to be emitted

[digital sun]

oh? is dispose called on remove?

[unique axle]

actually, is that gone from v13 or just not documented (anymore) found it, it's documented in CollectorOptions

[digital sun]

remove? its not in the code

but it shows as an option on the collector

all ik is that its just not firing at all like the other callbacks

Collector.js line 84 and 85 i see

this.handleCollect = this.handleCollect.bind(this);
this.handleDispose = this.handleDispose.bind(this);

No mention of remove

[unique axle]

no, i mean that the "remove" event only emits if the option dispose: true is provided
https://discord.js.org/#/docs/main/stable/typedef/CollectorOptions

[digital sun]

Oh??? Thats really nebulous

[unique axle]

it is documented

https://discord.js.org/#/docs/main/stable/class/ReactionCollector?scrollTo=e-remove

Emitted when the reaction had one user removed and the dispose option is set to true.

the "dispose" event emits when the entire reaction "stack" is removed in one go, also only if the dispose option is set to true
https://discord.js.org/#/docs/main/stable/class/ReactionCollector?scrollTo=e-dispose

[digital sun]

the naming is misleading i guess

you would think dispose is for the dispse call back

[unique axle]

disposal is required for either to work, hence why it's documented on each event

[digital sun]

Cool cool, I had it there before, but wasnt sure why so I removed it not thinking it would stop the remove callback lol

[analog oyster]

Why is repliedUser under Message#mentions? I feel like that's misleading since the repliedUser property will be populated regardless if the reply is mentioning the user or not. It should perhaps be put under Message#reference?

I guess the description of the property explains what it actually is, i just think its a bit odd

The author of the message that this message is a reply to

[outer raven]

definitely not under reference as that's an object sent by Discord iirc

think it should be its own property

[analog oyster]

yeah thats fine as well

[unique axle]

the mentions options object answers the question "who is mentioned"
-> roles, users
-> the user this replies to - repliedUser
-> parse all roles, users, everyone

[outer raven]

you can reply without mentioning tho

[unique axle]

yes, i don't see how that plays a role.

[outer raven]

well then it shouldn't be in mentions if no one was mentioned

[unique axle]

then i'd consider the part bug that it is there, even if not mentioned

[outer raven]

can we patch that in a semver patch?

the fix is quite easy

[visual hornet]

would fixing a bug that changes behaviour have to fall into semver major?

[outer raven]

i mean it's removing something that would be there before, so could be major idk

[unique axle]

i suppose it'd be major, since it is documented correctly

[outer raven]

well, incorrectly for that matter

[unique axle]

no, correctly. it documents the current behaviour

[outer raven]

which is incorrect, so its documented incorrectly too

[unique axle]

... no. it is currently documented correctly := documentation and behaviour match
-> a change would not be a fix
-> it does not bring any new functionality. you can find out if the user was mentioned via #mentions#users
-> it's a change in behaviour and semantics

[outer raven]

semver major it is ig

[rough roost]

InteractionResponses#update throws INTERACTION_ALREADY_REPLIED if you call it after deferUpdate, that doesn't look right

[ruby terrace]

it's right

[vivid field]

reply, deferReply, update and deferUpdate all act as the first reply to an interaction

after that you will have to use followUp, editReply or whatever

[rough roost]

ah thanks

this is a very confusing api

but discord moment ig

[outer raven]

Quick question about collection: the es2022 PR is being merged on TS and they decided not to make the argument in array.at optional, despite it working with no errors so I'd like to do this for collection too. Question is: should I remove the test that has no index or should I change it to something else? I don't think it should be expect().toThrowError() since, well, it doesn't throw an error

[tacit crypt]

if the spec says its optional it sounds like they need to fix it on TS side, no?

context is needed

[slate nacelle]

mdn documents it as required, the spec doesn't mention anything, but will treat it as 0 due to how ToInteger is defined to work

No idea if intentional or not

[outer raven]

I specifically requested for it to be made optional and they said that even if something works they’re not gonna allow it because it doesn’t work as intended

Lemme link the convo

https://github.com/microsoft/TypeScript/pull/46291#discussion_r738796155

[outer raven]

Also could someone rerun this check? https://github.com/discordjs/discord.js/runs/4080702654?check_suite_focus=true
Feel like this one is quite important since the commit only changed documentation xD

[outer raven]

So @tacit crypt what should I do about the tests?

[tacit crypt]

More of a crawl question I'd say.. specifically if we want to make .at take in a required number.. CC @wild flax

[outer raven]

Aight will wait for him then

Also noticed an awkward issue: the parameter is marked as not optional in the documentation 😬

So ill wait for his response to fix that too

[wild flax]

What documentation?

[outer raven]

this

[wild flax]

You don’t seem to understand typedoc

Check the actual docs

[outer raven]

oh right I forgot about that

nvm i guess

well then

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