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

Improve useQuery arguments so react-hooks/exhaustive-deps recognise missing deps #6259

Open
bimusiek opened this issue Nov 16, 2023 · 3 comments

Comments

@bimusiek
Copy link
Contributor

Problem

Currently there is no way to know if you forgot deps besides testing. We would love to use react-hooks/exhaustive-deps to recognise the missing deps in the useQuery function.

Solution

If we move the type argument as the last one, so example usage of useQuery:

  const results = useQuery<ImporterAuthenticationModel>(
    (objects) =>
      objects.filtered(query, ...args).sorted([
        ['importerId', false],
        ['title', false],
      ]),
    [query, args],
    ImporterAuthenticationModel.schema.name
  );

Then react-hooks/exhaustive-deps correctly recognises missing deps (as it expects deps to be on second position and callback to be on the first one like in useEffect).

Alternatives

No alternatives really. For our code base we copy-pasted useQuery source code and modified it to suit our needs, but I think it would be really beneficial to all developers using @realm/react.

Maybe alternative is to fork react-hooks/exhaustive-deps and make it work for realm-hooks :)

How important is this improvement for you?

Fairly niche but nice to have anyway

Feature would mainly be used with

Atlas Device Sync

@kneth
Copy link
Contributor

kneth commented Nov 20, 2023

@bimusiek Thank you for the suggestion. As you can guess, we didn't design @realm/react with react-hooks/exhaustive-deps fresh in our memory 😄

Maybe alternative is to fork ...

Maintaining additional code isn't such a good idea, and we should avoid it.

As @realm/react is still early in development, I suppose the proposed change is acceptable.

@takameyer
Copy link
Contributor

Ah, good catch. This would unfortunately be a breaking change, so we'll have to consider how we approach this.

@kneth
Copy link
Contributor

kneth commented Nov 21, 2023

unfortunately be a breaking change

Imho, the major version is 0, so we can allow to introduce breaking changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants