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: typed queries proposal #4313

Merged
merged 2 commits into from
Apr 10, 2025
Merged

feat: typed queries proposal #4313

merged 2 commits into from
Apr 10, 2025

Conversation

capJavert
Copy link
Contributor

@capJavert capJavert commented Mar 28, 2025

Changes

Check comments below ⬇️

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

Preview domain

https://typed-queries-proposal.preview.app.daily.dev

@capJavert capJavert self-assigned this Mar 28, 2025
Copy link

vercel bot commented Mar 28, 2025

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

Name Status Preview Updated (UTC)
daily-webapp ✅ Ready (Inspect) Visit Preview Apr 10, 2025 9:52am
1 Skipped Deployment
Name Status Preview Updated (UTC)
storybook ⬜️ Ignored (Inspect) Apr 10, 2025 9:52am

Comment on lines 95 to 104
export const postByIdQueryOptions = ({ id }: { id: string }) => {
return {
queryKey: getPostByIdKey(id),
queryFn: async () => {
const res = await gqlClient.request<PostData>(POST_BY_ID_QUERY, { id });

return res;
},
};
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of building custom hooks each time to fetch specific data this way we export all useQuery options in this creator function which we can then reuse in new hooks to get types automatically in queries, mutations and updates.

Comment on lines 84 to 88
const [getPost, updatePost] = useUpdateQuery(
postByIdQueryOptions({
id: post.id,
}),
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used as a special hook to replace updatePostCache and different helpers we have for updating different caches during optimistic actions. This accepts same parameters as useQuery so you are sure that updating and getting the data is typed and you don't make accidental mistake.

Comment on lines 90 to 91
useQuerySubscription(({ variables }) => {
const { type, entityId } = variables;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

In contrast to useMutationSubscription that you have to manually type and provide matcher to catch mutation events this hook is fully typed automatically due to it using same creator function as the useMutation.

Comment on lines 98 to 105
const newPost = getPost();
newPost.post.userState = {
...newPost.post.userState,
awarded: true,
};
newPost.post.numAwards = (newPost.post.numAwards || 0) + 1;

updatePost(newPost);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use getPost to get immutable copy of current data for the post query (with structuredClone) which we can directly mutate and then use updatePost to update the query without using queryClient. Again all typed to avoid mistakes:
image

{ productId: product?.id, type, entityId: entity.id, note },
],
mutationFn: award,
...awardMutationOptions(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as query mutation function we also do the same for mutations. We can then reuse the function in useQuerySubscription to listen to mutation events from other places avoiding props drilling and options passing all over the place.

} from '@tanstack/react-query';
import { useMutationSubscription } from './mutationSubscription';

export const useQuerySubscription = <
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not 100% about the name, it is basically typed wrapper over useMutationSubscription, WIP 😆

@rebelchris rebelchris requested a review from Copilot March 31, 2025 09:13
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces typed query functionality and associated hooks to support more robust query updates and subscriptions. Key changes include:

  • Adding new hooks: useUpdateQuery, useQuerySubscription, and a helper postByIdQueryOptions.
  • Enhancing the award flow by integrating awardMutationOptions and updating the PostActions component.
  • Minor refactoring in the translation and njord modules for better type exports and mutation configuration.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/shared/src/hooks/useUpdateQuery.ts Introduces a hook to get and update query data with type safety.
packages/shared/src/hooks/useQuerySubscription.ts Adds a hook for subscribing to mutation updates using a JSON.stringify-based comparison.
packages/shared/src/hooks/usePostById.ts Provides a new helper for creating query options based on a post id.
packages/shared/src/hooks/translation/useTranslation.ts Updates the translation hook to export its type.
packages/shared/src/graphql/njord.ts Adds awardMutationOptions to configure award mutations.
packages/shared/src/components/post/PostActions.tsx Updates award handling logic by replacing updatePostCache with the new query hooks.
packages/shared/src/components/modals/award/GiveAwardModal.tsx Refactors award mutation usage to adopt the new awardMutationOptions.
Comments suppressed due to low confidence (2)

packages/shared/src/hooks/useQuerySubscription.ts:23

  • Using JSON.stringify for comparing mutation keys can be brittle due to potential ordering differences. Consider using a deep equality check (for example, lodash isEqual) to improve reliability.
JSON.stringify(mutation.options.mutationKey) === JSON.stringify(mutationKey)

packages/shared/src/components/post/PostActions.tsx:98

  • Ensure that getPost() returns a defined object before using it to update properties. Adding a null/undefined check can help prevent potential runtime errors if the query data is missing.
const newPost = getPost();

@@ -211,3 +211,10 @@ export const getTransactions = async ({

return result.transactions;
};

export const awardMutationOptions = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ok, this is pretty neat actually! :)
Also promotes consistent re-use accross files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, queryKey and queryFn is tied together and then you just shove them in whatever hook and you get types automatically.

if (entityId !== post.id) {
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still wondering if some of this logic could be automated?
EG only passing the actual change and underlying logic can determine what the initial data is based on query key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically it can, if we split that each award mutation has different key then we can just update, currently all awards are under same mutation/key and this is why it has some additional checks.

@capJavert capJavert marked this pull request as ready for review March 31, 2025 12:06
@capJavert capJavert requested a review from a team as a code owner March 31, 2025 12:06
@capJavert capJavert requested review from omBratteng, ilasw, sshanzel, nensidosari and AmarTrebinjac and removed request for a team March 31, 2025 12:06
Base automatically changed from feat-credit-system to main April 9, 2025 13:42
@capJavert capJavert force-pushed the typed-queries-proposal branch from ad6d769 to 2aedadb Compare April 10, 2025 09:44
@capJavert capJavert force-pushed the typed-queries-proposal branch from 2aedadb to 3393584 Compare April 10, 2025 09:48
@capJavert capJavert enabled auto-merge (squash) April 10, 2025 09:52
@capJavert capJavert merged commit 1f0af4d into main Apr 10, 2025
10 checks passed
@capJavert capJavert deleted the typed-queries-proposal branch April 10, 2025 09:52
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