feat(MessageButton): builder and associated interfaces #6

monbrey · 2021-07-04T07:08:44Z

The MessageButton builder, in TypeScript

Builder class for Message Buttons
Interfaces and types
Some Util functions as static methods that would probably belong better elsewhere
Added ow as a dependency to validate input
Tests

This is a draft for now for a few reasons:

Unsure if it needs to extend a base component class or not
It's the first time I've used jest for anything, so will need advise/reviews on the tests
I've re-written a lot of types that could just be imported from discord-api-types, but it would require bumping the dependency version to 0.19

Status and versioning classification:
TBD

codecov-commenter · 2021-07-04T07:09:31Z

Codecov Report

Merging #6 (2aea829) into main (97cf337) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main        #6   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         2    +1     
  Lines           12        61   +49     
  Branches         4        19   +15     
=========================================
+ Hits            12        61   +49

Impacted Files	Coverage Δ
src/components/MessageButton.ts	`100.00% <100.00%> (ø)`
src/messages/formatters.ts	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97cf337...2aea829. Read the comment docs.

iCrawl

Depending on how we decide for v13 or not, ignore the customId changes 👍

iCrawl · 2021-07-04T11:35:06Z

src/components/MessageButton.ts

+	 * The text to be displayed on this button
+	 * @type {?string}
+	 */
+	public customID?: string;


Suggested change

public customID?: string;

public customId?: string;

We'll not use this in v13 anyway, right? cc @discordjs/the-big-3

Otherwise we'll have to change this once we go to the rewrite, which is a bit iffy I feel.

We can always break this for v13, depends on when you want to release it.

I opened discordjs/discord.js#6036 a few hours ago, so depending on whether we go for ID or Id, depends the resolution of that PR for v13.

Yup, merged, we can move forward with that change.

iCrawl · 2021-07-04T11:37:39Z

src/components/MessageButton.ts

+ */
+export class MessageButton {
+	/**
+	 * The text to be displayed on this button


Copy paste error from label

iCrawl · 2021-07-04T11:38:23Z

src/components/MessageButton.ts

+	 * @param {string} customID A unique string to be sent in the interaction when clicked
+	 * @returns {MessageButton}
+	 */
+	public setCustomID(customID: string): this {


Suggested change

public setCustomID(customID: string): this {

public setCustomId(customId: string): this {

iCrawl · 2021-07-04T11:38:33Z

src/components/MessageButton.ts

+	}
+
+	/**
+	 * Sets the custom ID of this button


Suggested change

* Sets the custom ID of this button

* Sets the custom id of this button

iCrawl · 2021-07-04T11:39:33Z

src/components/MessageButton.ts

+
+	/**
+	 * Sets the custom ID of this button
+	 * @param {string} customID A unique string to be sent in the interaction when clicked


Suggested change

* @param {string} customID A unique string to be sent in the interaction when clicked

* @param customId A unique string to be sent in the interaction when clicked

In TS we don't need to document param types.

iCrawl · 2021-07-04T11:44:06Z

tests/components.test.ts

+					label: 'discord.js',
+					style: 'Primary',
+				});
+				expect(button.customID).toBe('discord.js');


Suggested change

expect(button.customID).toBe('discord.js');

expect(button.customId).toBe('discord.js');

iCrawl · 2021-07-04T11:44:12Z

tests/components.test.ts

+		describe('constructor', () => {
+			test('GIVEN data THEN class properties are set', () => {
+				const button = new MessageButton({
+					customID: 'discord.js',


Suggested change

customID: 'discord.js',

customId: 'discord.js',

iCrawl · 2021-07-04T11:44:19Z

tests/components.test.ts

+				expect(button.style).toBe(1);
+			});
+
+			test('GIVEN raw data THEN customID is set to custom_id', () => {


Suggested change

test('GIVEN raw data THEN customID is set to custom_id', () => {

test('GIVEN raw data THEN customId is set to custom_id', () => {

iCrawl · 2021-07-04T11:44:27Z

tests/components.test.ts

+					custom_id: 'discord.js',
+					style: 2,
+				});
+				expect(button.customID).toBe('discord.js');


Suggested change

expect(button.customID).toBe('discord.js');

expect(button.customId).toBe('discord.js');

iCrawl · 2021-07-04T11:44:46Z

tests/components.test.ts

+				expect(
+					new MessageButton({
+						style: 'Primary',
+						customID: 'discord.js',


Suggested change

customID: 'discord.js',

customId: 'discord.js',

vladfrangu · 2021-07-05T06:56:52Z

src/components/BaseComponent.ts

+/**
+ * https://discord.com/developers/docs/interactions/message-components
+ */
+export interface APIBaseComponent {


~~Why are these exported?~~

Second thought, I know why, i'll fix this today

vladfrangu · 2021-07-05T06:59:41Z

src/components/MessageButton.ts

+	 */
+	public setCustomID(customID: string): this {
+		ow(customID, ow.string.nonEmpty.maxLength(100));
+		ow(this.style, ow.optional.number.inRange(1, 4).message('Cannot set customID on a button that is Link style.'));


Doesn't ow have something for enums?

Didn't you forget a .not?

Won't this always match, since you basically make ow validate that style is in the range of 1-4. Did you mean .equals(Styles.Link)?

vladfrangu · 2021-07-05T07:00:26Z

src/components/MessageButton.ts

+	 * @returns {MessageButton}
+	 */
+	public setURL(url: string) {
+		ow(url, ow.string.nonEmpty);


Pretty sure there's a hyperlink option

vladfrangu · 2021-07-05T07:00:40Z

src/components/MessageButton.ts

+	 */
+	public setURL(url: string) {
+		ow(url, ow.string.nonEmpty);
+		ow(this.style, ow.optional.number.equal(5).message('Cannot set URL on a button that is not Link style.'));


Is optional intended?

vladfrangu · 2021-07-05T07:11:14Z

src/components/MessageButton.ts

+	 * @returns {APIButtonComponent} The raw data of this button
+	 */
+	public toJSON() {
+		ow(this.style, ow.number);


Might as well validate it's not any number, but valid numbers, no? Unless we want to allow... forwards-compatibilty with new types

vladfrangu · 2021-07-05T07:21:50Z

src/components/MessageButton.ts

+			style,
+			ow.any(
+				ow.number.inRange(1, 5),
+				ow.string.is((value) => ['primary', 'secondary', 'success', 'danger', 'link'].includes(value.toLowerCase())),


Should we support strings for builders? Or should we stick to just enum values? CC @discordjs/the-big-3

I'd rather just go enum only.

feat(MessageButton): builder and associate interfaces

2aea829

iCrawl suggested changes Jul 4, 2021

View reviewed changes

vladfrangu suggested changes Jul 5, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(MessageButton): builder and associated interfaces #6

feat(MessageButton): builder and associated interfaces #6

monbrey commented Jul 4, 2021

codecov-commenter commented Jul 4, 2021

iCrawl left a comment

iCrawl Jul 4, 2021

iCrawl Jul 4, 2021

kyranet Jul 4, 2021

kyranet Jul 4, 2021 •

edited

iCrawl Jul 4, 2021

iCrawl Jul 4, 2021

iCrawl Jul 4, 2021

iCrawl Jul 4, 2021

iCrawl Jul 4, 2021

iCrawl Jul 4, 2021

iCrawl Jul 4, 2021

iCrawl Jul 4, 2021

iCrawl Jul 4, 2021

iCrawl Jul 4, 2021

vladfrangu Jul 5, 2021 •

edited

vladfrangu Jul 5, 2021

vladfrangu Jul 5, 2021

vladfrangu Jul 5, 2021

vladfrangu Jul 5, 2021

vladfrangu Jul 5, 2021

iCrawl Jul 5, 2021

	public setCustomID(customID: string): this {
	public setCustomId(customId: string): this {

	* Sets the custom ID of this button
	* Sets the custom id of this button

	* @param {string} customID A unique string to be sent in the interaction when clicked
	* @param customId A unique string to be sent in the interaction when clicked

	expect(button.customID).toBe('discord.js');
	expect(button.customId).toBe('discord.js');

	test('GIVEN raw data THEN customID is set to custom_id', () => {
	test('GIVEN raw data THEN customId is set to custom_id', () => {

feat(MessageButton): builder and associated interfaces #6

Are you sure you want to change the base?

feat(MessageButton): builder and associated interfaces #6

Conversation

monbrey commented Jul 4, 2021

codecov-commenter commented Jul 4, 2021

Codecov Report

iCrawl left a 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

kyranet Jul 4, 2021 • edited

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

Choose a reason for hiding this comment

vladfrangu Jul 5, 2021 • edited

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

kyranet Jul 4, 2021 •

edited

vladfrangu Jul 5, 2021 •

edited