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

feat: query onError & onComplete callbacks #1097

Closed fabis94 closed 2 years ago

fabis94 commented 2 years ago

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: https://github.com/zino-hofmann/graphql-flutter/issues/954

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

codecov[bot] commented 2 years ago

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%) :arrow_down:
...ackages/graphql/lib/src/core/observable_query.dart 38.46% <12.50%> (-1.54%) :arrow_down:
...s/graphql_flutter/lib/src/widgets/hooks/query.dart 85.00% <100.00%> (+3.75%) :arrow_up:

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 commented 2 years ago

In addition, can rename the commit header to feat: addedonError&onCompletecallbacks, we use this message to generate the CHANGELOG.md file

fabis94 commented 2 years ago

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

vincenzopalazzo commented 2 years ago

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 :)

fabis94 commented 2 years ago

i've fixed the remaining issues :)

vincenzopalazzo commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@vincenzopalazzo how should i proceed here?

vincenzopalazzo commented 2 years ago

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!