Proposal - revert PR 7584

[forest jacinth]

So PR 7584 has caused a lot of issues for me personally and I'm sure I'm not alone since I've seen some reactions on some discussions about this same topic. Basically that PR made it impossible to find a builder and modify it from an array of builder classes for example, or any place where you're not aware of what data you have and where. I, for example, have a system where an indefinite amount of buttons is pushed to an array and then action rows are made out of them, and later, once each button is clicked, I have to disable them and update them in the message on Discord. This has been made impossible since you can't use the built structures to find a property with a certain name, nor can you modify the received structures after finding the one you want.
I initially proposed to add back getters in a PR that I later closed for reasons I now regret but I believe it is better to revert the PR entirely, as it, in my opinion, simply adds clutter and complicates a lot of simple code bases and makes situations where we are agnostic of the data we are building (when it comes from external resources) and want to modify it. I would like to hear from the maintainers on this to know if this is the desired approach or not, so that we can discuss a solution that fits everyone.
PR link for reference: https://github.com/discordjs/discord.js/pull/7584

[ancient plank]

yes please

I don't really care about the reasons for needing it reverted, but the PR made everything massively annoying to use

[forest jacinth]

yep that was the reaction I was talking about

[ancient ruin]

I've provided solutions to all the problems that you've faced that are less lines of code and more efficient but you've failed to accept them for some reason. And would prefer to repurpose custom ID's as a references and action rows as maps. All of which are wrong use cases. Furthermore the old behaviors allows users to modify the cache directly whereas it's not possible now. In addition, some builder structures aren't symmetrical when sent back in forth such as TextInputComponent, so representing them as a single structure makes no sense anyways. I want to reiterate that a bunch of other discord libraries don't have getters on their builders, so you'll also need to explain why discord.js is an exception here.

I was blatantly told that this solution was stupid? Because I had to use a map instead of iterating through an entire action row everytime I needed to access an element

[ancient plank]

so you'll also need to explain why discord.js is an exception here.
cuz its js and not java or php or rust or whatever

[ancient ruin]

but the design pattern is the same

you don't have to use builders we have json

[forest jacinth]

The issue with discord.js is that these classes have always worked both ways and were never explicitly and exclusively builders. You’re always bringing up other libs from other languages when in reality that shouldn’t matter at all. This is JS and this is discord.js. The users are used to a specific behavior and will not like this transition as it makes stuff unnecessarily complicated and the average user will not be able to upgrade seamlessly.
That solution you gave me might work and might even look better, but this isn’t and cannot be about me. If I had these issues in 3 different places in my relatively small bot who knows what other people with bigger bots on more servers will face. In my case I can stick to customIds just fine, but what if someone else needs that AND a label? They just can’t do that now with this split

[ancient ruin]

The answer is to not use custom_id's as a replacement for references and to not use action rows as replacement for maps. In essence use the data structures appropriate for the task

[forest jacinth]

And you might argue that you’d be able to find solutions for the other users but would you be able to do that for 100.000 people when v14 launches publicly? You really cannot stick to one case only because we’re working on a dev version used by very few and on which even fewer give feedback

[ancient ruin]

Yuudatchi our moderation bot has no issues despite it heavily using componentns

[forest jacinth]

Again, you’re thinking of my example, think outside the box

Literally anything is possible

[ancient ruin]

yes and my answer can be generalized

The answer is to not use custom_id's as a replacement for references and to not use action rows as replacement for maps. In essence use the data structures appropriate for the task

[forest jacinth]

now go tell 100k people that

[ancient ruin]

also regarding languages, it was asked in the TS server and response was pretty much the same thing I was thinking builders shouldn't have getters.

[ancient ruin]

ok if I have to I will

it's just a breaking change

like every other rename or removal

in fact I might add a section in the upgrade guide

in similiar fashion to how the removal of .deleted was presented

[forest jacinth]

can you link me that conversation

[ancient ruin]

#640177484913967105 message

[forest jacinth]

you got an answer from 1 person that is disregarding agnostic functions, they always assume you instantly know everything you put into a builder, which may not always be true, although it will be 90% of the time

[ancient ruin]

you missed the "short lived" aspect of that comment

[tardy bison]

Could someone show a earlier - now code?
want to judge it neutrally

[ancient ruin]

sure

I can give two examples actually

[forest jacinth]

how does that matter lol

[ancient ruin]

example 1: https://github.com/Hypixel-Translators/hypixel-translators-bot/pull/682/files
example 2: https://github.com/Hypixel-Translators/hypixel-translators-bot/compare/main...suneettipirneni:refactor/prefix?expand=1

[forest jacinth]

I don't think those are the examples he was looking for, they're different ways to do the same thing but they don't really touch the core issue with the props

[ancient plank]

i havent updated djs dev since modals, can u do this for my bot too lmao

[ancient ruin]

sure

[tardy bison]

Does Builders just accept json?

[ancient ruin]

Yes

[tardy bison]

Btw the .get option seems to be more suitable, better for typescript and less code

[ancient ruin]

Wdym

[tardy bison]

I probably have to make a own builder now lol

[ancient ruin]

Oh for the map yeah

[tardy bison]

1 vs. 2, 2 wins

[tardy bison]

Does this also accounts for embeds?

[ancient ruin]

yes

[shell lantern]

Tiny bit off topic but still on the topic of the PR, but I don't see why this couldn't be supported with generics or a different type altogether?

Is it seriously impossible to represent a value as non null there?

I feel like a lot of decisions the lib has been making are making developers have to work harder not smarter, by writing more code for little to no gain in devex.

[ancient ruin]

yes it is impossible because if it was non-null, the types would be inaccurate

it's holds partial API data

it's even worse with text input components that returns data in a different shape than it's builder

[shell lantern]

Builders select menu component's customId is optional when it's always non-null from the API

so which is it

[ancient ruin]

literally what I said? its optional

[ancient plank]

it is/was nullable in the builder but not the discord api data

[shell lantern]

Could you not support a generic or use a different altogether to represent embeds specifically in message.embeds?

[ancient ruin]

the core issue is that the received components would mark types as optional when in reality they could never be

[shell lantern]

Or whatever.embeds*

[tardy bison]

I can agree to this, I now have custom made builders for embeds and components. Rather outsourcing your code, you do the opposite, when there are changes.

But new users should use the best practice.

[ancient ruin]

no, bc those still allow mutation of the cache

[shell lantern]

so it'd fix the original issue though

[ancient ruin]

what?

[ancient plank]

and you could just add a clone method

[ancient ruin]

that was actually brought up internally, clone method still doesn't address mismatched received types and shapes

[ancient plank]

generic & clone

[ancient ruin]

yeah but generics still don't address shape changes. They can only address types

and you can make data from the API mutable anyways via Builder.from(receivedComponent)

also massive amounts of js users can ignore those types without even noticing or incurring an error of any kind

[shell lantern]

And? People that wouldn't notice the changes aren't noticing the current issue anyways

[ancient plank]

I never had issues with custom_id being nullable in ts before the change

a non-null assertion takes care of it for the few times I need the id

[ancient ruin]

point is you shouldn't need a non null assertion to begin with bc it's already well-defined that it won't be null

[ancient plank]

is a single operator in ts only land really worth it

[ancient ruin]

and again what about structures like TextInputs that have different structures

[shell lantern]

What about interaction.client.user being nullable, yet it cannot be null?

[ancient ruin]

it can be?

if you're not a bot user it can be null

[shell lantern]

its impossible to use djs without a token

and to get a token you need a bot application

[ancient ruin]

you know you don't need a bot user for slash commands right?

[shell lantern]

You do if you want to use discord.js, the library we're using

[ancient ruin]

nope

that's just incorrect

[shell lantern]

Really? Spin up a working djs bot that doesn't use a bot application and send me a link to the repo

[ancient ruin]

I mean I can, but like I'm not going to do that right this minute

[shell lantern]

Do it when you can and ping me, i'll bet $5 you can't lol

[ancient ruin]

sweet sounds good

[ancient ruin]

also incorrect terms here, bot user*

[shell lantern]

Yeah, I meant making the bot login and receive interactions without clicking this button

[ancient ruin]

that has nothing to do with client.interaction.user being null or not.

[shell lantern]

djs communicates only over ws

to communicate over ws you need a token, and then you'll have a client instance. its not pulling the client from each interaction, the client is constantly passed around to every part of the app.

[ancient ruin]

alright this is going wildly off-topic

go ask in #djs-help-v14 or #943190505720786985 if you can host slash commands without a bot user

[forest jacinth]

all props in a builder are null anyways, that can just be a note in the docs, simple as that

you can even cast the types

have a type generic boolean called Received

[stuck hornet]

some of the typeguards in the interaction class could narrow the type of the client prop

[forest jacinth]

if true, some props will be of type never

[ancient ruin]

always null props sounds useless and confusing especially for users using autocomplete in their editor. And that still doesn't account for the fact that people can mutate cache. Sure you can type narrow the returned builder, but most people that are using js they'd still modify cache without any form of an error

furthermore as I stated earlier and in my PR it's not the responsibility of a builder to represent data, why? bc it's a shitty container for representation as it's mutable. It also would have two responsibilities of partial construction and representation. They're being stretched too thin. In general classes should be good at a single set of related responsibilities

[forest jacinth]

and why do you care if they modify the cache? I have yet to understand how someone would do that unknowingly but if they do that's their issue, why do you care?

the ! operator in TS exists for a reason. You simply have to know when a property can be null and when it can't, and add the appropriate checks to your code if needed

just because a prop can be null it doesn't mean it will always be null in every code sample

[ancient plank]

to me it seems like the solution was way over engineered for a problem that no one really had

[ancient ruin]

Bc they don’t own the cache and many times it’s being done unknowingly. djs cross references caches all the time. Mutation of caches and/or the desyncing of caches can lead to hard to diagnose issues. In fact we had an issue just today with something unknowningly modifying cache #958065189993975868 message

[ancient ruin]

Again the wrong use,

A new ! post-fix expression operator may be used to assert that its operand is non-null and non-undefined in contexts where the type checker is unable to conclude that fact.

• TS docs

We're more than capable of providing the fact that props are non-null without forcing everyone to use a non-null assertion

In fact the recommended typescript/eslint rules will default to warning whenever it's used

[ancient plank]

?

[limber lark]

?

I'm just going to dump my thoughts

• no I don't think it should be reverted, discord.js should not be using builders to represent received components
• the received components should be immutable
• maybe a .toBuilder() method could exist like a clone, idk. This would give you a mutable version without comprising cache, or types for nullable props, or difference in structure
• stop complaining about TS actually imposing strict types. If you don't want proper strict types, go back to JavaScript

• builders suck anyway

[hollow flume]

I for one don't see why we should revert the PR you linked. It finally shows the clear separation between data received from the api and a builder you may use to build raw data, including perks like validation and cleaner code, and theres literally a method to convert from data to builder

[limber lark]

Oh that method already exists, perfect. My third point stands

[forest jacinth]

im sorry that example just shows clear lack of knowledge. Everyone should know you're not supposed to be modifying stuff directly in a message, you're supposed to modify your own variables. Anyone with a good knowledge of JS and common sense would avoid that problem

[forest jacinth]

but why would you check if something is null if you're absolutely sure it will never be?

[ancient ruin]

So how does an autocompleted method like setX or addX on a recieved object help the situation?

[hollow flume]

if you know its not null, use ! lol

what is the end goal here

[limber lark]

The not-null assertion is for situations where the typing system cannot strictly type it as being not null even though youre sure it will never be. This is not that situation because we literally don't have that issue in the current split.

Planning to intentionally use not-null simply to make code easier or cleaner is an improper use of TS

[forest jacinth]

if you have an array of received buttons you can't modify them all, even with that method. You either access their properties or modify them, you can't have both and that's the issue I'm trying to get at here. All that PR did was separate something we had in 1 class into two, generating more confusion among end-users who will be confused as to why they can't construct the new classes and when they figure out they need to use the builders they will be confused why they can't access their properties. Again I mention: if I, with a small bot, faced 3 different situations where I couldn't convert my code how many more will thousands of others face?

[forest jacinth]

that is what im saying

[hollow flume]

i'm sorry to say it bluntly, but you're literally the only one with that issue...

[forest jacinth]

that situation should not exist to begin with. You're not supposed to modify message.components, period

[limber lark]

So is part of the issue that you cannot see the set properties on a builder? Thats the getters PR you closed?

[hollow flume]

even with that method wrong? Once you build a ButtonBuilder from the data, call .setWhtever

[forest jacinth]

so far, you're on an unstable dev release cycle with few users, I'm curious to see what will happen the day v14 comes out

[ancient ruin]

I agree so why have methods on it that allow modification

[forest jacinth]

if you read my initial message I said I later regretted closing it and considered reopening it but thought this would be a better approach

[limber lark]

Well that I completely disagree with, I'd lean heavily to the getters

[forest jacinth]

yeah and how do you put that back in the array of buttons you had? You have to convert say 10 buttons to edit 1 and send them all back

[hollow flume]

The getters are just a lazy shortcut to builder.data

[hollow flume]

no you don't

[forest jacinth]

if users are stupid let them be stupid, why do you care

[limber lark]

Yeah but camelCase is very important supposedly, that's why the getters existed

[forest jacinth]

wrong, builder.data is snake_case

[hollow flume]

Builders always have and should always be JUST taking in chained methods and returning api data

[limber lark]

I mean it's not wrong, there's just an additional factor to consider

It's a lazy camelCase shortcut

[forest jacinth]

yeah... exactly?

[forest jacinth]

if builder.data is snake_case it should be private and not used by the end user. The whole goal of that PR was to make .data completely inaccessible

[limber lark]

Make one data getter that returns the object in camelCase

Change the other to _data

[hollow flume]

const actionRow = ActionRowBuilder.from(message.components[0]);

const buttonIndex = actionRow.components.findIndex(item => item.data.custom_id === '123 bite me please');
const newButton = ButtonBuilder.from(actionRow.components[buttonIndex]).setDisabled(true);

actionRow.components[buttonIndex] = newButton;

await interaction.editReply({ components: [actionRow] })

[forest jacinth]

that's just stupid man

[hollow flume]

also no??

[limber lark]

I know lol

[hollow flume]

look at slash commands

all fields are exposed in snake case

as they should be

[limber lark]

Isn't part of the issue that actionRow.components isn't directly accessible/editable in builders?

[forest jacinth]

this is so much more complicated than simply editing a button on an already existing action row, and much more memory consuming too since you're creating 2 whole new classes

[hollow flume]

oh boo hoo

they don't even stay in ram for long

come on

thats like crying that validation slows down creation by a few nanoseconds

actually technically

it'd be more like

const actionRow = ActionRowBuilder.from(message.components[0]);

actionRow.components.find(item => item.data.custom_id === '123 bite me please')?.setDisabled(true);

await interaction.editReply({ components: [actionRow] })

or it should be

[forest jacinth]

no?

because the buttons inside the action row are immutable

[hollow flume]

read again rodry

read again

const actionRow = ActionRowBuilder

[forest jacinth]

yeah the row is a builder, the components inside of it aren't

right?

[hollow flume]

well for one that method doesn't exist yet because

but if it did exist it'd convert data to builders

[hollow flume]

I see now... it should be like... public readonly

its not

[limber lark]

But if it was your whole example doesn't work?

Wait, read-only on that prop but not the nested components?

[hollow flume]

yes

[limber lark]

Gotcha

[hollow flume]

if the .from method existed and components was declared as a public readonly field (so you can't do row.components = []) it would work

[limber lark]

And the things in the components array were also builders

[hollow flume]

hmm

sec

[ancient ruin]

Wrong

[forest jacinth]

wdym if it existed... doesn't it already?

[forest jacinth]

sure?

[ancient ruin]

Yes bc I coded it to be that way