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

update gql_exec, gql_link, gql_http_link #1051

Closed toonvanstrijpwavy closed 2 years ago

toonvanstrijpwavy commented 2 years ago

Working on an update to graphql_codegen this makes use of gql_code_builder. Which makes use of gql_exec: ^0.4.0.

Fixes / Enhancements

vincenzopalazzo commented 2 years ago

update just the pub deb looks like not enough!

toonvanstrijpwavy commented 2 years ago

@vincenzopalazzo the tests are failing because I cannot change this line since it's making use of the published beta package: line

vincenzopalazzo commented 2 years ago

it is not the problem of dependencies, or better it is not ONLY, please take a look at this https://github.com/zino-hofmann/graphql-flutter/runs/5274956012?check_suite_focus=true#step:4:256

There is one reason that we stay with the fixed version of gql_exec: 0.3.0, we are not only lazy

toonvanstrijpwavy commented 2 years ago

@vincenzopalazzo Thanks for pointing that out. Should I set it fixed to 0.4.0 then?

codecov[bot] commented 2 years ago

Codecov Report

Merging #1051 (eb103a7) into main (66eb97a) will not change coverage. The diff coverage is 100.00%.

:exclamation: Current head eb103a7 differs from pull request most recent head 88f8a05. Consider uploading reports for the commit 88f8a05 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1051   +/-   ##
=======================================
  Coverage   55.81%   55.81%           
=======================================
  Files          41       41           
  Lines        1521     1521           
=======================================
  Hits          849      849           
  Misses        672      672           
Impacted Files Coverage Δ
packages/graphql/lib/src/core/query_manager.dart 70.51% <100.00%> (ø)

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 66eb97a...88f8a05. Read the comment docs.

toonvanstrijpwavy commented 2 years ago

@vincenzopalazzo could you let me know if this needs any more work?

vincenzopalazzo commented 2 years ago

@vincenzopalazzo could you let me know if this needs any more work?

we need to fix the CI if you rebase now you have melos to play with, if you want I can do it, but I have other things in my TODO list before fixing this PR

toonvanstrijpwavy commented 2 years ago

@vincenzopalazzo I updated it but somehow melos doesn't override the packages properly. Because of that graphql-flutter still uses the specified dependency in pubspec.yaml. We could fix this by adding a dependency_overrides but that's not the way to go I think.

vincenzopalazzo commented 2 years ago

@vincenzopalazzo I updated it but somehow melos doesn't override the packages properly. Because of that graphql-flutter still uses the specified dependency in pubspec.yaml. We could fix this by adding a dependency_overrides but that's not the way to go I think.

Right, in the near future blob PR will be not accepted anymore, in fact, one PR like that need to be divided in two different PR one for graphql and one for graphql_flutter

I updated it but somehow melos doesn't override the packages properly.

Melos is an env manager, not versioning management, this means that in the CI we test the package with clean flutter action, we don't take into consideration melos, so in this case, the dependency_overrides is the only solution. But I'm working to improve the ci and the release process, and we will have more strict rules for each PR.

The PR that will make this PR workable is open here #1055

toonvanstrijpwavy commented 2 years ago

@vincenzopalazzo Alright that makes sense! Then let's wait for the other PR to get merged.

vincenzopalazzo commented 2 years ago

Sorry for the delay here @toonvanstrijpwavy, I merged the change, if you rebase on the last main branch we can start to work on fixing the CI error in this PR

toonvanstrijpwavy commented 2 years ago

@vincenzopalazzo I'm not sure what you mean by that, If I did something wrong, or handled inappropriate please let me know? Besides that, thanks for taking a look at it! 😄

vincenzopalazzo commented 2 years ago

I'm not sure what you mean by that, If I did something wrong, or handled it inappropriately please let me know? Besides that, thanks for taking a look at it!

There is nothing wrong, but I just ask myself why in this case we need an empty map as a response. But it is not your fault, it is from the gql API