#archive-library-discussion

25217 messages · Page 22 of 26

solemn oyster
#

@analog oyster don't we also need to define an external?

analog oyster
#

yeah, about that..

solemn oyster
#

... oh right

#

No website, I forgot lmao

analog oyster
alpine bough
#

there is many "errors" like this one in the code
should it be fixed or no need? FeelsDankMan

#

import/order eslint error

analog oyster
#

probably just a version mismatch between your editors eslint and the actual eslint version on the repo, because there are no such errors in the workflows / on my end

alpine bough
#

alright, thanks :)

dawn merlin
vernal rose
void rivet
#

regarding #7395, shouldn't we update the tests?

opaque vessel
#

Yes

void rivet
#

okay then, i missed that when making the pr 👍

opaque vessel
#

(:

void rivet
#

Done! All tests passed 🤠

woeful rain
outer raven
#

boop

solemn oyster
lofty beacon
real jetty
#

You can ignore, it was made by a recent failing commit

lofty beacon
#

Alright thanks

opaque vessel
#

This can be specifically useful when working with moving threads between 2 channels.
You can move threads between channels?

outer raven
#

And how would a channel url be useful in that scenario?

nova kestrel
opaque vessel
#

Why are you passing it in the first place?

nova kestrel
#

im not, but someone i was just helping had an issue where null was being passed

#

makes it quite confusing to debug

#

had to read the source to figure it out

opaque vessel
#

Are you telling me one doesn't check their input values

#

That seems to be the bigger issue

nova kestrel
#

it obviously wasnt intentional

#

they were passing in a slash command ID arg but made a mistake with it, resulting in null unexpectedly

#

so is this just a case of "not my data not my problem, check your inputs"? or is it going to get changed to be useful

opaque vessel
nova kestrel
opaque vessel
#

It should be clear that the third line there you're doing something wrong, I wouldn't say that is not clear

#

The problem is that, if we add checks for nullish behaviour, might as well add it across the whole library... and that just ends up bloating it

nova kestrel
#

its clear that something went wrong internally sure

opaque vessel
#

Incorrect, something went wrong with what the user did

#

That error is only happening because you passed something that was not documented nor typed. That's on you

ornate topaz
#

does that mean that we have to remove every other input check now?

nova kestrel
#

"nor typed" in.. javascript.. a dynamically typed language

#

consistency would sure be nice

opaque vessel
#

I feel like this conversation has happened before

nova kestrel
#

probably has

ornate topaz
#

wasn't it about checking stuff passed directly to API though?

#

and not stuff that library blindly tries to access?

nova kestrel
#

both are pretty annoying to deal with

#

dapi returns pretty nonsensical errors at times

#

in this specific case, if djs threw an error saying "Malformed options passed", that would go a LONG way to helping people debug things

#

luckily the lib failed quite early, but if this object had made it more internal it would have been a nightmare to debug

opaque vessel
#

Maybe I see things differently but that looks braindead easy to see what went wrong

nova kestrel
#

to you and i perhaps

visual hornet
#

isn't this the kind of thing typescript helps with

nova kestrel
#

but we both know djs is used by much less experienced developers

nova kestrel
#

but yes, TS would have stopped this

visual hornet
#

when did i assume the majority know...?

ornate topaz
#

if TS would have stopped this, why does the lib need any input checks at all then

nova kestrel
#

the.. the lib isnt written in TS

#

and can be used by JS

#

JS obviously has no static typing

visual hornet
#

typescript prevents bugs, if someone doesn't want to debug they can use ts for that to prevent them, if they don't know how to debug then maybe they should learn that first

nova kestrel
#

"maybe people should learn JS before writing a bot", we all know this is not the case, and we can all hope however unfortunately a lot of new developers try djs because it looks easy

#

also the sort of people i see who know nothing about programming arent interested in using TS

visual hornet
#

are the changes you're proposing even going to benefit them if they know that little

ornate topaz
#

do the 23 instances of d.js throwing INVALID_TYPE on input do that?

nova kestrel
nova kestrel
ornate topaz
#

benefit them

nova kestrel
#

i would say so

#

i see countless people posting about the intents error / the invalid token error

#

makes helping so much easier

#

also all the embed checks

visual hornet
# nova kestrel it would benefit the people they come complaining to with a vague error message

even if there was another custom error message for input validation, it'd have to use the proper terminology to be beneficial to the developers that understand them, and then complete beginners would still say they were vague.

i see countless people posting about the intents error / the invalid token error
...which means it isn't helping then? it makes you help them easier, because you know of the error, or if you don't, how to find it
to more experienced people, more abstract errors can be more useful, like the error you mentioned

TypeError: Cannot read properties of null (reading 'guild')

with experience you can know where to look or what to look for to debug.

nova kestrel
#

they are helped, be that through their own research or through someone else

#

wrt the abstract error, i literally had to read the source to figure out what was going wrong since the person i was helping was obviously too new to debug it themselves

ornate topaz
nova kestrel
#

ok and? why not do this everywhere then?

#

make a util and use it

visual hornet
#

uh, i mean, if you want it why not you do it everywhere?
you can just make a pull request

nova kestrel
#

i can

ornate topaz
#

and it started with a question about it

nova kestrel
#

i dont know enough about djs internals or design, nor am in particularly interested. i was just interested to see if there was any motive for this design and / or plans to change it

#

and tbh i assumed it would get rejected so i didnt bother

void rivet
#

according to my tests
i had an account with the MANAGE_THREADS permission but not the send messages in threads permission
and the account couldn't unarchive archived threads
so i'd say this code is ok
but the way it wouldn't let me unarchive was weird

void rivet
#

yes and how is that not correct? you tested it as well

#

so what, you're suggesting this.archived && this.sendable && (this.locked ? this.manageable :true)

#

is it like this for you too when you try to unarchive a thread?

visual hornet
void rivet
#

reading the dapi docs, even that's so unclear, but regardless i think you need send messages in threads to unarchive in general

visual hornet
#

although it still could be a UI-specific issue, api tests would probably be better

loud jayBOT
sullen idol
#

isCategory() method? 👀

opaque vessel
#

What about it

sullen idol
#

I'm not seeing one, how do I request to add?

opaque vessel
sullen idol
#

Weird, alr thnx

opaque vessel
#

In future, it would be better to ask your actual question first

sullen idol
#

Wdym?

raven wind
# sullen idol Wdym?

means isCategory() method? isn't a clear question and could mean anything, so ask directly next time PepeChrist

brittle cosmos
ornate topaz
#

no, not quite. and this isn't really for this channel either

jovial smelt
#

how long djs cache the last message id when the last message deleted?

outer raven
#

Iirc it deletes messages from cache if they’re deleted

jovial smelt
outer raven
visual hornet
#

it's not cached, it's a property that discord sends
so i guess it stays "cached" as long as discord doesn't send data to overwrite it?

jovial smelt
#

cause when my bot deleted the old message embed that has been edited, and send a new message with same command and trying to edit that message, and wallaaa it crash cause the my bot edited the last message that has been deleted

#

so i wonder how long the cache is stored and deletd from the memory idontknow

visual hornet
#

that seems like it would be a logic issue, between using channel.lastMessage.edit/channel.messages.cache.first().edit/interaction.editReply

analog oyster
#

discord.js updates the lastMessageId when a new message is received

jovial smelt
unique axle
#

#lastMessageId is not evicted
#lastMessage is a getter from #lastMessageId and directly depends on the cache
we evict messages from cache when we receive a websocket delete event

visual hornet
#

wait, "evicted" is used in this context?

unique axle
#

delete, remove, yeet, pummel, obliterate, nuke, call it what you want

jovial smelt
#

like this

#

when the new command has been run, the bot trying to edit this embed that has been deleted

outer raven
jovial smelt
vernal rose
#

Djs uses null everywhere but in some places undefined will make sense. An example is CommandInteractionOptionResolver#getX, here if a string named name isn't present that means its undefined which doesn't exists. But null is a value which exists but its null. So in these places why null is used instead of undefined?

inland lotus
#

null means it doesnt exist

vernal rose
real jetty
#

Using undefined in a lib can get confusing quickly, because js also regularly throws undefined if a method doesn't exist

rustic boughBOT
#

Documentation suggestion for @vernal rose:
_ null
The value null represents the intentional absence of any object value. It is one of JavaScript's primitive values and is treated as falsy for boolean operations.

analog oyster
#

read the first phrase

drifting knot
wild flax
#

Meh

golden mortar
#

that doesn't explain why at all

copper laurel
#

Yeah that's just an opinion which states a list of reasons but doesn't really justify any of them.

  1. Developers being inconsistent is because they're bad developers.
  2. Supporting both doesn't complicate input validation because they have distinct use cases, its a symptom of the above, bad developers.
  3. "Newer JS features like default parameters only work with undefined" yes, because undefined is fit for purpose. Passing null should be intentional and not default. This is #2, again a symptom of #1.
  4. "I strongly believe it was a mistake to have two have both null and undefined." - purely subjective
  5. "Using null makes TypeScript types more verbose" - good. Verbosity is a good thing. Stop trying to make code shorter, and usually worse, for no good reason.

This whole things reads as an instance of #1, a developer who used them inconsistently, didn't understand what they were both for, and just wants to eliminate one

#

typeof null being object is the only remotely valid point but they proposed changing it once and it was a disaster

#

That's a problem with JavaScript being backwards compatible to the dawn of JavaScript

solemn oyster
#

Why is this in #archive-library-discussion, and why is such a 40 IQ discussion even brought up?

Also, good luck removing nicknames or timeouts without null in Discord agooglethumbsup

strange igloo
#

Yeah that's a little bit complicated to get rid of it
null and undefined don't have the same meaning and both are useful when used accordingly

And for people who don't like (I include myself in this) writing | null all the time they can simply disable the strictNullChecks rule in their TS config instead of blindly enabling all strict options

vernal rose
strange igloo
# vernal rose From where it was started and now 😂

undefined means "no idea" while null is a concrete lack of value
Which is likely why these methods return null when no data can be retrieved

From undefined you can't guess much, but from null you can determine that what you are looking for is definitely not where you asked

Which is probably why (I don't hold the truth) methods in option resolver return null when they can't return an actual value

solemn oyster
#

When setting values, undefined means no behaviour or unset, while null is a reset signal

real jetty
#

If #7023 (modals pr) is ready to be merged before v14 is ready to be released, would a release be held off until v14 or would you guys do another semver minor in v13?

solemn oyster
#

We'll have an answer to that in the upcoming hours, deciding internally what we'll do

copper laurel
raven wind
gaunt lotus
woeful rain
#

why discordjs@rest 0.3.0 is not supported? (latest tag)

visual hornet
#

a newer (dev) version of @discordjs/rest is being targeted now i guess?
isn't the latest tag automatic? that would be why latest points to a deprecated version if that were the case, i'd guess

fiery ocean
visual hornet
fiery ocean
#

Not... sure how I missed that, thanks ameowderp

void rivet
#

asking again 😢
about the setEmoji methods, now only taking an object instead of an EmojiResolvable, is that intended behavior? last i heard someone said it may not be but didn't get any follow-up to that

also should the documentation for the methods list the API defaults as it's own default value?
like https://discord.js.org/#/docs/discord.js/stable/typedef/FetchGuildsOptions this lists limit's default as 200, in the code it doesn't set a default value, it's actually the api default that's mentioned here

quiet viper
#

Is there a event/way (yes, know the invalidRequest ones...), in order to get informed about 40X errors?
Having one for metrics would be great

tacit crypt
#

we have apiResponse events iirc that you can tap into

copper laurel
#

@woeful rain @visual hornet @fiery ocean There was an error in deprecating old versions of those packages. All @discordjs/<package> are still supported - the source code have all been moved to the monorepo

wintry pendant
#

I'm running into an issue while making my pr (https://github.com/discordjs/discord.js/pull/7445):

I have modified GuildAuditLogsResolvable and GuildAuditLogsFetchOptions to:

export type GuildAuditLogsResolvable = AuditLogEvent | null;

export interface GuildAuditLogsFetchOptions<T extends GuildAuditLogsResolvable> {
  /* ... */
  type?: T;
}

The issue is that typescript still allows for T to extend any arbitrary number even if its not a value of AuditLogEvent. This causes the following test case to fail:

expectType<Promise<User | undefined>>(
  // @ts-expect-error Invalid audit log ID
  guild.fetchAuditLogs({ type: 2000 }).then(al => al.entries.first()?.target),
);

Am I doing something incorrectly / is there anyway to make sure T can only extend AuditLogEvent properly?

woeful rain
cerulean pumice
#

when are modals expected to be available on here?

opaque vessel
cerulean pumice
#

👍

outer raven
#

hey crawl, just realized we can't actually commit like that, can you add it to the config?

tacit crypt
#

Add it in the message body not the title

outer raven
#

Crawl told me to put it in the title

wintry pendant
tacit crypt
outer raven
dawn merlin
#

@vernal rose the src folder is published in the package

vernal rose
dawn merlin
#

Well compiled js isn’t the same as the source js

vernal rose
#

but when we'll import anything from djs it'll come from compiled one and not source one right? as main is dist/src

dawn merlin
vernal rose
dawn merlin
vernal rose
# dawn merlin Yeah and the typings folder is published too

ok so i was saying that dist/src is what everyone will use and src will be published as a root folder but except for typings this src folder is useless means no one will be able to use it. So I think you can add all src/**/*.js files in npm ignore and src/**/*.ts will be published only for typings. Means these source js files don't have any use in npm only compiled will be used

dawn merlin
vernal rose
#

ok i didn't tested it xD lemme test it.

#

I'm not seeing any errors when importing in vsc and this file isn't included in the build so idk how can I test it

vernal rose
strange crane
#

Isn’t it suppose to call the identify method 🤔

ruby terrace
#

The webscoket code is pretty complex, if you look into it you'll see it does attempt a resume, its just not the way you think its supposed to happen

outer raven
visual hornet
#

well, the behavior would change if the defaults changed in the api, so i'd guess that could be why?

unique axle
#

you literally noted it as a breaking change in the commit notation with the ! shibaThinking

visual hornet
#

hmm that was after the label was added though

outer raven
visual hornet
#

well it's making the behavior rely on the api instead of the library, and whether the behavior changes at all would be out of djs' control, so yeah i'd say that could possibly be a breaking change

outer raven
#

aight

#

On this typedef, the default for failIfNotExists comes from the default client options, which can be misleading because a user might be expecting this to default to true even if they set the client option as false. Should it be removed from the typedef?

tacit crypt
#

If it defaults to the client, you could also just do failIfImLazy=this.client.options.failIfImLazy

outer raven
#

that's probably a better move

#

also nice name

woeful rain
solemn oyster
#

Internal discussion

#

Basically we want the TypeScript rewrite to be done from the ground up, instead of translating everything with the flaws and worrying about the rest later

#

And our docsgen parses either JavaScript or TypeScript, not both together

lilac tulip
copper laurel
#

I'm not against it, but I don't quite understand the difference between 10 "addSubcommand" function calls, or an array with 10 builders in it

lilac tulip
#

imo it would look "cleaner", but I guess that's something everyone has to define for themselves at some point. I'm sure there's people that would argue that it's not cleaner in any way shrug

void rivet
#

the thing is to make it consistent with the other methods, you'd need to use rest parameter
which brings me to a bigger question which monbrey and i talked about already i think, what the usage of addSingular(obj) now? when they're basically just addPlurals(obj) with one parameter
example: addField(field) is just addFields(field)

tacit crypt
#

Pretty sure most singular adds are called by the plurals

unique axle
#

still pretty useless, if we decide to go with rest parameters

void rivet
#

yeah there are also some that call plurals internally
it's a mix in the implementation

but not the point, i'm asking about the user's side, is it really useful to keep the singulars?

unique axle
#

making that all one method seems more streamlined, if the usage is the exact same, but with one more character

void rivet
#

soo, should we remove them?

outer raven
#

Getting this error with the v13 modals PR installed but I can't seem to understand why it's happening, is it related to that PR?

void rivet
#

version mishap i guess

outer raven
#

the only version of discord-api-types I have is 0.26.1

#

Also this might be more insightful but I'm still clueless

tacit crypt
#

mismatched dapi versions

copper laurel
#

Yeah I didn't touch those afaik

warped crater
#

yeah that's what the errors look like when you miss match versions

#

means djs or whatever package is using a different version than the one you're explicitly using

outer raven
#

it's weird bc I don't have discord-api-types as an explicit dependency in my project

#

but alright

tacit crypt
#

If you npm ls discord-api-types what does it show

#

Also interesting, you have a deduped -types and then d.js has its own -types

outer raven
tacit crypt
#

Probably needed a reinstall of deps yeah

outer raven
#

probably got messed up from changing between monorepo and v13 branches

manic cairn
#

i'd make a PR for this, but it's feeling pretty crazy pills that nobody else has this problem:

  • i try to create a guild emoji, using a web URL
  • resolveImage checks it, see it's not a data:
    and passes it to resolveFile
  • resolveFile performs a node-fetch res = await fetch
    then returns res.body
  • the body, per node-fetch v2.x.x docs,
    conforms to node's Readable Stream spec.
    it identifies in Console Log as a PassThrough object
  • so, what resolveFile has returned, is a stream. not a buffer.
  • the results of resolveFile are fed into resolveBase64
  • resolveBase64's only check is "is this a Buffer" (it is not)
  • resolveBase64 gives up, returns the readable stream as-is
  • that object is stuck, as-is, into the API create emoji request
  • this fails, obviously
DiscordAPIError[50035]: Invalid Form Body
image[BINARY_TYPE_INVALID_DATA_URI]: Only data uri strings can be used.
[...]
requestBody: {
    files: undefined,
    json: { image: [PassThrough], name: 'MyEmojiName' }
  }
#

a minimal repro in a clean dir, under current node 16, takes a few lines and fails. but the code looks like this in stable too. it's an easy fix, but i am sure i am doing something wrong here

#

otherwise it would be broken for everyone right???

visual hornet
#

do you have said minimal repro?

manic cairn
# 
$ node -v
v16.14.0
$ npm install discord.js@dev
# 
const { Client } = require("discord.js");

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

client.once("ready", async () => {
  await client.guilds
    .resolve("7777777777777777777777")
    .emojis.create(
      "https://www.bungie.net/common/destiny2_content/icons/eaa17a0ce79c7caa171ce1852ef27569.jpg",
      "myTestEmoji"
    );
});
golden mortar
dawn merlin
#

Honestly the author hasn’t been too active on maintaining the PR

#

CC @errant brook

errant brook
#

also its been approved by so many people, what's holding it?

dawn merlin
errant brook
#

nah that's fine

#

ok I just saw the comments monbrey left, seems like he was asking for a second opinion

outer raven
#

nah he's right, you can never have an APIAttachment so you dont need the type

zenith oracle
manic cairn
#

doesn't exist in main

visual hornet
#

im getting the same error as well as using the docs example and an emoji link; honestly the reason it hasn't been brought up much is probably just this isn't really utilized

manic cairn
#

i'm surprised, i assumed emoji from urls would be more of a newb's path of least resistance

#

thank you though @zenith oracle, that explains the difference between stable

#

i'll drop a pr RQ i guess

errant brook
#

wait what should cache type reducer be changed to then? collection?

zenith oracle
outer raven
errant brook
outer raven
#

no, Collection<Snowflake, MessageAttachment>

errant brook
#

ok makes sense

#

what exactly is cached tho

dawn merlin
analog oyster
#

the pr has a lot of unrelated style changes on the typings file

#

and the attachment option interface is not correct

clever crypt
#

when passing stringified json as the body to REST#post, i get the (python) error Only dictionaries may be used in a DictType from Discord. i'd argue typing RequestData#body as unknown is somewhat misleading because how was i supposed to know you can't pass through a string meguReach

outer raven
errant brook
outer raven
#

just format with prettier

#

but i think there is

analog oyster
#

it should just run when you commit

#

don't know how that even happened in the first place?

outer raven
#

yarn format

outer raven
errant brook
errant brook
analog oyster
#

typescript formatter?

outer raven
#

yes

#

the default

analog oyster
#

whats that

errant brook
#

inbuilt vscode formatter

outer raven
#

its the default formatter that comes with vscode

errant brook
#

yarn format didnt fix anything

outer raven
analog oyster
#

given there are not a lot of changes you could also just get away with rolling back the file and adding the changes back

hollow stirrup
#

I heard that discord.js will soon go modular

errant brook
#

yeah

hollow stirrup
#

At what major semver exactly?

dawn merlin
errant brook
#

guys i need help with git

hollow stirrup
#

Or I missed something

errant brook
#

if you set it up to be modular then yes?

dawn merlin
hollow stirrup
#

Hmm

#

I see

outer raven
#

can someone PR an update to discord.js's version of discord-api-types please? I don't really know how to update the deps on the monorepo so I'm scared I might mess it up

wild flax
#

It is updated

#

It’s on 0.27

vernal rose
# errant brook yarn format didnt fix anything

You've all the dependencies installed? It should format the file when committing or it won't let u commit. But if you don't install them it'll allow u to commit without running git hook

errant brook
#

it does run the hooks and stuff

#

but it didn't fix the formatting

vernal rose
vernal rose
# errant brook

Git revert? Ig you can just use reset --hard to delete previous commit right?

outer raven
#

could you release one maybe? Since the attachment option PR was merged

wild flax
#

Only on builders

#

The one on djs is still open

outer raven
#

oh right ye

#

why did that PR bump deps then

#

for all projects

wild flax
#

I did to fix some stuff

#

So i just bumped all of them to align them

outer raven
#

Ah alright

outer raven
#

@zenith oracle how does using optional chaining fix that issue?

zenith oracle
#

Whoops I commented in the wrong PR lol sorry

outer raven
solemn oyster
#

@clever summit tests are failing

#

At __tests__/components/actionRow.test.ts:93:53

clever summit
#

two tests actually, should be fixed now

dawn merlin
#

Thannnnksss

woeful rain
#

Which style guide does discord.js follow ?

golden mortar
woeful rain
oak quail
#

is the plan for api v10 to be used in v14?

ruby terrace
#

tis my hope

vivid field
#

I don't see why it shouldn't

ruby terrace
#

especially since it'll release so close to v9 deprecation date

lilac tulip
#

will v14 be released before the message content deprecation day?

#

30th april I think

#

because if so, and api v10 is used, wouldn‘t that mean that bots that dont have access to the message content will be restricted as soon as they update to v14?

wild flax
#

Don’t see how it matters?

#

v13 is on v9 which is not decommissions but only deprecated.

#

You can still run on that

#

Whether v14 is out or not

lilac tulip
#

yeah it won‘t be a problem, just something worth noting rather since people might be wondering after updating why they dont have access to the message content anymore even though it‘s before april 30th

solemn oyster
#

By doing so now, they'll notice and start applying over time, that way it makes the transition easier for both us (since they'll have an alternative) and for Discord (since the intent requests will be more spread out)

lilac tulip
#

okay sounds good

outer raven
#

I can already imagine the response to this but I’d honestly like to discuss this
Why don’t we undo the switch to builders?
Yes, maintenance overload by having to maintain the same thing in two places, fair, but discord.js will always need classes for every discord structure because it was built like that. Having those specific ones depend on a different package that uses snake_case has caused way more hassle than it could’ve solved (judging by the sheer amount of fix PRs that have been made since). If we just stick to one style (discord.js one) and keep the old classes, all this hassle would be gone and we could focus development on those classes only and not need to worry about complicated fixes for them. I personally don’t see a point in having builders at all but that’s personal and I won’t get into that. I think discord.js would gain a lot more by keeping its classes streamlined than having things all over the place with quick and messy fixups, but I’d like to know your thoughts

wild flax
#

You think too much of discord.js and less in sub-packages

outer raven
wild flax
#

That’s been our sole goal for the last 6+ months

#

Having sub packages that make up discord.js

#

And we won’t go back

golden mortar
#

Why don’t we undo the switch to builders?

I think the major problem with builders is that it's almost entirely useless. Embeds, Formatters, etc. are exported from discord.js so there is next to no reason to even use builders in the first place. Add on slow zod validation and it's pretty bad for now.

outer raven
# wild flax And we won’t go back

that goal seems reasonable but I think you should look at the hassle that that has become. There are ways to do that properly, having all these inconsistencies and dirty patches is surely not one of them, so something should be changed IMO to improve maintenance overload while keeping functionality. It seems like the only goal you wanted to achieve with this has been the only thing this didn't achieve

wild flax
#

I don’t even know what hacky or dirty things you talk about

copper laurel
dawn merlin
outer raven
wild flax
#

Yeah that’s what I mean with most of you think too much in “discord.js”

outer raven
#

Because that's the package we use and that's where the issue lies

golden mortar
wild flax
#

Unsafe is fair, the getters were a good improvement after we’ve seen some usage

#

And allowing camelcase was always planned

#

So..?

dawn merlin
#

I can’t remember if we use them

copper laurel
#

The wrapper only exists to add back camelCase support, maybe .equals() too

golden mortar
dawn merlin
#

Tbh that should be replaced with the builders formatters

copper laurel
outer raven
copper laurel
#

Those aren't fixes

#

None of them are fixes

outer raven
copper laurel
#

None of them are hacks either

golden mortar
copper laurel
outer raven
golden mortar
#

of course something like equals could be handled by the user as well by extending the class

outer raven
copper laurel
golden mortar
dawn merlin
outer raven
copper laurel
#

Its just iterative development

golden mortar
copper laurel
#

Build something that can work alone, add additional features to bring up to parity

#

This happens all the time in the enterprise world where the 2.0 version of something won't have feature parity in it's initial release, but what it does have is done better

outer raven
# golden mortar why not? genuinely curious

Because quite a lot of other properties have those methods and the point of semver is to only have to adapt your code to work properly on semver major changes. If you have an equals method in your code you will have to update it on every minor version that adds a new prop

outer raven
#

it's better now

copper laurel
#

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

outer raven
#

but it's still far from perfect

woeful rain
copper laurel
#

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

outer raven
copper laurel
dawn merlin
#

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

outer raven
golden mortar
#

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

woeful rain
#

👍

dawn merlin
#

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

copper laurel
#

I'm really starting to lose understanding in these arguments

copper laurel
copper laurel
outer raven
copper laurel
#

No we dont

dawn merlin
#

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

outer raven
#

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

golden mortar
#

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

copper laurel
#

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

#

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

copper laurel
golden mortar
#

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

copper laurel
#

snake

#

builders are snake

golden mortar
#

yes my bad

copper laurel
#

Guess what

#

They always did that

#

MessageEmbed always accepted both in its constructor

outer raven
# copper laurel Nothing wrong with dependencies

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

wild flax
#

They do serve a purpose

golden mortar
#

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

#

however it won't ever receive camelcase

wild flax
#

Users who think less deps = better are not our concern

#

Never were

#

The argument is silly

copper laurel
golden mortar
#

I was mostly referring to the api

dawn merlin
#

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

copper laurel
#

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

outer raven
# wild flax The argument is silly

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

dawn merlin
#

Dude

#

It’s 16kb

golden mortar
#

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

copper laurel
#

Which is only half the picture

outer raven
# dawn merlin It’s 16kb

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

golden mortar
#

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

copper laurel
#

can be yes

wild flax
#

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

woeful rain
copper laurel
#

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

dawn merlin
copper laurel
#

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

#

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

dawn merlin
copper laurel
#

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

dawn merlin
#

I also never use builders i always use json.

#

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

golden mortar
#

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

copper laurel
#

But it isnt released

#

And isn't going to change

visual hornet
#

has duplicate properties with setters been discussed

golden mortar
#

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

dawn merlin
#

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

solemn oyster
golden mortar
#

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

dawn merlin
#

Aren’t you using unsafe builders anyways?

golden mortar
#

I'm using json

dawn merlin
#

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

golden mortar
#

yeah

#

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

solemn oyster
outer raven
solemn oyster
#

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

woeful rain
solemn oyster
#

Shapeshift

dawn merlin
outer raven
solemn oyster
#

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

golden mortar
#

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

dawn merlin
#

How often are you fetching dependencies rodry

solemn oyster
#

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

dawn merlin
outer raven
golden mortar
#

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

dawn merlin
#

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

golden mortar
#

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

dawn merlin
#

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

outer raven
#

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

golden mortar
#

eslint & typescript eslint is about 75% of my dependencies

woeful rain
dawn merlin
#

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

#

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

solemn oyster
#

And eslint installs lodash anyways

dawn merlin
#

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

solemn oyster
#

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

#

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

dawn merlin
#

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

solemn oyster
#

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

outer raven
#

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

dawn merlin
#

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

outer raven
#

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

dawn merlin
#

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

solemn oyster
#

Rodry, one thing

#

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

dawn merlin
#

I do mmLol

outer raven
solemn oyster
#

They use a lot more memory mmLol

#

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

outer raven
#

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

solemn oyster
#

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

dawn merlin
#

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

solemn oyster
#

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

#

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

#

Simply because it's an object with all the properties

outer raven
solemn oyster
#

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

#

They aren't

#

Not after the getters PR

dawn merlin
#

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

outer raven
#

Embed#addField to not take an object

#

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

copper laurel
#

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

outer raven
#

sometimes you only need one

copper laurel
#

Which is entirely supported by addFields

dawn merlin
outer raven
copper laurel
#

Theres no >=2 constraint on rest params

outer raven
dawn merlin
#

It’s getter does

#

It’s a js class

outer raven
copper laurel
#

I disagree, I like objects

#

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

outer raven
outer raven
dawn merlin
copper laurel
#

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

#

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

dawn merlin
#

Yeah I’ve personally found objects to be better here

outer raven
#

it does, for ease of code

copper laurel
#

Its easier to know one method adds fields to an embed

dawn merlin
#

Objects are more portable than parameters anyways

copper laurel
#

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

woeful rain
#

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

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

outer raven
dawn merlin
#

Also things like that neglect the maintainers pov

copper laurel
#

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

#

But its not a democracy

#

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

outer raven
woeful rain
copper laurel
#

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

dawn merlin
outer raven
outer raven
dawn merlin
copper laurel
#

I could make exactly the same claims in reverse though

#

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

#

Theyre just mad that it changed

dawn merlin
copper laurel
#

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

#

Because any major change would be voted against

woeful rain
outer raven
# copper laurel Every time we get a complaint about something we changed people refuse to listen...

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

copper laurel
#

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

wild flax
#

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

#

Yeah sure, someone prs something

#

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

outer raven
#

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

dawn merlin
#

which PR is that

wild flax
#

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

#

But still get applied and go through

#

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

outer raven
dawn merlin
outer raven
outer raven
dawn merlin
#

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

wild flax
#

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

#

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

#

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

outer raven
#

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

outer raven
wild flax
#

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

outer raven
#

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

wild flax
#

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

outer raven
#

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

#

there's both sides of the same coin

wild flax
#

It’s not like everyone uses that feature

#

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

outer raven
#

I do sometimes yeah

wild flax
#

Then it’s 8:15, much more closer

outer raven
#

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

wild flax
#

And we can disagree with those

#

Some of those arguments sound like we just do whatever lol

copper laurel
#

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

#

Or change their code style slightly

#

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

wild flax
#

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

#

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

outer raven
tacit crypt
#

our past record of breaking changes begs to differ with that

wild flax
#

Yeah I don’t understand the current outrage sorry

#

As much as I try to read into it

#

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

oak quail
#

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

opaque vessel
oak quail
oak quail
#

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

#

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

ruby terrace
#

you mean files :)

#

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

mighty sequoia
#

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

visual hornet
#

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

mighty sequoia
#

the commandName for the interaction

#

theres no commandName on the SelectMenuInteraction object

visual hornet
#

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

mighty sequoia
#

yeah

visual hornet
#

interaction !== interaction.message.interaction

mighty sequoia
#

I don't understand why thats normal

#

Yeah. I'm aware

#

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

visual hornet
visual hornet
mighty sequoia
#

how?

#

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

visual hornet
#

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

#

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

copper laurel
#

Yeah hold up, lets not argue this one

#

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

#

right?

#

Because it wont be defined unless the interactionType has a name

copper laurel
mighty sequoia
#

should have commandName in the dropdown

#

I went into debugger and it is there

copper laurel
#

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

mighty sequoia
copper laurel
#

I dont see APIMessageInteraction anywhere in the codebase

mighty sequoia
#

and I installed a few hours ago

copper laurel
#

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

#

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

ornate topaz
#

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

outer raven
#

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

outer raven
tacit crypt
#

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

opaque vessel
#

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

#

Put set positions and fetch active threads on it

#

Have the guild channel manager and category children one extend it

#

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

outer raven
#

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

outer raven
#

wouldn't help much

opaque vessel
#

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

outer raven
#

don't do what

solemn oyster
#

Wrong channel, @indigo bison.

indigo bison
#

whats the right channel?

solemn oyster
#

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

outer raven
opaque vessel
#

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

#

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

dry totem
#

I don't really understand how discordjs work

#

But it don't show me when it is triggered

solemn oyster
dry totem
#

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

#

When it has been started

opaque vessel
#

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

solemn oyster
outer raven
dry totem
solemn oyster
#

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

outer raven
#

What would we call it

opaque vessel
#

NonCategoryGuildBasedChannel, just like the types shrugs

#

I think it's called that

solemn oyster
#

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

#

I assume they do, but I want to make sure

#

Well, I guess CI will tell us

outer raven
#

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

outer raven
#

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

#

if you wanna be 100% accurate that is

solemn oyster
#

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

outer raven
#

Should I say holds instead?

solemn oyster
#

computes?

#

Or just don't mention it at all

#

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

outer raven
#

I just copied the style from other managers

solemn oyster
#

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

outer raven
#

Nitpicking at it’s finest kek

#

Ill just remove the cache part

#

Done

outer raven
#

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

wild flax
#

If tests are being changed it should be reviewed too

#

It doesn’t matter

#

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

#

Thanks discord

outer raven
#

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

wild flax
#

Seen them plenty out there

outer raven
#

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

solemn oyster
#

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

