gitify icon indicating copy to clipboard operation
gitify copied to clipboard

Response caching with conditional requests

Open setchy opened this issue 1 year ago • 12 comments
trafficstars

📝 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 avatar May 20 '24 11:05 setchy

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 avatar Jun 13 '24 19:06 dammy95

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!

dammy95 avatar Jun 14 '24 13:06 dammy95

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

setchy avatar Jun 17 '24 01:06 setchy

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

afonsojramos avatar Jun 17 '24 02:06 afonsojramos

The other option would be to go down this route https://github.com/gitify-app/gitify/issues/823

setchy avatar Jun 17 '24 09:06 setchy

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

afonsojramos avatar Jun 17 '24 09:06 afonsojramos

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

I believe so - https://github.com/octokit/octokit.js/issues/890#issuecomment-1335979030

It also has built in throttling for primary and secondary quota exhaustion

setchy avatar Jun 17 '24 09:06 setchy

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

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

afonsojramos avatar Jun 17 '24 11:06 afonsojramos

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

dammy95 avatar Jun 17 '24 12:06 dammy95

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

setchy avatar Jun 20 '24 21:06 setchy

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

afonsojramos avatar Jun 28 '24 04:06 afonsojramos

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

setchy avatar Jun 28 '24 13:06 setchy

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

Araxeus avatar Jul 05 '24 08:07 Araxeus

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.

akellbl4 avatar Jul 25 '24 00:07 akellbl4