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

Todo #1

Open
14 of 23 tasks
nfriedly opened this issue Aug 25, 2023 · 10 comments
Open
14 of 23 tasks

Todo #1

nfriedly opened this issue Aug 25, 2023 · 10 comments
Labels
enhancement New feature or request

Comments

@nfriedly
Copy link
Member

nfriedly commented Aug 25, 2023

Moving this out of the readme so that I can publish the module without the todo list being immortalized.

Todo

  • transpiling
  • linting
  • ci
  • documentation
  • publish
  • auto-publish from CI
  • full test coverage
  • parse RateLimit-Policy headers
  • maybe expose parseCombinedRateLimitHeader in a way that allows it to be included without pulling in the entire library?
  • make parseCombinedRateLimitHeader operate in a single pass instead of 3
  • handle responses with more than one set of ratelimit headers
    • Probably two APIs: one that just returns a single RateLimit object, and one that returns an array of RateLimit objects.
  • more examples
  • test in Deno
  • test browsers
  • test in React Native
  • add support for more headers formats
  • Use a proper structured fields parser, such as one of these https://www.npmjs.com/search?q=rfc8941
@nfriedly nfriedly added the enhancement New feature or request label Aug 25, 2023
@nfriedly
Copy link
Member Author

It's published! https://www.npmjs.com/package/ratelimit-header-parser

@nfriedly
Copy link
Member Author

I'm also thinking about renaming the public API:

  • getRateLimit: replace current parseRateLimit
  • getRateLimits: new function that returns an array of all the ratelimits (if any)
  • parseCombinedRateLimitHeader: expose the current method. Maybe rename it to parseCombinedRateLimit or something else shorter?

Additionally, I'm thinking about Policy headers. In general, we could add policy info onto the RateLimit object, but it's possible to have a policy without the associated remaining or reset values, so what then? Maybe omit them from getRatelimit(), but have a setting on getRateLimits() to include policy-only objects in the results?

@gamemaker1
Copy link
Member

getRateLimit: replace current parseRateLimit

Maybe this could be getRateLimitInfo? Similar to the getRateLimitPolicy function I propose below.

getRateLimits: new function that returns an array of all the ratelimits (if any)

Could you please give me an example of when multiple rate limit headers could be returned?

parseCombinedRateLimitHeader: expose the current method. Maybe rename it to parseCombinedRateLimit or something else shorter?

Maybe we can export these functions as parsers? Each type of ratelimit headers gets its own parsers/<name>.ts file, and all these functions are exported from index.ts as a parsers object.


About the policy headers - should we make that a separate method, like getRateLimitPolicy? That way we don't need to worry about whether the policy appears with or without the rest of the headers.

@gamemaker1
Copy link
Member

Also, should we add a test-examples step to the CI, to run all the programs in the examples/ folder against the source code on every commit? It'll work like the external tests we have in express-rate-limit.

@nfriedly
Copy link
Member Author

Multiple rate limits example: User & Client limits: https://apidocs.imgur.com/

In general, I'd like to match the policy to the RateLimit and include all the details in a single object. However, the IETF draft had had a few examples with multiple policies but only one RateLimit header, which is the scenario I'm thinking about how to handle.

As for testing the examples, that sounds like a good idea, at least to ensure they don't throw. Validating the results might be a bit harder without making the examples themselves more tricky

@gamemaker1
Copy link
Member

Multiple rate limits example: User & Client limits: apidocs.imgur.com

In general, I'd like to match the policy to the RateLimit and include all the details in a single object. However, the IETF draft had had a few examples with multiple policies but only one RateLimit header, which is the scenario I'm thinking about how to handle.

Oh I see what you mean. I think we should do the following:

  • have only a getRateLimitInfo function
  • which returns an array of rate limit info objects
  • match each policy to its extracted info using the limit field

As for testing the examples, that sounds like a good idea, at least to ensure they don't throw. Validating the results might be a bit harder without making the examples themselves more tricky

I don't see the need to validate the results per se, we're already doing that in the tests. Just a no-throw is what I implemented in 9e6a84b, does that seem good?

@nfriedly
Copy link
Member Author

I feel like the normal case is going to be only retrieving a single rate limit, which is why I was thinking of differentiating between the singular and plural functions.

The singular one can also be a bit better optimized, because it can stop as soon as it finds a single match, whereas a plural version would have to do an exhaustive search each time. (Not that it makes a huge difference...)

I don't see the need to validate the results per se, we're already doing that in the tests. Just a no-throw is what I implemented in 9e6a84b, does that seem good?

Yeah, that's awesome!

@nfriedly
Copy link
Member Author

nfriedly commented Aug 31, 2023

One other thought: It currently can potentially return an object with NaN's and/or Invalid Dates if parsing some of the values fails. That's probably not very useful, but I'm on the fence about what to do in that scenario:

  • Throw an error?
  • Discard the entire RateLimit object and return undefined?
  • Discard only the invalid value and replace it with 0, Infinity, undefined, new Date(), etc?

I'm kind of leaning towards throwing an error as the default, but having a config option to do something else.

@gamemaker1
Copy link
Member

gamemaker1 commented Aug 31, 2023

The singular one can also be a bit better optimized, because it can stop as soon as it finds a single match, whereas a plural version would have to do an exhaustive search each time. (Not that it makes a huge difference...)

Yea, exactly. I wrote a library to parse the Accept header a while back and faced the same problem. I ended up doing this, where the singular function basically calls the plural one and returns the first result.

It currently can potentially return an object with NaN's and/or Invalid Dates if parsing some of the values fails.

Given that the Retry-After header and reset time are mentioned quite frequently in IETF's draft 7, I too think we should throw an error if we can't parse the date.

We could add a option to the ParserOptions type called strict, which defaults to true. If set to false, we don't throw an error and instead returned undefined in place of a Date.

@nfriedly
Copy link
Member Author

nfriedly commented Nov 6, 2023

After looking at ietf-wg-httpapi/ratelimit-headers#130 and https://datatracker.ietf.org/doc/rfc8941/ I added this to the list:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants