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

Return limits and remaining count in headers #634

Merged
merged 4 commits into from
Aug 7, 2023

Conversation

ChaituVR
Copy link
Member

@ChaituVR ChaituVR commented Aug 4, 2023

🔴 Need a new version from snapshot-labs/keycard.js#7

Summary of changes:

  • Use new values returned from keycard.js
  • Return remaining count, limits and reset time in headers
    Untitled 3
  • Fallback to default rate limit if the max number of requests on keycard is reached

How to test:

  1. Install the new version of keycard.js with yarn link or wait for the new version to be released
  2. Run the server
  3. Go to http://localhost:3000/graphql?apiKey=1234 (replace apiKey if required)
  4. Try sending the requests and look into the networks tab in console
  5. It should display the correct limits and remaining count
  6. If it reaches our limit, it should fall back to the default rate limit (express-rate-limit)

@ChaituVR ChaituVR requested a review from wa0x6e August 4, 2023 19:57
Comment on lines +16 to +19
res.locals.keycardData = keycardData;
res.set('X-Api-Key-Limit', keycardData.limit);
res.set('X-Api-Key-Remaining', keycardData.remaining);
res.set('X-Api-Key-Reset', keycardData.reset);
Copy link
Member Author

Choose a reason for hiding this comment

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

These lint errors should be fixed once we install new version of keycard.js

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's merge and publish the new keycard.js version first, can not even yarn dev because of typescript error

Copy link
Member Author

@ChaituVR ChaituVR Aug 6, 2023

Choose a reason for hiding this comment

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

Using keycard.js from github for now. will use from npm once it is published

@codecov
Copy link

codecov bot commented Aug 6, 2023

Codecov Report

Merging #634 (5346755) into master (6f2649f) will not change coverage.
Report is 5 commits behind head on master.
The diff coverage is 0.00%.

@@          Coverage Diff           @@
##           master    #634   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          38      38           
  Lines        1662    1668    +6     
  Branches       38      38           
======================================
- Misses       1624    1630    +6     
  Partials       38      38           
Files Changed Coverage Δ
src/helpers/keycard.ts 0.00% <0.00%> (ø)
src/helpers/rateLimit.ts 0.00% <0.00%> (ø)
src/index.ts 0.00% <0.00%> (ø)

Copy link
Contributor

@wa0x6e wa0x6e left a comment

Choose a reason for hiding this comment

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

tAck

@ChaituVR ChaituVR merged commit 617c795 into master Aug 7, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants