-
-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(Structures): remove Structures #6027
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reflect.defineProperty(Message.prototype, 'property', { get() { /* ... */ } });
goes brr
Because we didn't have enough breaking changes already. 📦 |
👎 please don’t |
👍 Completely break commando and any bots that use |
I see how people can get mad at this, but I personally agree. A library should not let its users touch to its own structures, because as @1Computer1 said it is likely for people to break stuff. One example, that doesn't run into the Structures case but still, is extending Now, if you used to have Also, it requires you to overwrite discord.js typings for your IDE to work with them properly, which I've never found a good thing to do (no matter what library we're doing that to). When using your own structures, you perfectly know how they work, you have full control over the name you give to your properties, and you're guaranteed to not break discord.js any time (at least on that side). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
True |
Sorry to break it for you, but you're a bit (+10 semver-major PRs) late, Commando doesn't work with v13 at all, not even without this PR.
I repeated this like a lot of times, you don't override types, you can't. The correct way to do this is to use module augmentation, which works quite flawlessly. |
This reverts commit ab0b3b9.
🔥 |
I'm not even going to complain. Also this is probably the best time to delete it. Creating a new version is anyway mostly full of breaking changes and other libraries have a lot of time to become compatible to v13. I mean personally i had to rewrite a lot of stuff so i've experienced the problems. |
Please don't remove Structures. My bot's entirely dependent on it. If someone adds something that doesn't work, its them to blame, not you. They should've known what they're messing with before updating the structure. |
The number of people who want structures are larger than the number of people who don't. |
They already did... |
I'd imagine most bots are also dependant on the |
Well it's not the same... |
Being honest here, I have used and abused structures to hell and forth since they were ever released in v12's master branch, I also personally use Like you, I used to support Structures, and did so for quite some time, however, as much as it pains me (specially since I have to rewrite my bot, which has around 71000 lines of code in over 600 files), the issues that came with Structures outweight the benefits, and as such, it's better to have them removed. Between the issues that came with Structures:
I hope this is enough to explain all the issues this system has, I commented earlier in #6039 (comment) but I felt like I had to sum up and clarify some information around this. Now, I would like you to take a look at our Code of Conduct before commenting any further on this, it's not helpful at all to receive an email or a notification in a thread that's "+1", "-1", "don't do this", "I prefer doing If you have a good proposal that solves all the aforementioned issues, please open a new discussion in this repository or tell us about it in the |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Since the structures are removed can we get a more structurized way rather than using "functions" |
Without the issues mentioned in my previous comment? I don't think it's really possible. Also, given how much inheritance discord.js has, the functional way of doing things to extend the library's functionality is very good, for example, if you added a getter or a method in your channels to send localized input, you'd need to extend Since functions don't rely on that, you can seamlessly make it so it works for any channel, current and future, without extra maintenance needed in your end. Also, the difference between As @1Computer1 mentioned several times, the only benefit of structures is the usage of the I hope this is understandable for you, and for future reference, please feel free to talk further about this in the official Discord server. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Amazing.. time to take my bot down for DAYS to fix this mess. Thanks DJS. Mentioned above was that no other library does this. So what? Is it wrong for DJS to be unique or there is some unspoken rule that you cannot do something different than other "big" libraries in the ecosystem? Even if it was removed due to maintenance burden... just why? Why would someone remove a well-liked feature which a lot of projects depend on, just because the team wants to make their lives easier? |
Sounds a great plan to me :D |
Commando is deprecated, and doesn't support V13, it is recommended that you use Sapphire instead. As stated in the original description, they are impossible to maintain, unpredictable, and still can be easily implemented using databases. Using structures is a personal preference, and complicates support. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This reverts commit ab0b3b9.
uses WeakMap so that when a djs object is dropped its associated value will be as well. See discordjs/discord.js#6027 (comment)
Please describe the changes this PR makes and why it should be merged:
Structures is not good for several reasons:
It inverses the control of the data types in the library. This means if you add something new, the library cannot guarantee what you added works with the library. Most prominently, if property or method names conflict, everything breaks.
It is not composable. If multiple extensions are added at once, there's no guarantee those work either, for similar reasons. In essence, no one owns the type, so no one can be sure it works.
There are established ways of doing "adding new functionality" that does not require this hack. And everyone knows it, they're just regular ol functions.
It is much better to do something like this:
A more concrete example:
To:
Basically, remove this mess. 🔥🌶🔥🌶🔥🌶🔥
Status and versioning classification: