feat: add missing v13 component methods

[suneettipirneni]

Please describe the changes this PR makes and why it should be merged:

Adds

• Embed#hexColor
• #equals
• Select menu allows objects to be passed in to setOptions and addOptions instead of exclusively just classes.

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)

[ImRodry]

Can you also add hexColor back and overwrite setColor to use Util.resolveColor?

Stale

[iCrawl]

This needs a rebase.

Stale

[KhafraDev]

I haven't tested much but wouldn't something like this work for equals?

import { deepStrictEqual } from 'assert';

export const equals = (a, b) => {
    try {
        deepStrictEqual(a, b);
        return true;
    } catch {
        return false;
    }
}

It works with classes, nested objects (as far as 2-3 levels as I tested) and it's simple enough. 🤷

[kyranet]

Feels like using the wrong tool to do the right thing. Also, the fact it throws an error (and thus needing a try/catch) has severe performance implications.

[KhafraDev]

I wouldn't necessarily call it the wrong tool, from its description: Tests for deep equality between the actual and expected parameters. "Deep" equality means that the enumerable "own" properties of child objects are recursively evaluated also by the following rules.

For performance, I doubt that using try/catch is going to be a bottleneck. It might be slower, but slower does not mean slow.

[ImRodry]

I never saw the point in the .equals() methods, why are they being added back?

They're not being used internally, and now there's going to be a brand new dependency on the package just for them. Why can't the users that will be using the method (if any) install the dependency and compare the objects themselves?

If you never saw the point that’s your problem but you’re not the only person using the package. There’s plenty of use cases and I initially PR’d it so you can assume at least I do + if there’s no replacement and no good reason to remove it it shouldn’t be removed. I agree that the dependency is extremely necessary and the old implementation should be brought back but apparently I’m alone in that

[almeidx]

dependencies in a package are not a bad thing.

I wasn't trying to say that dependencies are a bad thing. What I was trying to say is that, in this case, the implementation for the method is so simple (using an external package; as simple as just calling a function) that I don't see the need for having it in this package

[suneettipirneni]

dependencies in a package are not a bad thing.

I wasn't trying to say that dependencies are a bad thing. What I was trying to say is that, in this case, the implementation for the method is so simple (using an external package; as simple as just calling a function) that I don't see the need for having it in this package

For one, if you're using typescript data isn't publicly exposed so it doesn't get mutated externally. Also in some cases like Embed, ActionRow, and SelectMenu there are special cases you need to handle when doing comparisons. For example SelectMenu stores options in a separate options array rather than directly on the data itself. Embed would do equality based on image height and proxy url by default, which are props that will never be equal when compared to a received Embed from the API.

[suneettipirneni]

For one, if you're using typescript data isn't publicly exposed so it doesn't get mutated externally.

This being said, yes you could extend the class to access data if need be.

[KhafraDev]

For one, if you're using typescript data isn't publicly exposed so it doesn't get mutated externally.

I think this is more so an issue with the semantics. According to kyra's post, the raw data should be available to users ("[t]he raw data must be exposed so users can access it"). By allowing access to the data would also forgo any need for future PRs to add utility functions as well.

export const whateverEquals = (a: WhateverData, b: WhateverData) => {
    return util.deepStrictEqual(a.data, b.data);
}

export const embedHex = (a: Embed) => {
    return `#${a.data.color.toString(16).padStart(6, '0')}`;
}

p.s.: I'm fine with this change if util.deepStrictEqual is used in either case. Otherwise, almeidx's arguments still apply and I do agree with them.

[suneettipirneni]

p.s.: I'm fine with this change if util.deepStrictEqual is used in either case. Otherwise, almeidx's arguments still apply and I do agree with them.

util.deepStrictEqual is extremely slow compared to the fast-deep-equals package, I'm not sure the mantra of "avoid extra dependencies" is enough to warrant not using it. This is especially true when the package is less than 1 kb, and looking at the package code, the implementation is anything but trivial.

Once again the argument that it should be handled by the user is a rationale that could be applied to any utility method in the library. That being said, yes there are situations where the library isn't responsible for functionality that doesn't fit in the scope of library. But I don't think adding an .equals method is even close to that. If we're responsible for dictating the internal values we should also be responsible for deciding equality between those values. This is especially true considering that other non-builder discord.js entities also have equals() methods.

[KhafraDev]

Slower does not mean slow. At 46,000 ops a second (fast deep equal benchmark) I would say that it’s well past fast enough. I would be surprised if equals has been used 46k times since it’s inception (half joking lol).

[suneettipirneni]

Slower does not mean slow. At 46,000 ops a second (fast deep equal benchmark) I would say that it’s well past fast enough. I would be surprised if equals has been used 46k times since it’s inception (half joking lol).

It's still bottom of the barrel when it comes to performance if I can get better performance at the cost of 1kb I'll take that instead. In terms of usage it's hard to tell how many times a builder method is used because it could be invoked only once or be invoked every time a command is run. Furthermore you can have situations where you need to run a find() on an array that would use .equals for every element. It's a very primitive base method that can be applied in a bunch of different areas. Regardless of the use cases it would be nice if the built in solution can scale well if needed.

[KhafraDev]

Regardless of the use cases it would be nice if the built in solution can scale well if needed.

46k ops/s does scale well. As said earlier, maybe not as fast as other dependencies, but it is fast enough for 100% of use cases. If someone using it needs the best performance humanly possible (spoiler: no one does), then they can override the equals method by extending the class and implement it using whatever dependency they want.

edit: example

export class MyEqualsIsFaster extends Embed {
    public override equals (a: Embed, b: Embed): boolean {
        return fastDeepEqual(a, b); // or whatever
    }
}

it could be invoked only once or be invoked every time a command is run.

It still wouldn't be the bottleneck. I was also joking that the only person I've ever seen need this method is Rodry, and even with it being gone entirely from v14, he's the only person who has wanted it to be re-implemented.

It's a very primitive base method that can be applied in a bunch of different areas.

Have any real life use cases?

[suneettipirneni]

46k ops/s does scale well. As said earlier, maybe not as fast as other dependencies, but it is fast enough for 100% of use cases. If someone using it needs the best performance humanly possible (spoiler: no one does), then they can override the equals method by extending the class and implement it using whatever dependency they want.

Sure, but it'd be nice if the library included a common and highly used external library so performance isn't always a concern for bots that want to scale.

I was also joking that the only person I've ever seen need this method is Rodry, and even with it being gone entirely from v14, he's the only person who has wanted it to be re-implemented.

This is a not a very good point. I we want to get into semantics I can list at least four people who wanted it restored (rodry,IronM00n imranbarcaria and smidul). But still these numbers prove absolutely nothing, and there's no way they can extrapolated to the already small percentage of users using v14. User request numbers are pretty much meaningless.

Have any real life use cases?

Here's some real world support messages that needed them:
https://discord.com/channels/222078108977594368/447463323135508480/930914561350049842

https://discord.com/channels/222078108977594368/824411059443204127/914678861755744386

https://discord.com/channels/222078108977594368/824411059443204127/918374538457280512

In addition, one thing that wasn't mentioned is tracking differences between messages like on the message update event. You'd have to do a value comparison for tracking those differences.

[kyranet]

Sorry, wrong button. ^

Stale

[suneettipirneni]

Don't merge yet, need to adjust one thing in action row

[suneettipirneni]

Don't merge yet, need to adjust one thing in action row

Nevermind, it's good to go.