wintry pendant
outer raven
#

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

dawn merlin
outer raven
#

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

outer raven
#

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

copper laurel
#

npm link works for me

outer raven
#

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

wild flax
#

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

#

Because links or deps could have changed

outer raven
#

I did, my issue is in the link command

#

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

wild flax
#

Just link npm link usually

outer raven
#

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

wild flax
#

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

outer raven
#

if I do that it tells me this

#

that was the v1 syntax

wild flax
#

You could probably also just try npm link

#

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

outer raven
wild flax
#

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

outer raven
#

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

wild flax
#

Yes

outer raven
#

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

wild flax
#

That is indeed a problem

outer raven
#

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

#

would it be impossible in that case?

wild flax
#

Could be

#

But it is better

#

Just use the node-linker mode instead of pnp

#

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

outer raven
#

what's that?

copper laurel
outer raven
#

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

copper laurel
#

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

outer raven
#

I am yeah

wild flax
#

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

#

Run npm link

#

And then your project

#

Run the same command

outer raven
#

the typings file is correct but full of errors

manic cairn
copper laurel
#

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

#

Im literally just following one instruction

wild flax
#

since you moved it already

outer raven
wild flax
#

Yeah probably if I'm honest

#

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

outer raven
#

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

wild flax
#

this would be one

outer raven
#

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

analog oyster
#

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

#

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

wild flax
#

No, I think thats ok

analog oyster
#

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

opaque vessel
#

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

outer raven
#

Psst @solemn oyster webhook handlers?

outer raven
opaque vessel
#

Including reason in the JSON params

outer raven
#

Ah yeah alright

solemn oyster
outer raven
#

Isn’t it websocket?

solemn oyster
#

There, fixed

#

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

outer raven
#

Lmao yeah I figured

#

Happens to the best of us

solemn oyster
#

We talk about webhooks more than websockets tho

outer raven
#

True lol

outer raven
#

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

zenith oracle
#

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

slate nacelle
#

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

outer raven
slate nacelle
#

PartialGroupDMChannel has no text capabilities.

#

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

outer raven
slate nacelle
#

I think so

outer raven
#

Alright, will do

outer raven
#

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

wild flax
#

Sure

outer raven
#

oh great, that's nice to know

#

to the v13 branch, not stable right?

wild flax
#

Yes

outer raven
#

alright

opaque vessel
outer raven
#

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

charred mural
#

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

copper laurel
#

Is it actually one hour though? Or is 10500 accurate

charred mural
#

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

copper laurel
#

It should be from the headers

charred mural
#

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

#

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

#

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

copper laurel
#

The correct period being one hour?

charred mural
#

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

#

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

copper laurel
#

Thats not how a rate limit should work to my knowledge

#

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

charred mural
#

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

#

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

#

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

wild flax
#

I could lie but

#

There might be 2 limits in place

#

1/10s and then 10/1h

charred mural
#

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

#

Could be they changed more stuff with that

wild flax
#

10500s is also like 3h or smth

#

So not sure what’s up with that

charred mural
#

It's 10500ms, not s

#

So 10,5s

wild flax
#

Yeah just thinking out loud

charred mural
#

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

#

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

#

timeout in RateLimitData (in ms)

#

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

void rivet
#

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

#

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

dawn merlin
#

Good point

#

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

void rivet
#

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

golden mortar
#

or it could just be left alone

dawn merlin
golden mortar
#

you can add to builders without removing from djs

void rivet
#

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

golden mortar
#

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

void rivet
#

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

dawn merlin
golden mortar
#

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

#

also would builders' attachment support Buffer/node streams?

#

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

dawn merlin
golden mortar
#

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

#

it's not really a structure that constantly changes

#

idk why im arguing because the unsafeattachment would be good anyways

#

im just wondering if it would support djs features

dawn merlin
dawn merlin
golden mortar
#

also please support Blob and other new things please 🤞

dawn merlin
#

Is blob in node now?

golden mortar
#

const { Blob } = require('buffer')

dawn merlin
#

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

golden mortar
#

since v15, backported to 14.18

#

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

new MessageAttachment(blob, 'name.something')

dawn merlin
#

Well yeah considering that standard fetch uses a blob

#

Will the undici merge add Formdata to the node scope?

golden mortar
#

eventually but not in v17.xx afaik

#

it's not in core yet

dawn merlin
#

Epic

golden mortar
#

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

#

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

dawn merlin
#

We’re still using node-fetch so kek

golden mortar
#

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

#

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

wild flax
#

I think it does

golden mortar
#

oh cool, they have their own blob implementation

dawn merlin
#

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

#

Well for our use cases at least

golden mortar
#

yes - I implemented in my original undici pr actually

dawn merlin
#

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

golden mortar
#

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

dawn merlin
#

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

#

But that’s a large refactor for them

golden mortar
#

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

#

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

dawn merlin
#

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

opaque vessel
#

Can't you re-run the action

golden mortar
#

I forgot I could push an empty commit

#

maybe it's better to remove those tests?

dawn merlin
#

Iirc

copper laurel
#

You can push an empty commit to trigger it again

ruby terrace
ruby terrace
ruby terrace
analog oyster
golden mortar
#

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

dawn merlin
#

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

golden mortar
#

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

golden mortar
#

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

dawn merlin
#

wdym uploaded like local file paths?

golden mortar
#

if someone has a Buffer/etc they want to upload

#

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

dawn merlin
#

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

honest barn
#

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

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

dawn merlin
#

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

golden mortar
dawn merlin
golden mortar
#

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

dawn merlin
#

ah

golden mortar
#

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

dawn merlin
#

so more of toData rather than toJSON?

golden mortar
#

that could work, yeah

#

toJSON for the APIAttachment, toData for the attachment data

dawn merlin
#

read my mind

golden mortar
#

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

analog oyster
#

filename is 1024

#

description is also 1024

golden mortar
#

thanks 🙏

golden mortar
#

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

#

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

dawn merlin
#

or calling toJSON doesn't do anything?

#

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

golden mortar
#

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

#

it was on djs' side

#

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

dawn merlin
#

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

golden mortar
#

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

dawn merlin
#

it should be isJSONEncodable(embed)

sullen marlin
#

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

copper laurel
#

Thats an API option

#

Its not something discord.js custom implements

sullen marlin
#

oh, didnt knew that. Thanks!

copper laurel
#

e.g. the rejectOnRateLimit option

#

Which makes rate limited promises throw instead of queuing

#

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

charred mural
stoic olive
#

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

wild flax
#

Buttons don’t expire

stoic olive
wild flax
#

That doesn’t make the button expire

#

Only our collector

dawn merlin
stoic olive
wintry pendant
#

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

zenith oracle
#

Wdym? Getters are read-only...

wintry pendant
#

right but why don't you make them actually getters

slate nacelle
#

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

analog oyster
#

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

outer raven
#

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

#

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

vernal rose
#

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

wild flax
#

Won’t work in our setup

ruby terrace
charred mural
#

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

wintry pendant
#

is this undocumented for a reason?

slate nacelle
#

I don't think so.

outer raven
#

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

dawn merlin
outer raven
#

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

dawn merlin
outer raven
dawn merlin
#

They’re not hard to use at all

outer raven
#

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

wild flax
#

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

#

Just to correct that statement

outer raven
#

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

#

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

#

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

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

That’s why you don’t use magic numbers

outer raven
#

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

dawn merlin
#

Yes hence enums

outer raven
#

did you read what i just said or

#

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

dawn merlin
#

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

#

It’s the same thing it sounds like?

#

And the magic number argument still doesn’t make sense

outer raven
#

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

#

so you have to transform them

#

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

dawn merlin
#

Yeah, and we chose to use enums

#

It was deliberate

wild flax
#

I don’t want strings in the lib

#

I will say it till people get it

outer raven
#

but why don't you want them

wild flax
#

No matter if it’s OUR Interface or discords

outer raven
#

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

#

partials isnt even a class

#

its just an enum

#

it's impractical to use

wild flax
#

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

#

It’s basic api design

outer raven
#

right now you don't have a consistent interface

wild flax
#

Because 14 is in development

#

We do iterative programming

dawn merlin
#

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

wild flax
#

Not “all at once”

outer raven
#

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

dawn merlin
#

Which PR was that?

outer raven
dawn merlin
#

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

wintry pendant
dawn merlin
#

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

#

Otherwise it would’ve been stuck in rebase hell

outer raven
#

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

dawn merlin
loud jayBOT
outer raven
#

nope, it isn't

#

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

dawn merlin
#

Should be, the latest version has the same git hash

outer raven
#

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

#

I currently have 0.13.0-dev.1644797188.0dfdb2c installed

solemn oyster
#

Probably due to cache

outer raven
#

how could I update them then?

wild flax
#

You’ll have to use resolutions

outer raven
#

how?

wild flax
#

Not supported by npm I think

#

But yarn has it

outer raven
#

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