feat(backend): cache Discord API requests - #574
Conversation
|
@stijnvdkolk I understand from @Rocked03 you’re working on something related to guild memberships. We figure you might have some thoughts? |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
1031389 to
9abb2cc
Compare
Yeah I did some stuff with guild membership caching in #562 so we don't always fetch the new guild memberships. Will look through this PR sometime tomorrow 🙌 |
bumping :) |
Hope to be able to get to it today, got some other things that came in between yesterday! |
| return await fetchWithRetries(url, init, retryOptions); | ||
| } | ||
|
|
||
| const cacheKey = `${url.pathname}${url.search}`; |
There was a problem hiding this comment.
It's global, not per user.
There was a problem hiding this comment.
Saw you made it based on token, is that smart? There is a part of the token that is the user id, cant we just only take that part and leave the rest out? Might be a bit more secure
There was a problem hiding this comment.
is that smart
Not really. If you snoop around the commit history you’ll see that I described it as rudimentary user scoping or something like that 😅
Realistically I’m not that worried because it’s in memory in the backend. My bigger issue with this is just that it’s rather inelegant. I didn’t realise the user ID was baked into the JWT; I’ll look into that. It was my first instinct to use user ID, but I thought I had to go through 3–4 layers of dependency injection
My other gripe is that if you provide the same search params in a different order, it’s a cache miss. Originally hoped keys could just be deterministically derived from the request, but might end up putting burden on the dev to come up with sensible keys like TanStack QueryKeys. Not sure yet
All of which is to say: still working on it! 👷
| } | ||
|
|
||
| class DiscordApiClient { | ||
| declare baseUrl: string; |
There was a problem hiding this comment.
Are you sure that we need declare here?
There was a problem hiding this comment.
Apparently so 🤷
Property 'baseUrl' does not exist on type 'DiscordApiClient'.ts(2339)| function logWithTag(...args: Parameters<typeof console.debug>) { | ||
| console.debug(styleText(["dim"], "[DiscordApiClient]"), ...args); | ||
| } |
There was a problem hiding this comment.
I feel like we are using a variants of this in multiple places, can we pull this out and make a full logger setup? Might be worth it to do this in a separate PR.
There was a problem hiding this comment.
Def on board with setting up proper logging. Just realised we don’t even have Morgan (or equivalent) at the moment? 🥴
When you say pull this out, do you mean just remove all logging-related additions from this PR?
There was a problem hiding this comment.
No, just keep it like this for now, and pick it up separately, there is a separate issue for it, but we could look into a short time fix to streamline it a bit what we already have if you want by just copying these functions in a shared file that we can use in the other places too, before we switch to a better logging solution
|
Any chance we can give this a nudge please? |
Oh, are you wanting to release this mid-event? It’s a bit of a high-risk change |
| function sanitizeRequestInit(requestInit: RequestInit): RequestInit { | ||
| const headersInit = requestInit.headers; | ||
| if (headersInit === undefined) return requestInit; | ||
| const headers = new Headers(headersInit); | ||
| headers.delete("Authorization"); | ||
| return { ...requestInit, headers }; | ||
| } |
There was a problem hiding this comment.
Would’ve been nice to keep the whatever shape HeadersInit was supplied in, but:
Headersinterface matches case-insensitively, whereas[string, string][]and{ [key: string]: string }requires more manual work; and- normalising for a production log feels fine to me. In local dev we can just
console.logthe entire thing with no redactions
3 participants
Note
Had started work on this, but then momentum trailed off.
This PR is missing unit tests and has not been comprehensively manually tested. Cursory smoke testing in local dev environment hasn’t raised any flags, but I’m not comfortable deploying without more thorough testing.
You’ll see I’ve implemented
cache.invalidate(), but don’t actually call it anywhere; but only because I haven’t reviewed the endpoints to see which ones warrant imperative invalidation.🥞 Stack