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

Proposal: Make WatchQueryOptions's variable field optional #351

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

chuganzy
Copy link

@chuganzy chuganzy commented Jun 14, 2024

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
Copy link
Contributor

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
Copy link
Author

chuganzy commented Jun 22, 2024

@budde377 Thank you for the reply!

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

  • calling the fetchResults
  • setting the WatchQueryOptions's fetchResult option true (false by default) and listen to the stream

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!

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

Successfully merging this pull request may close these issues.

2 participants