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

feat: Persistent cache #3721

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

feat: Persistent cache #3721

wants to merge 18 commits into from

Conversation

AmarTrebinjac
Copy link
Contributor

@AmarTrebinjac AmarTrebinjac commented Oct 24, 2024

Changes

Adds capability of persisting queries in localStorage using persistQueryClient.
This PR also includes moving the Squads from Boot to the persisted cache in local storage

Events

Did you introduce any new tracking events?

Experiment

Did you introduce any new experiments?

Manual Testing

Caution

Please make sure existing components are not breaking/affected by this PR

WT-2201
WT-2231

Preview domain

https://wt-2201.preview.app.daily.dev

Copy link

vercel bot commented Oct 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
daily-webapp ✅ Ready (Inspect) Visit Preview Oct 25, 2024 11:40am
1 Skipped Deployment
Name Status Preview Updated (UTC)
storybook ⬜️ Ignored (Inspect) Oct 25, 2024 11:40am

});

export const queryClientPersister = createSyncStoragePersister({
storage: typeof window !== 'undefined' ? window.localStorage : undefined,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Recommended way by TKDodo (core maintainer) to handle window being undefined in SSR

Copy link
Contributor

Choose a reason for hiding this comment

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

We actually have globalThis?.window you probably want to use that one.

persister: queryClientPersister,
dehydrateOptions: {
shouldDehydrateQuery: ({ queryKey }) => {
return persistedKeys.includes(queryKey[0] as RequestKey);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

shouldDehydrateQuery gets called for each query. Keys that match persistedKeys list will be persisted in localStorage

Copy link
Contributor

Choose a reason for hiding this comment

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

So adding more we should just use useQuery on the normal client and by this it will define which keys are persisted?
That's pretty neat actually.

}

const ItemsSkeleton = () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a quick loading animation for the squad section in the sidebar, as squads may not always be loaded at render time

Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably good to check how many squads the average normal user has to avoid shifts for the mass

Copy link
Contributor

Choose a reason for hiding this comment

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

I did a query on db, looks like only ~10.000 has more then 5, so probably not bad default

Copy link
Contributor

Choose a reason for hiding this comment

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

found a better query, i think

select count(*) from public.user u where (select count(1) from source_member sm where sm."userId" = u.id) > 5

9,009

and more then 1 is 55,435

Copy link
Contributor

Choose a reason for hiding this comment

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

so probably 1 is the best 😆

@AmarTrebinjac AmarTrebinjac marked this pull request as ready for review October 25, 2024 11:55
@AmarTrebinjac AmarTrebinjac requested a review from a team as a code owner October 25, 2024 11:55
Copy link
Contributor

@rebelchris rebelchris left a comment

Choose a reason for hiding this comment

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

Wow nice work!
Got some questions/remarks, but overall super neat work

@@ -20,6 +20,7 @@
"@dailydotdev/shared": "*",
"@tanstack/react-query": "^4.36.1",
"@tanstack/react-query-devtools": "^4.35.3",
"@tanstack/react-query-persist-client": "^4.36.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

It will conflict with: #3718
Just to note we might have to choose how to release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, also looking at your comments in that PR, I will have to change some syntax here too when it comes to queryKeys and queryFns

}

const ItemsSkeleton = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably good to check how many squads the average normal user has to avoid shifts for the mass

};

export const useSquads = (): UseSquads => {
const { isFetched: isBootFetched, user } = useContext(AuthContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we care if boot is fetched?
Basically if you have a user you can go right?

const { deleteSquad: deleteCachedSquad } = useBoot();
const { mutate } = useMutation(leaveSquad, {
onSuccess: () => {
persistedQueryClient.invalidateQueries({ queryKey: [RequestKey.Squads] });
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also mutate the persistent client instead of just invalidating it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, not quite sure I understand the question. 😅

You mean use the setQueryData function to update the cache instead of refetching?

});

export const queryClientPersister = createSyncStoragePersister({
storage: typeof window !== 'undefined' ? window.localStorage : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

We actually have globalThis?.window you probably want to use that one.

persister: queryClientPersister,
dehydrateOptions: {
shouldDehydrateQuery: ({ queryKey }) => {
return persistedKeys.includes(queryKey[0] as RequestKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

So adding more we should just use useQuery on the normal client and by this it will define which keys are persisted?
That's pretty neat actually.

Comment on lines +211 to +214
<PersistQueryClientProvider
client={persistedQueryClient}
persistOptions={persistedQueryClientOptions}
>
Copy link
Contributor

Choose a reason for hiding this comment

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

This will then underlying resolve the regular one as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The persistedQueryClient is just the regular queryClient with some additional options, and the provider has some additional utilities, so everything that is not related to persisting should be working as it always did

@@ -0,0 +1,38 @@
{
"name": "daily-apps",
Copy link
Contributor

Choose a reason for hiding this comment

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

we use pnpm, you probably accidentally run npm install, so this file should be removed

Copy link
Contributor

@capJavert capJavert left a comment

Choose a reason for hiding this comment

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

Looks pretty good from quick test 👏 , will look again after react-query is merged!

Comment on lines +53 to +59
const { data: squads, isLoading } = useQuery(
[RequestKey.Squads],
() => getSquads(user?.id),
{
enabled: isBootFetched && !!user,
},
);
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I noticed is that if I create a new squad on another device, or another tab not sharing local storage the cache will never be updated, probably only after stale time goes by. It should always invalidate in the bg I think.

@sshanzel
Copy link
Member

Do we have the information on how much time the cache is stored and how we clear the cache for when the user changes account (probably doesn't happen often, just something to consider if easy) as it might blow up the storage (for us at least)?

Maybe let's add some general details about this feature/references we can read to help the team understand how to use it properly.

@sshanzel
Copy link
Member

As we have moved to v5, and this is still referencing the v4, it might be good for us to park this in the meantime as there can be additional work with the dependencies plus the actual feature. Thoughts? cc @dailydotdev/web-team

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.

4 participants