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

Lazy useQuery #119

Closed
danielkcz opened this issue Jun 22, 2019 · 9 comments · Fixed by apollographql/react-apollo#3214
Closed

Lazy useQuery #119

danielkcz opened this issue Jun 22, 2019 · 9 comments · Fixed by apollographql/react-apollo#3214
Assignees
Labels
confirmed Approved and will be worked on.

Comments

@danielkcz
Copy link

danielkcz commented Jun 22, 2019

Perhaps I am missing some more obvious solutions and patterns. Would love to hear about those :)

The problem

The known "problem" of hooks is an inability to call them conditionally. At the moment query needs to be executed conditionally based on user action (or some other side effect), it requires extra ceremony to the component. Value to be searched needs to be stored to state first to let useQuery notice it on next render AND the skip needs to be used to avoid calling query when it's not intended.

Workaround

Use the Query component (or write one with useQuery) which can be rendered conditionally. The render prop pattern does not fit well in the hooks world. It also requires writing the result into a state to re-render a whole component based on it. Lastly, conditional rendering can cause unnecessary DOM trashing if not treated carefully.

Poor solution

Grab the useApolloClient and call client.query directly. Once again it requires storing results into a state when an operation is done. Also tracking the "loading" and "error" means solving something that's already solved more carefully (within useQuery).

A custom hook like that can surely be tackled in user-land (and I do that), but it leaves an ugly blind spot for newcomers and possible bugs that might be solved better in a community solution.

Better solution

Add lazy option to useQuery and provide execute function. It will provide a nice declarative approach we are used to without any extra ceremony.

const SearchPage = () => {
  const { loading, data, execute } = useQuery(doc, { lazy: true })

  const onSearch = () => {
    execute({ variables })
  }

  return <div>
    <SearchForm onSubmit={onSearch} />
    <SearchResults loading={loading} results={loading ? null : data.results} />
  </div>
}

What needs to be solved is the initial loading state in such a case. It should be false probably.

Also, would be nice to get called prop similar to what useMutation has. That can be useful for a different rendering based on that, eg. instructions to the user. The "non-lazy" mode will toggle it to true pretty much immediately.

What happens when skip and lazy are used together? What will execute do? Should be an invariant error most likely. Or execute forcefully even over the skip?

How does it work together with polling? The first call to execute could be used to start polling. Subsequent ones could be for changing variables and interval and restarting polling.

@hwillson
Copy link
Member

I really like this idea @FredyC! I'll discuss this further with the team. Thanks!

@stonebat
Copy link

stonebat commented Jul 3, 2019

I ran into two issues with the current implementation of useQuery.

const { data, loading, error } = useQuery(SOME_GQL, {
variables: { input: { id: prop.id }
})

When prop.id is empty, I don't want to run the query. But my functional component has to run useQuery since useQuery as a hook cannot be wrapped in a conditional statement. The order of hooks must be always same.

I have a refresh icon. Clicking on it should refetch the data. But there is no way to trigger the useQuery again. I wrote a dummy mutation and set its refetchQueries. Yes 2 full round trips.

@danielkcz
Copy link
Author

@stonebat You can use skip option in these cases and I do that often. And useQuery returns the refetch which you can call any time (but skip cannot be used there). So yea, executable option definitely solves some of these issues.

@henrikgs
Copy link

henrikgs commented Jul 4, 2019

For the loading state issue, maybe it's possible to introduce a new state variable like Elm does, NotAsked? https://package.elm-lang.org/packages/krisajenkins/remotedata/latest/RemoteData#RemoteData

@hwillson hwillson added the confirmed Approved and will be worked on. label Jul 4, 2019
hwillson added a commit to apollographql/react-apollo that referenced this issue Jul 4, 2019
Initial work to support deferring `useQuery` execution until
some point in the future (after `useQuery` has been initialized).

If a `lazy` option is set to a boolean, `useQuery` will return a
tuple instead of a result object. The first position of the tuple
is the standard query result object, and the second position is
a function that can be triggered to kickstart the normal
`useQuery` query cycle.

If `lazy` is `true`, the initial query result with have a
`loading` status of `false`, and a `data` value of `undefined`.
This will only change after query execution has been triggered
using the delayed start function.

If `lazy` is `false`, a tuple is still returned, but
`useQuery` will still fire immediately (similar to calling
`useQuery` without a `lazy` option).

Fixes apollographql/apollo-feature-requests#119.
@hwillson
Copy link
Member

hwillson commented Jul 4, 2019

I've kickstarted this work in apollographql/react-apollo#3214. There is still work to do for sure (dealing with skip being called, verifying SSR results, consider a called prop, variable handling, tests, etc.), but feedback welcome!

@marconucara
Copy link

I think that you could totally remove skip parameter at this point, because that can be easily achieved by a conditionally call of execute() into the very next line of useQuery. Am I wrong?

@danielkcz
Copy link
Author

danielkcz commented Jul 11, 2019

@marconucara That's not the brightest idea, because you would end up in endless loop :) Execute would cause re-render and then you would call execute again... You would need useEffect with some conditions or do it as a reaction to the user action.

The skip, on the other hand, has its uses as well, for example when the query depends on some other data you don't have just yet.

@stonebat
Copy link

@FredyC Thanks! Invoking refetch from useQuery worked for my case. No more dummy mutation :)

@marconucara
Copy link

Of course I was wrong, you will need useEffect ad it will probably become too much. I'm still thinking that skip and lazy that are too similar, so just an idea: what about using execute without lazy? You could just set skip: true and execute the query when you need. I would even prefer to rename skip property to 'execute' and use it in reverse.

Also what should happened if you change lazy value in a next render? if it could trigger execution would be exactly like 'skip', right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed Approved and will be worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants