-
Notifications
You must be signed in to change notification settings - Fork 258
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
Response caching with conditional requests #1124
Comments
Hi @setchy 👋🏾 – I can look into this. I'll do some research and do a small write-up for you to review before I start. Let me know if that works for you 😄 |
Hey @setchy – I have some options on how we can implement this:
Somehow I sense that I could be overcomplicating the implementation? Let me know what you think, thanks! |
Great research @dammy95. I don't have a strong view on the best way to implement this. Happy to try and few things and adapt as we move forward |
We're already using axios, so I'd suggest taking advantage of that and its large community! Easy solutions are usually the best ones 😁🚀 |
The other option would be to go down this route #823 |
@setchy how does that tackle this issue? Do they have built-in caching or something into their client? |
I believe so - octokit/octokit.js#890 (comment) It also has built in throttling for primary and secondary quota exhaustion |
Alright!!! Then that looks like a great solution to me 🚀 @dammy95, as the person assigned to the issue, is that okay with you? |
Yupp that sounds great to me @afonsojramos @setchy – I'll look in to the octokit refactor solution and create a PR for review 🚀 |
I guess there is also the option of using react-query (or similar) - https://www.welcomedeveloper.com/posts/managing-cache-react-query/ |
Might be the most extensible (and widely-used) option honestly, don't know how I forgot about that one 😬 |
Another library I saw this week - https://github.com/alexcambose/custom-cache-decorator |
Could also completely replace axios with https://www.npmjs.com/package/make-fetch-happen (the fetch client used by npm) |
Notifications are fetched by an interval which is not ideal, because if the connection is slow requests can stack and make multiple requests at once. I'm not familiar with the way electron runs timing functions, but a better way would be to use As an alternative, I can suggest using react-query which handles caching, retries, interval fetching, and request deduplication. Unless it was considered and didn't fit the project. |
📝 Provide a description of the new feature
Add response caching so that we can enable additional features (ie: show read notifications, fetch >50 (all) notifications) without the concerns of being rate-limited.
Using conditional requests should be sufficient, which requires us to store and use response
etag
orlast-modified
headers in subsequentGET
requests to the same resource.Changes required to https://github.com/gitify-app/gitify/blob/main/src/utils/api/request.ts
➕ Additional Information
No response
The text was updated successfully, but these errors were encountered: