Skip to content
This repository has been archived by the owner on Nov 20, 2020. It is now read-only.

Add LazyQuery Hook #133

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ZielonyBuszmen
Copy link

I added LazyQuery Hook binding, because I cannot find it in this repository.

@mbirkegaard mbirkegaard self-requested a review July 24, 2020 14:33
Copy link
Collaborator

@mbirkegaard mbirkegaard left a comment

Choose a reason for hiding this comment

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

Hi @ZielonyBuszmen,

Thank you for doing this! The lack of bindings to useLazyQuery has been a real weak spot of this package.

There are a few things that don't work with the useLazyQuery hook (see the docs here).

The signature of useLazyQuery on the ts side is

function useLazyQuery<TData = any, TVariables = OperationVariables>(
  query: DocumentNode,
  options?: LazyQueryHookOptions<TData, TVariables>,
): [
  (options?: QueryLazyOptions<TVariables>) => void,
  QueryResult<TData, TVariables>
] {}
  • LazyQueryHookOptions<TData, TVariables> doesn't accept the skip parameter so neither should options here.
  • The QueryResult<TData, TVariables> returned by useLazyQuery has a called field. That field should be exposed in the bindings and used to set an appropriate value for the simple variant when it's not true (e.g. NotCalled)
  • The fetchData function on the ts side has this signature (options?: QueryLazyOptions<TVariables>) => void where
export interface QueryLazyOptions<TVariables> {
    variables?: TVariables;
    context?: Context;
}

so beware that the options passed to useLazyQuery are not the same as those passed to the fetchData function.

Then there's a few things to do for consistency with the rest of reason-apollo-hooks.

  • This hook should also accept context. I don't think that it's particularly widely used, but the query and mutation hooks have it.
  • useLazyQuery should be available from the top-level module (same as how ApolloHooks.re has definitions for useQuery, useMutation and useSubscription)

Regarding using useMemo to wrap fetchData, I think the intention is that it is callable multiple times even with the same variables (akin to refetching) so I don't think useMemo is appropriate here (and the memo comparison [|variables|] will only check referential equality anyway).

@mbirkegaard
Copy link
Collaborator

Just took a look at ApolloHooksMutation which wraps mutate in useMemo. I think that's a mistake for similar reasons as I mention above.

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

Successfully merging this pull request may close these issues.

2 participants