Dev/button rework2 #14

rob-3 · 2021-07-24T22:39:37Z

See this for an example of how this works.

This feature allows users to build complex discord UIs more easily by abstracting `customId` management into dispatch.

src/Client.ts

suneettipirneni · 2021-07-25T16:33:17Z

I'm not against merging this, however I would personally use the collector api in most use cases, as it offers a balance of abstraction and escape hatches.

Some examples include:

A built-in filter predicates in the collector.
Using function references for the collector handlers.
Uses raw components so it's as customizable as possible.

rob-3 · 2021-07-25T17:11:16Z

Fair enough. This doesn't break any existing code so you can feel free to use the collector method if you prefer that. If nobody but me prefers this API, though, we probably should not add it. Filter predicates are a good point. I'm not sure I understand the function references; you can just do onClick: myref with my API.

suneettipirneni · 2021-07-25T17:18:46Z

We should get input from @vijaystroup and @Armintron on what method they prefer

Armintron · 2021-07-25T17:36:30Z

Could I get a summary of what this PR does? I haven't worked with buttons or anything so I'm not sure what's good/bad.

suneettipirneni · 2021-07-25T17:37:47Z

I’ll post two examples of both of the implementations later.

…

On Jul 25, 2021, at 1:36 PM, Armin ***@***.***> wrote: Could I get a summary of what this PR does? I haven't worked with buttons or anything so I'm not sure what's good/bad. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or unsubscribe.

suneettipirneni · 2021-07-25T17:46:11Z

Could I get a summary of what this PR does? I haven't worked with buttons or anything so I'm not sure what's good/bad.

But TLDR; if you've used to emoji reaction buttons, then the discord button collectors act almost exactly the same. This PR differs by taking a more abstract approach.

vijaystroup · 2021-07-25T20:34:20Z

As long as it is easier for the end developer or anyone new that might join the team in creating additions, I am all for it.

Armintron · 2021-07-25T21:15:33Z

As long as it is easier for the end developer or anyone new that might join the team in creating additions, I am all for it.

+1

suneettipirneni · 2021-07-28T22:01:19Z

src/Command.ts

+  run({
+    interaction,
+    registerUI,
+  }: {
+    interaction: CommandInteraction;
+    registerUI: (
+      ui: UIComponent | UIComponent[] | UIComponent[][]
+    ) => MessageActionRow[];
+  }): Promise<void> | void;


I don't know how I feel about registerUIbeing a parameter on run. registerUI doesn't seem like something that should be related to running the command, especially, if you don't use components.

That's why I used the destructuring syntax so you don't need to put it in your parameter list unless you need it. This will also make it easy to inject other dependencies like storage later without introducing more parameters or a IOC container.

But why does this need to be passed in as a function parameter in the first place?

It doesn't, I just thought a function injection was simpler than passing in the entire Client since you really only need the function.

I feel like having a class-based command with a client constructor (like other frameworks) would be easier to reason about, it would also allow for adding other properties later.

suneettipirneni · 2021-07-28T22:02:38Z

src/UI.ts

+
+export type UIComponent = DispatchButton | DispatchLinkButton;
+
+export type ButtonOptions = {


Can't this be an interface that extends MessageButtonOptions?

Possibly, I've never used extends with an interface. I'll look into it

Ok I did a thing with Omit, lmk what you think

suneettipirneni · 2021-07-28T22:04:37Z

src/UI.ts

+  onClick: ButtonHandler;
+};
+
+export type LinkButtonOptions = {


Same with this but use the Omit utility to not inherit all properties from MessageButtonOptions

Same as above

Armintron · 2021-07-29T22:40:05Z

Really indifferent on the structure; I can do whatever as long as documentation is maintained. So in that sense, whatever is easier to document is my choice.

rob-3 · 2021-07-29T22:45:20Z

In all honesty, the documentation is basically "look at another command similar to what you want to do" or ask someone who knows what they're doing either way.

Armintron · 2021-07-31T02:26:16Z

I think you can make a master's thesis out of all this.

suneettipirneni · 2021-07-31T03:14:46Z

Ok I've decided that we'll use @rob-3's method of passing data in and we'll take it from there. And we'll use a "wait and see" approach to this. If we run into issues we can look into classes.

rob-3 · 2021-07-31T03:16:45Z

Don't merge this or bother reviewing it yet. After our discussion I have a few ideas about how it could be improved.

suneettipirneni · 2021-07-31T03:18:32Z

I'm not planning on merging anything yet, but you can merge whenever you're satisfied with your implementation.

suneettipirneni · 2021-07-31T14:52:06Z

src/UI.ts

+export type ButtonOptions = Omit<
+  MessageButtonOptions,
+  'customId' | 'style' | 'url'
+> & {
+  style: Exclude<MessageButtonStyleResolvable, 'LINK'>;
+  onClick: ButtonHandler;
+};
+
+export type LinkButtonOptions = Omit<
+  MessageButtonOptions,
+  'customId' | 'style' | 'url'
+> & {
+  url: string;
+};


These can be simplified now that discordjs/discord.js#6212 is merged and deployed. From now on to check for differences between the two buttons you can use type narrowing:

if (button.options.type === 'LINK') { ... Skip listeners return; } // Otherwise we know for sure customID has to exist. button.customID

suneettipirneni · 2021-07-31T14:54:58Z

src/UI.ts

+export class DispatchButton {
+  constructor(readonly options: ButtonOptions) {}
+
+  toDiscordComponent(
+    buttonListeners: Map<string, ButtonHandler>
+  ): MessageButtonOptions {
+    const { onClick, ...options } = this.options;
+    // nonlink buttons must have a customId
+    const id = getID(this.options.label ?? '<unlabeled>', 'button');
+    buttonListeners.set(id, onClick);
+    return { ...options, type: 'BUTTON', customId: id };
+  }
+}
+
+export class DispatchLinkButton {
+  constructor(readonly options: LinkButtonOptions) {}
+
+  toDiscordComponent(): MessageButtonOptions {
+    // we override style in case the user omitted it
+    return {
+      ...this.options,
+      type: 'BUTTON',
+      style: 'LINK',
+    };
+  }
+}


Corollary to the comment above, these two classes can be merged together.

It's questionable if classes are even needed in this context. You could achieve polymorphism with just interfaces.

I thought about doing that. Before I thought this was a better solution because it makes the type errors better when the user doesn't include the correct properties.

suneettipirneni · 2021-07-31T20:24:41Z

Closed in favor of newer PR

rob-3 added 2 commits July 24, 2021 12:56

Add UUID npm package

6a56f7f

Add UI module and registerUI() method

3349e53

This feature allows users to build complex discord UIs more easily by abstracting `customId` management into dispatch.

rob-3 mentioned this pull request Jul 24, 2021

Update vibe command to use new Button API KnightHacks/Lancelot-Archived#19

Closed

rob-3 requested a review from suneettipirneni July 24, 2021 22:42

suneettipirneni reviewed Jul 24, 2021

View reviewed changes

src/Client.ts Outdated Show resolved Hide resolved

Fix bug with link buttons

58959ea

Armintron approved these changes Jul 26, 2021

View reviewed changes

rob-3 added 13 commits July 27, 2021 20:25

Merge branch 'main' into dev/button-rework2

28cbe93

Use an object for run() parameters

fa5c6fc

Inject registerUI function directly

6c7a8c5

Update docs

447bfc9

Remove FIXME

1734879

Fix bug with this binding

12c0397

Remove needless dependency on Client

924e7f7

Reformat code

263b634

Extract normalization code to function

904910d

Convert to idiomatic OOP style

afcfd0a

Simplify ButtonHandler arguments

b31b393

Improve docs

cbcc9f6

Add validation for row size constraints

74283e6

rob-3 requested a review from suneettipirneni July 28, 2021 21:51

suneettipirneni requested changes Jul 28, 2021

View reviewed changes

rob-3 added 4 commits July 28, 2021 18:21

Improve typing of ButtonOptions

4577a98

Use Omit with LinkButtonOptions as well

cda6aae

Format code

3c681df

Fix lint error

605cedd

This comment has been minimized.

Sign in to view

bump discord.js

5702170

suneettipirneni requested changes Jul 31, 2021

View reviewed changes

suneettipirneni closed this Jul 31, 2021


		export type UIComponent = DispatchButton \| DispatchLinkButton;

		export type ButtonOptions = {

Dev/button rework2 #14

Dev/button rework2 #14

Conversation

rob-3 commented Jul 24, 2021 • edited Loading

suneettipirneni commented Jul 25, 2021

rob-3 commented Jul 25, 2021

suneettipirneni commented Jul 25, 2021

Armintron commented Jul 25, 2021

suneettipirneni commented Jul 25, 2021 via email • edited Loading

suneettipirneni commented Jul 25, 2021 • edited Loading

vijaystroup commented Jul 25, 2021

Armintron commented Jul 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Armintron commented Jul 29, 2021

rob-3 commented Jul 29, 2021 • edited Loading

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.

This comment has been minimized.

This comment has been minimized.

Armintron commented Jul 31, 2021

This comment has been minimized.

This comment has been minimized.

suneettipirneni commented Jul 31, 2021

rob-3 commented Jul 31, 2021

suneettipirneni commented Jul 31, 2021

suneettipirneni Jul 31, 2021 • edited Loading

Choose a reason for hiding this comment

suneettipirneni Jul 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suneettipirneni commented Jul 31, 2021

rob-3 commented Jul 24, 2021 •

edited

Loading

suneettipirneni commented Jul 25, 2021 via email •

edited

Loading

suneettipirneni commented Jul 25, 2021 •

edited

Loading

rob-3 commented Jul 29, 2021 •

edited

Loading

suneettipirneni Jul 31, 2021 •

edited

Loading

suneettipirneni Jul 31, 2021 •

edited

Loading