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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: @discordjs/proxy #7925

Merged
merged 33 commits into from
Jun 4, 2022
Merged

feat: @discordjs/proxy #7925

merged 33 commits into from
Jun 4, 2022

Conversation

didinele
Copy link
Member

@didinele didinele commented May 15, 2022

Due to the unusual nature of this PR, I will be mostly opting out of the template.

Please describe the changes this PR makes and why it should be merged:
This PR introduces a new package to the monorepo - @discordjs/proxy. As the name implies, this package would enable users to create proxies for their REST calls.

Motivation and reasoning

Prior to #7747, there was no easy, non-hacky way to have "shared rate-limiting", which made things like offloading specific workloads to a different service or even just a worker_thread dangerous. Now that it's become possible, users might be already inclined to spin up a twilight proxy (or other Discord HTTP proxies!) to send their requests through.

This is fine, but realistically, I feel like proxying will mostly be done by power users, who may not have their use cases fulfilled by existing proxies. It's also a big turn-off that twilight is the most recognizable one, but is written in Rust and as such can feel a bit daunting to fork and modify to one's needs.

Goals

This is intended to be a library. It should most certainly not be specific to any HTTP framework, but rather expose middleware/request handler functions that would be compatible with most. Later down the road (probably out of scope for this PR) we could be somehow shipping "binaries" (i.e. docker images) with the more common defaults, making it extremely easy to spin a proxy up, while still enabling the creation of custom ones.

How would this work?

Simply put, we would be using @discordjs/rest. Our request handler would essentially just forward whatever request comes through, strip away the rate-limit headers, and forward the response from Discord. Stripping rate-limit headers is needed so whatever REST handler receives this response thinks there are no rate limits, and as such doesn't try to deal with them.

Current caveats

  • Response headers are not forwarded for non-2xx responses.
  • As REST does not support dictating the API version to use per-request, the /api/v[x] part of the URL is dropped and ignored by the current implementation, as such, the API version used is dictated solely by the proxy, not by the remote upstream.

WIP:

  • initialize package
  • finalize signature for proxyRequests - opted to only take in a REST instance
  • wait for feat: REST#raw #7929 to get merged
  • parse request URL properly, as REST always appends /api/v[apiversion] to it
  • figure out a more correct way to handle invalid methods - opted for just letting Discord throw Method Not Allowed
  • header to dictate if 429s should be retried - not possible with current REST, unsure if it'll still be implemented
  • better modularization, enabling easily writing custom handlers
  • write docs
  • write tests

packages/proxy/cliff.toml Outdated Show resolved Hide resolved
@didinele didinele force-pushed the feat/proxy branch 2 times, most recently from 1add7a5 to b2250af Compare May 15, 2022 19:49
Copy link
Contributor

@SuperchupuDev SuperchupuDev left a comment

Choose a reason for hiding this comment

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

really small thing but this is how native node modules are imported in every other djs package

packages/proxy/__tests__/index.test.ts Outdated Show resolved Hide resolved
packages/proxy/src/index.ts Outdated Show resolved Hide resolved
packages/proxy/package.json Outdated Show resolved Hide resolved
@didinele
Copy link
Member Author

With #7929 merged and rebased into this branch, it's now ready for review.

@didinele didinele marked this pull request as ready for review May 17, 2022 15:04
@Milo123459
Copy link
Contributor

Is this going to have support for disabling the ratelimiter?

@monbrey
Copy link
Member

monbrey commented May 25, 2022

Is this going to have support for disabling the ratelimiter?

Why? So you can just get rate limited and throw errors?

@Milo123459
Copy link
Contributor

Is this going to have support for disabling the ratelimiter?

Why? So you can just get rate limited and throw errors?

Some proxies handle ratelimiting for you.

@didinele
Copy link
Member Author

Some proxies handle ratelimiting for you.

Unsure what you mean with this in the given context - but no, the whole point of proxying is to have shared ratelimiting.

The only configurable behavior here comes from passing in your own REST instance, which will allow you to pick where you want ratelimit retries to take place.

Our default binary (once we start shipping those) will leave upstream to handle ratelimiting - i.e. the proxy will forward 429s immediately.

@Milo123459
Copy link
Contributor

Twilight proxy (the one you linked) supports ratelimiting.

@ckohen
Copy link
Member

ckohen commented May 26, 2022

Is this going to have support for disabling the ratelimiter?

Which rate limiter are you talking about, think that's the miscommunication here.

@Milo123459
Copy link
Contributor

Is this going to have support for disabling the ratelimiter?

Which rate limiter are you talking about, think that's the miscommunication here.

Does discord.js not handle ratelimits? That's the one I'm talking about, the one that is built-in

@ckohen
Copy link
Member

ckohen commented May 27, 2022

No, this does not include a method to disable ratelimits in djs, because end users would then do that without using a proxy. It will implicitly disable ratelimit handling by handling them at the proxy level and stripping ratelimit headers from the proxied response sent to the client.

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.

Just a few things, otherwise LGTM

packages/proxy/src/handlers/proxyRequests.ts Outdated Show resolved Hide resolved
packages/proxy/__tests__/proxyRequests.test.ts Outdated Show resolved Hide resolved
packages/proxy/__tests__/proxyRequests.test.ts Outdated Show resolved Hide resolved
@vladfrangu vladfrangu self-requested a review May 29, 2022 20:41
@iCrawl iCrawl merged commit 1ba2d2a into discordjs:main Jun 4, 2022
@iCrawl iCrawl added this to the proxy v1 milestone Jun 4, 2022
@didinele didinele deleted the feat/proxy branch June 4, 2022 16:20
@didinele didinele mentioned this pull request Jun 4, 2022
3 tasks
suneettipirneni pushed a commit to suneettipirneni/discord.js that referenced this pull request Jun 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.