-
Notifications
You must be signed in to change notification settings - Fork 49
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
fix: CloudFront CDN rate limit #74
fix: CloudFront CDN rate limit #74
Conversation
@@ -130,7 +118,7 @@ export class MovieDb { | |||
|
|||
// Get the params that are needed for the endpoint | |||
// to remove from the data/params of the request | |||
const omittedProps = (endpoint.match(/:[a-z]*/gi) || []).map((prop) => prop.substr(1)) | |||
const omittedProps = [...(endpoint.match(/:[a-z]*/gi) ?? [])].map((prop) => prop.slice(1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Destructed RegExpMatchArray
into an array to improve typing.
@@ -130,7 +118,7 @@ export class MovieDb { | |||
|
|||
// Get the params that are needed for the endpoint | |||
// to remove from the data/params of the request | |||
const omittedProps = (endpoint.match(/:[a-z]*/gi) || []).map((prop) => prop.substr(1)) | |||
const omittedProps = [...(endpoint.match(/:[a-z]*/gi) ?? [])].map((prop) => prop.slice(1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String.prototype.slice
was marked as deprecated.
@@ -71,7 +70,7 @@ export class MovieDb { | |||
|
|||
if (matches.length === 1) { | |||
return matches.reduce((obj, match) => { | |||
obj[match.substr(1)] = params | |||
obj[match.slice(1)] = params |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String.prototype.slice
was marked as deprecated.
@@ -80,17 +79,6 @@ export class MovieDb { | |||
return {} | |||
} | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused method.
import { | ||
HttpMethod, | ||
AuthenticationToken, | ||
RequestOptions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused import.
The irony is throttling was removed recently, but this is a better solution anyway |
This seems like a good fix for simple environments. Unfortunately it still won't be enough for production environments with more than one instance, for example those of us who use https://www.npmjs.com/package/pm2 if we are running 4 load-balanced instances, that's 200 requests per second with this change. |
This is a great idea. I did indeed not think about this problem before. How do you think could a limiter outside of Node look like, exactly? |
@alexanderroidl I guess an event queue would allow Node to have several instances but still a single pipeline for a certain event like this, but that's a lot of infrastructure complexity Maybe a simple further enhancement to this library could involve an optional delimiter field for total instances, with the logic |
Problem
While TMDB themselves still do not apply a rate limit, their CDN provider CloudFront does.
When sending many requests in a short timespan, a HTTP error is likely to be thrown (due to a
429 - Too Many Requests
HTTP response). See complete error object below.Here is an excerpt from the error, indicating it's origin from CloudFront:
This was confirmed by TMDB employees a while ago:
Changes made
String.prototype.substr
withString.prototype.slice
.Complete
AxiosError
object: