feat: query onError & onComplete callbacks

[fabis94]

Fixes / Enhancements

Added onError & onCompleted callbacks to the Query widget and useQuery hook. They work pretty much exactly as the same callbacks for Mutations.

Fixes: #954

Let me know if anything needs changing, I'm new to the Dart & Flutter ecosystem, but I'll try my best!

[budde377]

Great work! Thanks for contirbuting. I've added a few comments. Please don't hesitate to ask questions if something isn't clear or you don't agree on!

[codecov]

Codecov Report

Merging #1097 (f782ac1) into main (ff6fda6) will decrease coverage by 0.59%.
The diff coverage is 24.13%.

@@            Coverage Diff             @@
##             main    #1097      +/-   ##
==========================================
- Coverage   59.57%   58.97%   -0.60%     
==========================================
  Files          41       41              
  Lines        1561     1587      +26     
==========================================
+ Hits          930      936       +6     
- Misses        631      651      +20     

Impacted Files	Coverage Δ
packages/graphql/lib/src/core/query_options.dart	49.10% <11.76%> (-4.24%)	⬇️
...ackages/graphql/lib/src/core/observable_query.dart	38.46% <12.50%> (-1.54%)	⬇️
...s/graphql_flutter/lib/src/widgets/hooks/query.dart	85.00% <100.00%> (+3.75%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff6fda6...f782ac1. Read the comment docs.

[vincenzopalazzo]

Great Job, this is a very well done job, and happy to see that you are new on the repository.

I have a couple of style comments and also I would like your idea to put the QueryCallbackHandler in a new file maybe query_callback.dart?

and also move the following code in a new file called types.dart, I'm not a fan of the typedef but maybe if we refactor all in one class I can have a reason change my mind :)

typedef OnQueryCompleted = FutureOr<void> Function(dynamic data);
typedef OnQueryError = FutureOr<void> Function(OperationException? error);

[vincenzopalazzo]

In addition, can rename the commit header to feat: added onError&onComplete callbacks, we use this message to generate the CHANGELOG.md file

[fabis94]

@vincenzopalazzo @budde377 hi, thanks for your comments! i've resolved all of the issues you had commented about

[vincenzopalazzo]

Just some questions on the syntax type used.

Sorry, but I considered an important feature for the named optional parameter, because make the call of the method less messy.

[vincenzopalazzo]

LGTM, and again great job! Let's see what the CI says.

In addition, sorry if my comments can sound frustrating, but it is not I'm only a person that care to the elegance of a programming language :)

[budde377]

Thanks for updating! I've added a few comments, but this is really close to ship!

[fabis94]

i've fixed the remaining issues :)

[vincenzopalazzo]

The CI was saying that there is some test to update

00:14 +0 -1: loading /home/runner/work/graphql-flutter/graphql-flutter/packages/graphql_flutter/test/widgets/query_test.dart                                                                           
00:14 +0 -1: loading /home/runner/work/graphql-flutter/graphql-flutter/packages/graphql_flutter/test/widgets/subscription_test.dart [E]                                                                
  Exception: the Dart compiler exited unexpectedly.
  package:flutter_tools/src/base/common.dart 10:3  throwToolExit
  package:flutter_tools/src/compile.dart 784:9     DefaultResidentCompiler._compile.<fn>
  dart:async/zone.dart 1434:47                     _rootRunUnary
  dart:async/zone.dart 1335:19                     _CustomZone.runUnary
  
test/widgets/query_test.dart:57:9: Error: Type 'OnQueryComplete' not found.
  final OnQueryComplete? onQueryComplete;
        ^^^^^^^^^^^^^^^


test/widgets/query_test.dart:[60](https://github.com/zino-hofmann/graphql-flutter/runs/6025755389?check_suite_focus=true#step:5:60):9: Error: Type 'OnQueryError' not found.
  final OnQueryError? onQueryError;
        ^^^^^^^^^^^^


test/widgets/query_test.dart:57:9: Error: 'OnQueryComplete' isn't a type.
  final OnQueryComplete? onQueryComplete;
        ^^^^^^^^^^^^^^^
test/widgets/query_test.dart:60:9: Error: 'OnQueryError' isn't a type.
  final OnQueryError? onQueryError;
        ^^^^^^^^^^^^


test/widgets/query_test.dart:114:9: Error: No named parameter with the name 'onComplete'.
        onComplete: widget.onQueryComplete,
        ^^^^^^^^^^
/opt/hostedtoolcache/flutter/2.10.4-stable/x[64](https://github.com/zino-hofmann/graphql-flutter/runs/6025755389?check_suite_focus=true#step:5:64)/.pub-cache/hosted/pub.dartlang.org/graphql-5.1.1/lib/src/core/query_options.dart:13:3: Context: Found this candidate, but the arguments don't match.
  QueryOptions({
  ^^^^^^^^^^^^


lib/src/widgets/hooks/query.dart:37:7: Error: Method not found: 'QueryCallbackHandler'.
      QueryCallbackHandler(options: options).callbacks,
      ^^^^^^^^^^^^^^^^^^^^


lib/src/widgets/hooks/query.dart:38:7: Error: No named parameter with the name 'removeAfterInvocation'.
      removeAfterInvocation: false,
      ^^^^^^^^^^^^^^^^^^^^^

[fabis94]

@vincenzopalazzo i can't verify because I can't re-run the CI action, but locally it seems that the dart pub get in the test command undoes the symlink between the packages and breaks everything. i've pushed in changes with that pub get call removed

the missing type related issues are definitely fixed for me locally once I symlink everything with melos

[vincenzopalazzo]

i can't verify because I can't re-run the CI action, but locally it seems that the dart pub get in the test command undoes the symlink between the packages and breaks everything. i've pushed in changes with that pub get call removed

Yeah! right this now is tricky, we should split the PR in two, but maybe I will test it locally and after I merge a will fix the CI

[fabis94]

@vincenzopalazzo how should i proceed here?

[vincenzopalazzo]

how should i proceed here?

Nothing from your side, I need to split the PR in two, but this required time, and now there are Italian holidays.

I will try to find some time for the end of the week

The problem here is that the breaking chain needs to be divided into two PRs because we want that graphql and graphql_flutter live as two different packages.

This was my missing during the review, sorry!

[vincenzopalazzo]

Fixed the analyzer error that makes the CI unhappy and squashes some commits with no meaning!