Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Structures): remove Structures #6027

Merged
merged 2 commits into from Jul 4, 2021

Conversation

1Computer1
Copy link
Contributor

@1Computer1 1Computer1 commented Jul 3, 2021

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.

Structures.extend('Thing', Thing => {
  return class NewThing extends Thing {
    constructor() {
      // A property that has state, e.g. you change it over time.
      this.statefulProperty = statefulValue;
      
      // A property you only compute once.
      this.constantProperty = constantValue;
    }
    
    // A getter that gets something computed.
    get computed() { ... }
    
    // A method.
    doSomething() { ... }
  }
});

It is much better to do something like this:

// Stuff you put in here will go out with GC.
// Add stuff in when you want.
const thingMap = new WeakMap();
thingMap.set(thing, { statefulProperty, constantProperty });
thingMap.get(thing);

// Or just use a normal map if you don't care about invalidating cache.
const thingMap = new Collection();
thingMap.set(thing.id, { statefulProperty, constantProperty });
thingMap.get(thing.id);

// These are just regular functions.
// Put them somewhere and import them if you want them.
function computed(thing: Thing) { ... }
function doSomething(thing: Thing) { ... }

A more concrete example:

Structures.extend('Thing', Thing => {
  return class NewThing extends Thing {
    get config() {
      // Modifying client isn't a good idea either...
      return this.client.configurationDatabase.get(this.id);
    }
  }
});

// Inside whatever you use for commands:
function run(message) {
  message.guild.config.stuff ...
}

To:

// Change what you do. Don't modify types you don't own.
// Explicitly accept or import what you need.
// You can use dependency injection libraries if you're comfortable.

// Inside whatever you use for commands:
function run(configurationDatabase, message) {
  configurationDatabase.get(message.guild.id).stuff ...
}

Basically, remove this mess. 馃敟馃尪馃敟馃尪馃敟馃尪馃敟

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating
  • This PR changes the library's interface (methods or parameters added)
  • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

@iCrawl iCrawl added this to the Version 13 milestone Jul 3, 2021
Copy link
Member

@kyranet kyranet left a 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

@iCrawl
Copy link
Member

iCrawl commented Jul 4, 2021

Because we didn't have enough breaking changes already. 馃摝

@iCrawl iCrawl linked an issue Jul 4, 2021 that may be closed by this pull request
@lmfaobye
Copy link

lmfaobye commented Jul 4, 2021

馃憥 please don鈥檛

@GoldenAngel2
Copy link
Contributor

GoldenAngel2 commented Jul 4, 2021

馃憤 Completely break commando and any bots that use Structures properly.

@GamingGeek
Copy link
Contributor

https://youtu.be/I_cE4I5TyAo?t=13

@iCrawl iCrawl linked an issue Jul 4, 2021 that may be closed by this pull request
@kevinbioj
Copy link
Contributor

kevinbioj commented Jul 4, 2021

I see how people can get mad at this, but I personally agree.
That said, I've been using structures, do as I say, not as I do

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 Client and overwriting Client#options. Many people came in the discord.js server to troubleshoot that weird infinite recursive call that makes node crash, that is due to overwriting REST settings from client options. Maybe the guide should warn about that?

Now, if you used to have Message#settings or Guild#settings to hold guild settings for your commands to work properly, you should rather have a global class (like MessageContext or whatever) that would hold the message and the settings separately.

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).

Copy link
Member

@SpaceEEC SpaceEEC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃敟

@xHyroM
Copy link
Contributor

xHyroM commented Jul 4, 2021

馃憤 Completely break commando and any bots that use Structures properly.

True

@kyranet
Copy link
Member

kyranet commented Jul 4, 2021

馃憤 Completely break commando and any bots that use Structures properly.

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.

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).

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.

@iCrawl iCrawl merged commit ab0b3b9 into discordjs:master Jul 4, 2021
tandpfun added a commit to tandpfun/discord.js that referenced this pull request Jul 4, 2021
@AngeloCore
Copy link

馃敟

@discordjs discordjs deleted a comment from TheFerryn Jul 5, 2021
@discordjs discordjs deleted a comment from MrVaDiM4iK Jul 5, 2021
@redfer13
Copy link

redfer13 commented Jul 6, 2021

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.

@caj4
Copy link

caj4 commented Jul 6, 2021

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.

@caj4
Copy link

caj4 commented Jul 6, 2021

The number of people who want structures are larger than the number of people who don't.

@DTrombett
Copy link
Contributor

DTrombett commented Jul 6, 2021

Please don't remove Structures

They already did...

@TwilightZebby
Copy link

My bot's entirely dependent on it

I'd imagine most bots are also dependant on the <Message>.send() method, which received a breaking change in v13 - yet I don't see many complaints about that as with Structures being removed :lul:

@DTrombett
Copy link
Contributor

Well it's not the same... <Message>.send() was changed to make it easier to use and easier add new features. Structures were totally removed...

@kyranet
Copy link
Member

kyranet commented Jul 6, 2021

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 Message#send like @TwilightZebby has shown, however, as much as structures made our lives easier, they also carried a number of issues, from libraries being able to easily break our bots by assigning properties used by the library, to potential libraries taking advantage of this feature to register malicious code in bots without one knowing.

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:

  • Support:

    Support users are used to seeing a clean version of discord.js, a lot of them aren't well versed with discord.js extensions, and just like they can't give support for libraries that offer functionality similar to newer versions of the library, they're can't be expected to know what extensions you use, or why the code fails.

    Extensions also brought people who override discord.js's internal logic, when its original purpose was to extend by assigning new properties.

  • Predictability:

    Two libraries extend Message, one implements editable messages while the other one implements split messages, when registering both, which one is kept? What if you want to send non-editable split messages? What if you want to send editable messages?

    What if there's a hidden package that also extends Message and overrides it? How would you know? You have no control over how Structures is used because it can be called in any point on your code, even within transient dependencies.

  • Maintainance:

    Structures-related code was a burden to the development team, with many circular requires and having to check whether a structure can be extended, or even removing it in the future because Discord released a structure that behaves the same but has extra functionality.

    For example, we could have Dropdown, and then Discord releases DropdownWithButton which have all the functionality but add a button, we'd have to re-think the entire structure (BaseDropdown -> Dropdown; BaseDropdown -> DropdownWithButton) so we could have both extensible, but then you'd want to include DropdownWithButton when checking for instance of Dropdown (Dropdown -> DropdownWithButton) because it makes sense, but if we did this, you wouldn't be able to extend Dropdown, only DropdownWithButton.

  • Consistency:

    Did you know that if you use ECMAScript Modules, which is arguably the future of Node.js (top-level await, asynchronous parallel loading, no circulars, browser compatibility, etc), you can't extend User correctly? ClientUser is a structure built specifically for the client user, and it extends User. In CommonJS (what you use), the ClientUser export was a getter, and as soon as it was read, the ClientUser class locked into whatever was the last extension of User. Why is this a problem? Well, ESM doesn't support getters, so this is always evaluated, and as such, ClientUser extends the base User, and types say otherwise.

  • Performance:

    Extending classes by subclassing them is very expensive for performance, from instanceof to reading and assigning properties taking a lot longer, as well as increased memory usage due to extra prototype layers.

  • Practices:

    Did you know that no other [large] library in the ecosystem does this? The closest is Node.js's http module, which lets you define what classes to build, but not only that's very advanced, but its entire purpose is to allow HTTP libraries (express, fastify...) define their own router-related properties into the messages, but this is the opposite flow of Structures.extend, and not all libraries use it.

    However, this isn't composable, you most likely want to apply 2 extensions from two libraries to the same class, a lot of users do this. The only way to support it would be using very hacky code that doesn't play well with TypeScript at all.


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 <Object>.<Property>!", or something for that matter.

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 #library-discussion channel at our Discord server, we'd love to learn from it.

@subhiashraf912

This comment has been minimized.

@foxt

This comment has been minimized.

@ghost
Copy link

ghost commented Jul 18, 2021

Since the structures are removed can we get a more structurized way rather than using "functions"

@kyranet
Copy link
Member

kyranet commented Jul 18, 2021

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 TextChannel, NewsChannel, ThreadChannel, and a few more to come in the future.

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 send(message, contentOrOptions) and message.send(contentOrOptions) is that the former is 1 character longer, but nevertheless can be auto-imported by the text editor of your preference (VSCode and WebStorm do this!), and when using libraries, you can have in-editor documentation and other stuff while using untyped/undocumented JavaScript code.

As @1Computer1 mentioned several times, the only benefit of structures is the usage of the . operator, but other than that, it lacks of the composability that's much needed for Discord's structures.

I hope this is understandable for you, and for future reference, please feel free to talk further about this in the official Discord server.

@xHyroM

This comment has been minimized.

@PuneetGopinath

This comment has been minimized.

@scratchyone

This comment has been minimized.

@whirlxd

This comment has been minimized.

@ThatGuyJamal

This comment has been minimized.

@ThatAnonyG
Copy link
Contributor

ThatAnonyG commented Aug 23, 2021

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?

@ThatAnonyG
Copy link
Contributor

馃憤 Completely break commando and any bots that use Structures properly.

Sounds a great plan to me :D

@PenPow
Copy link

PenPow commented Aug 23, 2021

馃憤 Completely break commando and any bots that use Structures properly.

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.

@ThatAnonyG

This comment has been minimized.

@ThatAnonyG

This comment has been minimized.

GamingGeek added a commit to FireDiscordBot/discord.js that referenced this pull request Oct 29, 2021
@SpaceEEC SpaceEEC mentioned this pull request Nov 21, 2021
piemot added a commit to Rapha01/activityRank-bot that referenced this pull request Nov 19, 2023
uses WeakMap so that when a djs object is dropped its associated value will be as well. See discordjs/discord.js#6027 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Webhook Improve Structures - "binding"