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

Cache Focus View data #30

Merged
merged 2 commits into from
Apr 19, 2024
Merged

Cache Focus View data #30

merged 2 commits into from
Apr 19, 2024

Conversation

jdgarcia
Copy link
Contributor

This is just a simple version. I'll revisit the ideas we talked about it in the demo later but for now I'm switching focus to prioritize getting the core features in by Monday.

Copy link
Collaborator

@miggy-e miggy-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving, made one comment/suggestion

Comment on lines +109 to +118
if (data && data.timestamp > Date.now() - cacheTimeMinutes * 60 * 1000) {
return data.data;
}

const newData = await fetchFn();
if (newData) {
await storage.session.set({ [key]: { data: newData, timestamp: Date.now() } });
}

return newData;
Copy link
Collaborator

@miggy-e miggy-e Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we still call the fetch if we get a cache hit? We can return the cached data, but refetch in the background

Copy link
Contributor Author

@jdgarcia jdgarcia Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for rate limit reasons I want to say, not immediately. we basically want to introduce the concept of cache time vs stale time (something that react-query has), so it fetches in the background only if the state time has passed. I want to do make caching a lot better in a followup but I currently have another branch on top of this so I'll revisit caching after that branch(es) get merged.

@jdgarcia jdgarcia merged commit b6bca22 into main Apr 19, 2024
1 check passed
@jdgarcia jdgarcia deleted the focus-view-caching branch April 19, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants