Skip to content

Conversation

@vvito7
Copy link

@vvito7 vvito7 commented Jun 20, 2022

Please describe the changes this PR makes and why it should be merged:
Adds the constant CDNRoutes, that contains each CDN entpoint from DDevs Portal. Made it because of #453 (comment). With this, many other things are being added: ImageFormats, CDNQuery and *Format types.

If applicable, please reference Discord API Docs PRs or commits that influenced this PR:

@vercel
Copy link

vercel bot commented Jun 20, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
discord-api-types ✅ Ready (Inspect) Visit Preview Jun 20, 2022 at 11:08PM (UTC)

@vladfrangu
Copy link
Member

this PR doesn't add the last added CDN endpoint

It should, as the PR you linked adds it as a route, not a cdn route

Copy link
Contributor

@ImRodry ImRodry left a comment

Choose a reason for hiding this comment

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

Don't you wanna add a size parameter in every endpoint?

@vladfrangu
Copy link
Member

Outside of defining them as interfaces, just like how we do for other rest endpoints, no. So, if @vvito7 wants to do that then they can, otherwise this is fineee

@didinele
Copy link
Member

Should we maybe export & use types like EmojiFormat = Exclude<ImageFormat, ImageFormat.Lottie> instead of typing it out everywhere? It's more maintainable for both us and the end-user (i.e. if they write a function that takes a format it then passes it into our methods)

@vvito7
Copy link
Author

vvito7 commented Jun 20, 2022

Outside of defining them as interfaces, just like how we do for other rest endpoints, no. So, if @vvito7 wants to do that then they can, otherwise this is fineee

I could of course, but would it be something like this? I got a little confused

export interface RESTGetCDNRouteQuery {
	/**
	 * The returned image can have the size changed by using this query parameter
	 */
	size?: AllowedImageSizes;
}

export type AllowedImageSizes = 16 | 32 | 56 | 64 | 96 | 128 | 256 | 300 | 512 | 600 | 1024 | 2048 | 4096;

Should we maybe export & use types like EmojiFormat = Exclude<ImageFormat, ImageFormat.Lottie> instead of typing it out everywhere? It's more maintainable for both us and the end-user (i.e. if they write a function that takes a format it then passes it into our methods)

I like the idea 👍

@vladfrangu
Copy link
Member

Outside of defining them as interfaces, just like how we do for other rest endpoints, no. So, if @vvito7 wants to do that then they can, otherwise this is fineee

I could of course, but would it be something like this? I got a little confused

export interface RESTGetCDNRouteQuery {
	/**
	 * The returned image can have the size changed by using this query parameter
	 */
	size?: AllowedImageSizes;
}

export type AllowedImageSizes = 16 | 32 | 56 | 64 | 96 | 128 | 256 | 300 | 512 | 600 | 1024 | 2048 | 4096;

Yes, but maybe we should just type size as a number? Also this can use a diff interface name (CDNQuery?) if you want

@vvito7
Copy link
Author

vvito7 commented Jun 20, 2022

maybe we should just type size as a number?

Yes, sure. But there is a bunch of numbers that don't work
I could just add the allowed sizes in the docs

Also this can use a diff interface name (CDNQuery?) if you want

Sure👍

@vladfrangu
Copy link
Member

I could just add the allowed sizes in the docs

Well its just powers of 2 so... that should be easy enough

@ImRodry
Copy link
Contributor

ImRodry commented Jun 20, 2022

Except it isn’t

@vladfrangu
Copy link
Member

Source?

@ImRodry
Copy link
Contributor

ImRodry commented Jun 20, 2022

Discord.js, discord docs, whatever floats your boat

@vladfrangu
Copy link
Member

Well call me nuts because Discord says it themselves: https://discord.com/developers/docs/reference#image-formatting-image-base-url

The returned size can be changed by appending a querystring of ?size=desired_size to the URL. Image size can be any power of two between 16 and 4096.

@vvito7
Copy link
Author

vvito7 commented Jun 20, 2022

Tried to implement what @didinele said, but I don't think it's what they wanted. Also there is so many duplicated stuff
Let me know how I can improve it

@didinele
Copy link
Member

didinele commented Jun 20, 2022

That's basically it, just make each one a type rather than putting it under an interface, e.g.
type EmojiFormat = Exclude<ImageFormat, ImageFormat.Lottie>;

Also there is so many duplicated stuff

that's alright.

@vladfrangu vladfrangu changed the title feat: add CDNRoutes feat(REST): add CDNRoutes Jun 20, 2022
@vladfrangu vladfrangu merged commit 0609886 into discordjs:main Jun 20, 2022
@vvito7 vvito7 deleted the cdn-routes branch June 21, 2022 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants