zino-hofmann / graphql-flutter

A GraphQL client for Flutter, bringing all the features from a modern GraphQL client to one easy to use package.
https://zino-hofmann.github.io/graphql-flutter
MIT License
3.25k stars 620 forks source link

useQuery is causing unnecessary widget rebuilds #1104

Closed jamesgorman2 closed 2 years ago

jamesgorman2 commented 2 years ago

useQuery is causing unnecessary widget rebuilds.

Repro project is provided below. It uses tabs and print to switch between hook and widget graphql queries. It also includes a single state hook for reference.

To Reproduce

Download https://github.com/jamesgorman2/test_use_query

Observed results:

Expected behavior

device / execution context

additional context

budde377 commented 2 years ago

Thank you for the elaborate example. While I haven't had time to run it yet, it seems very strange, considering the fact that the query widget is implemented with hooks.

jamesgorman2 commented 2 years ago

No worries.

I thought so too, so wanted a bit of coverage (and to make sure I wasn't doing something silly). I couldn't figure out what was causing this from my surface reading, but I'm new to flutter so it's still a bit opaque to me.

budde377 commented 2 years ago

@jamesgorman2 I've had a look at your example. The re-builds are caused by subtle differences in how widgets work compared to hooks.

The hook will re-build every time the provided options change: In your example, this happens at every build. The reason why it happens at every build is that you're passing an anonymous function as the parserFn. At every build, the parserFn will be a new instance, hence forcing the options to fail equality check, and subsequently yielding a re-build.

You can solve this by moving your parserFn out as a static method:


  static parserFn(data) => StartUp.fromJson(data['startUp']);

  @override
  Widget build(BuildContext context) {
    final qhr = useQuery(
      QueryOptions(
        document: gql(getStartUp),
        parserFn: parserFn,
        variables: const {
          'id': '0',
        },
      ),
    );
    final result = qhr.result;
    ...

or use the useCallback hook:

  @override
  Widget build(BuildContext context) {
    final parserFn =
        useCallback((data) => StartUp.fromJson(data['startUp']), []);
    final qhr = useQuery(
      QueryOptions(
        document: gql(getStartUp),
        parserFn: parserFn,
        variables: const {
          'id': '0',
        },
      ),
    );
    final result = qhr.result;

or use the useMemoized hook:

  @override
  Widget build(BuildContext context) {
    final options =
        useMemoized(() => QueryOptions(
        document: gql(getStartUp),
        parserFn: (data) => StartUp.fromJson(data['startUp']),
        variables: const {
          'id': '0',
        },
      ), [getStartUp]);
    final qhr = useQuery(options);
    final result = qhr.result;

Why doesn't this fail on widgets? Good question. Under the hood, the useQuery will yield setState on the changes to the options. But no such functionality is implemented on the Query (and Mutation, Subscription, ...) widget. This means that receiving a new instance of the options won't update any state, thus not causing a re-render. You can think of the widget effectively short-circuiting the state update.

This behavior might sound counterintuitive, surely new options should cause a re-render? Well, maybe. But this would drastically complicate using the widgets causing infinite re-renders as you've just seen. Furthermore, without nifty helpers such as useMemoized and useCallback, these are non-trivial to resolve in native flutter yielding more boilerplate code. Instead, the widget will re-render with the state of the parent component change (something that makes sense most of the time) and can be forced to re-render (or not) using the key property.

I hope this makes sense!