willianjusten / nba-remix

A simple app to show NBA games and scores/details.
https://nba.willianjusten.com.br
210 stars 31 forks source link

Caching responses #72

Closed gustavoguichard closed 2 years ago

gustavoguichard commented 2 years ago

For even snappier UX.

Reference

pr2

I don't have a complete understanding of NBA's API and want to see your ideas on this but we could make app/routes/$date route return a long cache for past games, light cache for active games and medium for future games.

I'll explain my caching decisions on comments through the diff, opinions on it are welcome.

vercel[bot] commented 2 years ago

Someone is attempting to deploy a commit to a Personal Account owned by @willianjusten on Vercel.

@willianjusten first needs to authorize it.

gustavoguichard commented 2 years ago

It'd be good to allow the vercel deployment preview for a better QA ;)

vercel[bot] commented 2 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/willianjusten/nba-remix/GQeS71UjWs5kvxW24v7XvYPCve4Z
✅ Preview: https://nba-remix-git-fork-gustavoguichard-gg-cach-fcff1b-willianjusten.vercel.app

willianjusten commented 2 years ago

Hey @gustavoguichard , thanks for another great PR, I'll have a look at the end of the day and then I can give some feedback =D

helderberto commented 2 years ago

Can you share more details about the error? I think it's because the MSW is in the global scope of every test.


About the abstraction, it feels good for me. What do you think @willianjusten?

willianjusten commented 2 years ago

Hey @gustavoguichard that's indeed something very interesting to have, and thanks for showing me the video, very insightful! So, some details that we need to have in mind here:

Since we use useRevalidateOnInterval hook to revalidate and refresh the page to get the new data in a "near realtime", we just need to take care about the time we're going to refetch and the time that we are going to cache, if not, we can get "stale" data, that it's not the desired state.

For now, what we have, is:

I think only the ones that we need this "real time" we can change to 20s, the others we can have some bigger caching time, without problems.


About the code, I liked the abstraction, looks clean and readable, with the comments, got even better. I'd just ask to get the "magical numbers" of time to cache and put them to the constants file.

gustavoguichard commented 2 years ago

Thanks for the directions. I'm not gonna have much time to contribute in the next couple days. I set every cache to 20s, only leaving the standings to 10min on the CND. If you feel like this is going to bring benefits in the next few days feel free to merge it, otherwise I'll improve it soon ;)

helderberto commented 2 years ago

Is this already done to be merged?

gustavoguichard commented 2 years ago

Hey @helderburato , still didn't have time to go further. I think it brings value already but if you guys don't merge it I'll be doing the changes within the next couple days

willianjusten commented 2 years ago

I have to say it's good to merge already and after you can open another PR if you want =)