heftapp / graphql_codegen

MIT License
143 stars 53 forks source link

Proposal: Make `WatchQueryOptions`'s `variable` field optional #351

Open chuganzy opened 3 months ago

chuganzy commented 3 months ago

Thank you for creating this amazing tool and publishing it, we've been loving it!

As titled, this PR is a proposal to make variable field in WatchQueryOption$XXX optional. It makes sense to make variable required for query because it starts the request as soon as query is called, however since watch can run lazily, it is more useful for us to make it blank at first, and fill once it is ready.

This is especially useful when using with hooks (ex: useWatchQuery) where some parts of variables are not available on Widget's mount.


// BEFORE
class Widget {
  Widget build(BuildContext context) {
    final query = useWatchQuery$XXX(
      variable: Variable(requiredValue: 'temporaryValue') // 😵 non-useful variable here.. 
    )
    final snapshot = useStream(query.stream, ...)
    useEffect(() {
      fetch() async {
        final resolvedRequiredValue = await something;
        query.variables = Variable(requiredValue: resolvedRequiredValue)
        query.fetchResults()       
      }
      fetch();
     }, [query]);
  }
  ...
}

// AFTER
class Widget {
  Widget build(BuildContext context) {
    final query = useWatchQuery$XXX() // ✨ no redundant options and much cleaner
    final snapshot = useStream(query.stream, ...)
    useEffect(() {
      fetch() async {
        final resolvedRequiredValue = await something;
        query.variables = Variable(requiredValue: resolvedRequiredValue)
        query.fetchResults()       
      }
      fetch();
     }, [query]);
  }
  ...
}

Please have a look and let me know! Thank you!

budde377 commented 3 months ago

Thank you for taking the time to write up this PR, @chuganzy! If we make the variables optional on the watch query, then I would make the "watches" invalid until you've mutated the variables. What I mean is that the result of the observableQuery is useless until you have updated the variables - which might be fine, but maybe there's a better approach than sending starting the observable query.

If I understand your example correctly, you are trying to re-fetch data after an action (effect in your case, but it could just as well have been a user action); for this, I would usually use the client directly.


class Widget {
  Widget build(BuildContext context) {
    final client = useGraphQLClient()
    useEffect(() {
      fetch() async {
        final resolvedRequiredValue = await something;
        await client.queryXXX( ... )
      }
      fetch();
     }, [query]);
  }
  ...
}

Does this solve your problem or am I missing something?

chuganzy commented 3 months ago

@budde377 Thank you for the reply!

As you pointed out, observableQuery is useless until running the query for the first time by either:

but, once it runs the query, the stream sends the first result and can update the view (widget) accordingly. This is why I think the variable in at least useWatchQuery can be optional because developers can decide when to start the query (where it actually finally needs variable field set), and can choose displaying whatever they can, similar to useMutation.

By doing it, we can also easily create something similar to Apollo's useLazyQuery. Although it is totally doable by using the client directly, client.query() is not enough to support cache update, streams, etc, and we need to make something very similar to useWatchQuery by ourselves (and types as well), which I wanted to avoid first..

Please let me know how you think. Thank you again!