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

Response caching with conditional requests #1124

Closed
setchy opened this issue May 20, 2024 · 14 comments · Fixed by #1414
Closed

Response caching with conditional requests #1124

setchy opened this issue May 20, 2024 · 14 comments · Fixed by #1414
Assignees
Labels
enhancement New feature or enhancement to existing functionality priority:high Items of high importance. Applicable to all users or use-cases

Comments

@setchy
Copy link
Member

setchy commented May 20, 2024

📝 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 or last-modified headers in subsequent GET 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

@setchy setchy added the enhancement New feature or enhancement to existing functionality label May 20, 2024
@setchy setchy mentioned this issue Jun 6, 2024
1 task
@dammy95
Copy link
Contributor

dammy95 commented Jun 13, 2024

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 😄

@dammy95
Copy link
Contributor

dammy95 commented Jun 14, 2024

Hey @setchy – I have some options on how we can implement this:

  1. Using axios-cache-interceptor. Here's a good read by its author. This creates an axios interceptor that adds the etag (or last-modified) headers to the request and return the appropriate response.

  2. Implement step 1) above without the axios-cache interceptor library, using localStorage to store the cached response.

Somehow I sense that I could be overcomplicating the implementation? Let me know what you think, thanks!

@setchy
Copy link
Member Author

setchy commented Jun 17, 2024

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

@afonsojramos
Copy link
Member

We're already using axios, so I'd suggest taking advantage of that and its large community! Easy solutions are usually the best ones 😁🚀

@setchy
Copy link
Member Author

setchy commented Jun 17, 2024

The other option would be to go down this route #823

@afonsojramos
Copy link
Member

@setchy how does that tackle this issue? Do they have built-in caching or something into their client?

@setchy
Copy link
Member Author

setchy commented Jun 17, 2024

@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

@afonsojramos
Copy link
Member

Alright!!! Then that looks like a great solution to me 🚀

@dammy95, as the person assigned to the issue, is that okay with you?

@dammy95
Copy link
Contributor

dammy95 commented Jun 17, 2024

Yupp that sounds great to me @afonsojramos @setchy – I'll look in to the octokit refactor solution and create a PR for review 🚀

@setchy setchy added the priority:high Items of high importance. Applicable to all users or use-cases label Jun 17, 2024
@setchy
Copy link
Member Author

setchy commented Jun 20, 2024

I guess there is also the option of using react-query (or similar) - https://www.welcomedeveloper.com/posts/managing-cache-react-query/

@afonsojramos
Copy link
Member

Might be the most extensible (and widely-used) option honestly, don't know how I forgot about that one 😬

@setchy
Copy link
Member Author

setchy commented Jun 28, 2024

Another library I saw this week - https://github.com/alexcambose/custom-cache-decorator

@setchy setchy mentioned this issue Jul 2, 2024
1 task
@Araxeus
Copy link
Contributor

Araxeus commented Jul 5, 2024

Could also completely replace axios with https://www.npmjs.com/package/make-fetch-happen (the fetch client used by npm)

@akellbl4
Copy link

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 setTimeout recursively. So when a request is finished a new timeout is started.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement to existing functionality priority:high Items of high importance. Applicable to all users or use-cases
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants