Readonly array in message options interface
Hey @Almeida I just saw your PR https://github.com/discordjs/discord.js/pull/10045 which broke my code. I'm unsure as to what the reason behind this is since an interface like BaseMessageOptions (the one inpacting me in particular) is meant to be the data sent to Discord, so it should be able to be modified. I'm trying to push components to a components array in a variable of type InteractionUpdateOptions, how should that be done after this change?
62 Replies
- What's your exact discord.js
npm list discord.js and node node -v version?
- Not a discord.js issue? Check out #other-js-ts.
- Consider reading #how-to-get-help to improve your question!
- Explain what exactly your issue is.
- Post the full error stack trace, not just the top part!
- Show your code!
- Issue solved? Press the button!
- ✅ Marked as resolved by staffAlso please tag me when you respond!
I think this was made to make it possible to pass readonly arrays to the options
but this change as it sits restricts those arrays to always being readonly
why would I do that? What do you gain from this?
doing that just means having to store another global variable for no reason
well not global, but high scope
as syjalo said, that change allows passing readonly arrays whenever discord.js does not require a mutable reference (so pretty much everywhere)
a mutable array can also be passed, as evidenced by my code sample
but this doesn't simply allow that, it also makes it impossible to pass mutable arrays
but having to store another variable essentially creates two sources of truth for the same thing which is not good practice
or rather, mutating the array after creating the variable, which I don't think should be a restriction
it does not, given that
components shares the same reference with responseData.componentsbut it means having to change the same thing twice
changing the components on one variable and then the other options in another one
it's just unnecessary extra work
small price to pay for correctness imo
what correctess though? What error are you avoiding with this?
already mentioned twice
you're adding support for a niche case while dropping support for one that seems much more common
I agree that should be supported, but in a way that doesn't make it a breaking change
and this was deployed in a minor version too and it's a heavily breaking change, it envolves changing code and not just typings
unless I just decide to ignore this and force the type, which isn't good practice either
Can you use satisfies?
fairly certain changes to the types in discord.js do not follow semver
not really since this is a variable (not a constant) and can also be null
it's a type change that directly affects the code and requires more than just type changes
furthermore this code I'm showing is perfectly valid in JS, the error created by the type change doesn't exist in reality
and in my opinion avoid it doesnt make the code any more or less correct because this is a variable I created and I know exactly the types of its properties
feel free to adjust your types as you need to
but this is a change that was incorrectly done on the package side and wasn't even properly explained in the PR. I once again emphasize that the use case it's trying to fix is valid, but it shouldn't be done in a way that breaks existing, equally valid, setups
I shouldn't need to be forcing the types of something that I know exactly what type it is
I'm considering opening a PR to revert the changes to interfaces made by this PR, leaving the methods as they are since those shouldn't have this problem
but interfaces from djs are often used by devs to type their own variables and not just used internally or as input types to methods
For me this is not a breaking change, since
BaseMessageOptions is what discord.js expects to get in options and the change just extended it allowing readonly arrays to be used. You can use your own type, TypeScript is not that strict in types.it didn't extend, it forced the array to be readonly by adding the keyword
or another option could be having a union type of array or readonly array, something like this
this will not solve your issue
What do you mean? I can use both
that's once again not what I'm saying. I'm talking about a use case where you have the object typed as BaseMessageOptions in your code and you try to push to its components or embeds property. You cannot because that array is now readonly, even though it isn't. Despite it accepting both, when accessing it's always treated as readonly
because accessing it would represent it being accessed by discord.js
but it shouldn't, I should be able to access that just fine in my code because this interface is exported
if it was to be used by djs only it wouldn't be exported
another option would be to have two interfaces, one with mutable arrays which is exported and the other one with imutable arrays which is only used in methods that accept that type
the reason I want this to be fixed in the package itself is mostly because the type of this property is so complex that having to force the type of my variable to that would likely have issues in the future once it is updated and it would be difficult to track down
@Almeida would you be able to do something like this in a PR?
i think you underestimate the amount of interfaces in discord.js
the changes in your PR weren't that many
i dont believe that is a good solution either way
will only cause more confusion to the end user
then what is a good solution? I've only seen you throw down all the options I'm giving even though you caused the issue
because i dont believe this to be a big issue (if at all). besides, this can easily be "worked around" on your end
how so? I'm not gonna be storing two variables that's ridiculous
besides, i am not a maintainer, so feel free to make an issue or something so that maintainers can share their opinion
you're close enough
there's barely a distinction between who is and who isn't here
but we can also bring @vladdy who threw some eyes on another person that commented on that being a breaking change in that PR
i don't think there's one person here that'll approve it
lol
i'm sorry man but your attitude just sucks, you're constantly so entitled it's hard to have a conversation
but to get on topic
the types are more correct as they are now and that's what's most important
there were times we didn't follow that line of thought like , not even particularly because it was inconvenient/not the best DX, but because it actually encouraged you to
as cast so often it caused more harm than good
in this case
you get just a little cooked but it's more than acceptable by probably everyone's standards :meguFace:I'm not entitled to anything, I'm simply exposing my situatio and trying to get you to understand the issue while also trying to find a solution, you're the ones who are so close-minded you only think about what's right for you and not for others
the thing is that this change is barely benefitting anyone because most people don't go around declaring readonly arrays everywhere they go because it's just adding more clutter to the code and serving little to no purpose. I understand it might be "technically" correct but in my use-case it isn't because I need to be able to modify my code the way I want. I get that djs shouldn't modify the inputs people throw in but even that I have a hard time believing that it's actually followed since there are many transformations happening to inputs behind the scenes
I genuinely understand what the goal of this change was, I just don't see any real benefits from it apart from theoretical correctness (and even that is debatable)
there are many transformations happening to inputs behind the scenesCan you please say where? And those inputs are typed as readonly?
just in the context of messages, the files are parsed and embeds and components are turned from builders to objects when applicable
but that was not the main point of that message anyway
Packages set a standard and people have to adapt to it, not the other way around, packages can't be changing things just because one person said so, if the package doesn't align with your goals, you shouldn't use it
I don't see the original properties being modified anywhere, can you send what exactly you're talking about?
well it's not like there's much of an alternative is there
once again that is not the main point I'm focusing on, I'm not trying to argue whether this change makes sense or not, I'm trying to draw attention to the issues it created and trying to find a solution
You shouldn't claim stuff you can't back :shrug:
I am saying the principle behind the change makes sense, but it creates this issue which cannot be avoided in any way other than by forcing types, which is unsafe and you shouldn't be promoting that. Other than that, this change benefits close to no one because most people don't use the readonly keyword in a discord bot, just look at your average user here and you can see that very clearly
My point is that, for many methods, what you pass to djs isn't what discord receives - just look at files for example - and that's fine, but you're completely replacing what the user sent with a parsed version, that doesn't seem very readonly to me
Well if discord.js does nothing, what's the point of using it? read-only means that the parameter you pass in can't and won't be modified, it can clone it and make changes to that clone just fine, no issues there, and that's what discord.js does
where is this done while mutating the original objects
Can we stay on the topic?
this is literally the topic
you made a point that the types aren't even right because we mutate internally
no I didn't. I've literally been saying it makes sense, it just doesn't add anything valuable to the DX because we don't use readonly stuff often in a discord bot and the average user in this server doesn't know what that is. I mentioned it may not be 100% correct due to some edge cases where the arrays are modified but I don't have the data or the exact lines to show you nor do I give a shit, so let's just talk about what matters shall we
yes, people don't often explicitly annotate with
readonly because it's typically optional, but saying "we don't use readonly stuff often in a discord bot" and "the average user in this server doesn't know what that is" is completely made up
its projection, probably, not something you'd know so confidently to assert either wayyou know it's true though, just open #djs-help-v14 for 5 minutes
that doesn't tell me anything, looking at just that channel as a tell is inherently based
surely you're old enough to understand that
or intelligent enough, age shouldn't have much to do with it
no one's gonna come to support and go "I got this and I agree with it and I know what it means and I fixed it myself", that wouldn't even be productive. you're only gonna see the confused & angry people there, which is really safe to assume are a minority
moving on, if the primary means of designing APIs was around some alleged "skill level" or level of knowledge of the library's userbase
we might as well just not do anything
"average joe doesn't know what a readonly is" isn't a good reason to drop correctness
that's a ridiculous mentality
don't use typescript or tone down the strictness if you don't want to deal with that sort of thing
its that easy
not to mention more and more people use
as const and satisfies and other TS features that have come out in recent years
that help infer the readonly fields for you anywayI would just like to know what you're actually gaining from this change because I just wrote this on 14.14.1 and it worked with no issues
I think this is all just a bunch of theoretical correctness that doesn't translate to practicality or any actual real benefit in the real world
feel free to prove me otherwise
None of this was even mentioned in the PR, it just said "we're adding this rule because yeah" and went on with it, no before and after or even a motivation behind the change
when you don't do extensive testing when changing types in this manner it makes sense that it has unwanted side-effects
it does, as expected, not work in version 14.14.1
that's probably a tsconfig rule then because it literally does not error for me as you can see right above
im afraid not
its more likely that you were not using version 14.14.1 in your screenshot
I am
I'm sure of that
idk what else you want me to show you, it literally does not error
yeah no this is you doing smth wrong
everyone else can repro almeida's
Yep, even without a tsconfig
I didn't even picked that version I just checked whatever my test bot had
I do have a tsconfig I can even send it here if you really want
ok man
im convinced you just think everyone here is a moron
or that your ego is just this big
this isn't worth my time anymore, i'm out
Yes, this PR broke my code base as well. It should not have been approved for a non-major release. I think the code author is likely just inexperienced and doesn’t fully understand the change they made.
It’s a breaking change.
I’m not attacking them, I’m explaining why I think a breaking change like that would be submitted and allowed into the code base. If I use discord.js’s typescript package in my application, I expect not to have the typing introduce breaking changes on non-major releases. Types aren’t added for convenience, they’re added to support compiler-enforced data invariants.
The types are not very convenient when they break a code base. But fair enough, I will just lock the version in the future and expect other breaking changes in my typescript code base if I ever need to up version.