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

RJS-2649: Add more additional overload to useQuery to allow exhaustive-deps to work #6819

Merged
merged 7 commits into from
Oct 2, 2024

Conversation

bimusiek
Copy link
Contributor

What, How & Why?

Related to #6284

Comment on lines 2 to 3
"name": "@hernas/realm-react",
"version": "0.9.1",
Copy link
Member

Choose a reason for hiding this comment

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

Please revert these 🙂

@kraenhansen
Copy link
Member

Very interesting approach - I'd be happy to merge this, although this hook is turning into a bit of an overload nightmare 🙈

@kraenhansen kraenhansen changed the title Add more additional overload to useQuery to allow exhaustive-deps to work RJS-2649: Add more additional overload to useQuery to allow exhaustive-deps to work Jul 26, 2024
@bimusiek
Copy link
Contributor Author

@kraenhansen Updated. Not sure why my npm is changing lock to dev: true everywhere, but I have manually reverted it back to what is on the main.

Crossing my fingers to have this change on next release, we will finally be able to use official package rather than our fork 👍

/* eslint-disable-next-line react-hooks/rules-of-hooks -- We're calling `useQuery` once in any of the brances */
return useQuery(typeOrOptions, Array.isArray(queryOrDeps) ? queryOrDeps : deps);
return useQuery(
{ ...(depsOrPartialOptions as QuaryHookPartialOptions<T>), query: typeOrOptionsOrQuery as QueryCallback<T> },
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to write this using a type assert function instead? I.e. something which test the value and return : assert arg is QueryHookPartialOptions<T> instead of using the as QueryHookPartialOptions<T>?

@@ -31,6 +31,8 @@ export type QueryHookOptions<T> = {
keyPaths?: string | string[];
};

export type QuaryHookPartialOptions<T> = Omit<QueryHookOptions<T>, "query">;
Copy link
Member

Choose a reason for hiding this comment

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

A small typo:

Suggested change
export type QuaryHookPartialOptions<T> = Omit<QueryHookOptions<T>, "query">;
export type QueryHookPartialOptions<T> = Omit<QueryHookOptions<T>, "query">;

I wonder if it would be easier to declare a base type (without the query) and declare QueryHookOptions<T> as a union with that base type and { query: ... }?

@bimusiek
Copy link
Contributor Author

@kraenhansen Typeguards were a good suggestion. I went further and refactored the code quite a bit to make it more readable.

@kraenhansen
Copy link
Member

kraenhansen commented Aug 12, 2024

Just did a quick read through ... and I like it! 💙 It reads much better now. Thanks a lot!

@bimusiek
Copy link
Contributor Author

bimusiek commented Oct 2, 2024

@kraenhansen Is there a timeline for when is this going to be merged? We would love to switch from fork to official realm react package.

@kraenhansen kraenhansen merged commit 5bec6e4 into realm:main Oct 2, 2024
25 of 27 checks passed
@kraenhansen
Copy link
Member

This was just published as @realm/[email protected] 👍 Thanks for the nudge 🙂

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants