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

Upgrade packages in graphql pubspec.yaml and graphql_flutter pubspec.yaml #1463

Merged
merged 2 commits into from
Dec 13, 2024

Conversation

hagen00
Copy link
Contributor

@hagen00 hagen00 commented Oct 20, 2024

Upgrade packages in graphql pubspec.yaml and graphql_flutter pubspec.yaml to latest, where possible.

@hagen00 hagen00 changed the title Upgrade packages in pubspec.yaml and pubspec.yaml Upgrade packages in graphql pubspec.yaml and graphql_flutter pubspec.yaml Oct 20, 2024
@vincenzopalazzo
Copy link
Collaborator

Error: 84 tests passed, 2 failed.
--------------------------------------------------------------------------------

$ melos exec
  └> dart run test --coverage=coverage && dart run coverage:format_coverage --lcov --in=coverage --out=coverage.lcov --packages=.dart_tool/package_config.json --report-on=lib
     └> FAILED (in 1 packages)
        └> graphql (with exit code 1)

melos run client_test_coverage
  └> melos exec -- "dart run test --coverage="coverage" && dart run coverage:format_coverage --lcov --in=coverage --out=coverage.lcov --packages=.dart_tool/package_config.json --report-on=lib"
     └> FAILED
ScriptException: The script client_test_coverage failed to execute
make: *** [Makefile:38: ci_coverage_client] Error 1

probably there is some regression?

@hagen00
Copy link
Contributor Author

hagen00 commented Oct 21, 2024

If I remove the line expect(socketClient.socketChannel!.closeCode, isNotNull); in disconnect via dispose and disconnect via dispose graphql-transport-ws then all tests pass.

I'm unable to figure out why closeCode is now null. There seem to be a number of issues related to closeCode on web_socket_channel's Github issue tracker.

@vincenzopalazzo
Copy link
Collaborator

Unfortunatly I have not enough bandwidth to debug this issue, I do not want to promise that I will review, it because I am not in a good period, sorry!

Probably if I were you I would upgrade one dependency at a time(and make a single commit per dependency and per graphql package) in this way you can isolate what is going wrong.

@hagen00
Copy link
Contributor Author

hagen00 commented Oct 22, 2024

I have run some tests and it seems the problem lies with web_socket_channel no longer setting closeCode. I was able to reproduce this easily with the same code working on version 2.4.5 and not working on 3.0.1

I have submitted an issue: dart-lang/web_socket_channel#383

@hagen00
Copy link
Contributor Author

hagen00 commented Oct 22, 2024

@vincenzopalazzo I have had a look at the source code and closeCode is not necessary for graphql-flutter to work. The websocket is still being closed successfully, it's just that the closeCode is not set, but by the looks of it, the graphql-flutter code does not require it.

Hence, I have removed expect(socketClient.socketChannel!.closeCode, isNotNull) in two places and now all tests pass.

The tests that check disconnect still have expects that indicate things working e.g this still passes fine:

await expectLater(
        socketClient.connectionState,
        emitsInOrder([
          SocketConnectionState.connected,
          SocketConnectionState.notConnected,
        ]),
      );

The benefit of having a new version of graphql-flutter, supporting web_socket_channel 3.0.1 are that code bases can now be upgraded, since a lot of packages now require web_socket_channel 3.0.1

Hopefully this can be merged now? The only thing that will still need updating (not sure how to do that) is to fix this:

dependencies:
  graphql: ^5.2.0-beta.8 # --> this will need to change on merge? Not sure. 

I will monitor dart-lang/web_socket_channel#383 and if they fix closeCode (or indicate where the problem lies), I will update the tests again to check for this and do a new PR.

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense, but we just need to improve the git history sorry, because I need to have the ability to git blame or git revert.

So I suggest to make this PR in 2 different commit:

  1. Upgrade the pubspec version and test in graphql package
  2. Updagade the pubspec version in graphql_flutter

Thanks!

packages/graphql/test/websocket_test.dart Outdated Show resolved Hide resolved
@hagen00
Copy link
Contributor Author

hagen00 commented Nov 5, 2024

I have changed the dependency in graphql_flutter from graphql: ^5.2.0-beta.8 to graphql: ^5.2.0-beta.9. I hope that was correct?

The version in graphql is still version: 5.2.0-beta.9. I'm not sure if that also needs incrementing i.e to version: 5.2.0-beta.10.

@vincenzopalazzo
Copy link
Collaborator

I have changed the dependency in graphql_flutter from graphql: ^5.2.0-beta.8 to graphql: ^5.2.0-beta.9. I hope that was correct?

This should not be required, it is the dependencies resolution that will fix it for you, In this way you are forcing everyone to upgrade the packages, that can be a huge pain something

The version in graphql is still version: 5.2.0-beta.9. I'm not sure if that also needs incrementing i.e to version: 5.2.0-beta.10.

package version are managed by the changelog tool, so leave them as it is.

This PR is missing just a big clean up inside the git history, 7 comments should be reduced in a smaller one.

A good reading https://cbea.ms/git-commit/

@hagen00
Copy link
Contributor Author

hagen00 commented Nov 6, 2024

@vincenzopalazzo I was trying to address this 2. Updagade the pubspec version in graphql_flutter. I have now reverted it to be as before, i.e graphql_flutter is now once again graphql: ^5.2.0-beta.8

I'm not entirely sure what else I need to do. If there are more changes required, please suggest them and I'll accept.

I have also rebased into a single commit.

@hagen00 hagen00 force-pushed the hagen00-upgrade-packages branch from d577c8e to cd7fe08 Compare November 6, 2024 18:56
Chralu added a commit to archethic-foundation/libdart that referenced this pull request Nov 7, 2024
Chralu added a commit to archethic-foundation/libdart that referenced this pull request Nov 7, 2024
Chralu added a commit to archethic-foundation/libdart that referenced this pull request Nov 7, 2024
@hagen00 hagen00 force-pushed the hagen00-upgrade-packages branch from 76b76a0 to dd844b4 Compare November 7, 2024 17:15
@hagen00
Copy link
Contributor Author

hagen00 commented Nov 14, 2024

@vincenzopalazzo is there a problem with this PR? Is the formatting incorrect? Must I redo things so that there are no formatting changes? Is everything fine otherwise i.e 2 commits, with the 2 commit messages?

@redDwarf03
Copy link

Hello.
Is the merge of this PR considered please?
@hagen00, are you planning a fork, as we're currently blocked from generating libs on pub.dev using this PR?
Thank you

@vincenzopalazzo
Copy link
Collaborator

Mh sorry for delay here

Run make ci_fmt_client
dart pub global run melos run client_analyze --no-select
melos run client_analyze
  └> melos exec -c 1 -- "dart format --set-exit-if-changed . && dart analyze . --fatal-infos"
     └> RUNNING

$ melos exec
  └> dart format --set-exit-if-changed . && dart analyze . --fatal-infos
     └> RUNNING (in 2 packages)

--------------------------------------------------------------------------------
example:
Formatted 3 files (0 changed) in 0.24 seconds.
Analyzing ....
No issues found!
example: SUCCESS
--------------------------------------------------------------------------------
graphql:
Formatted test/websocket_test.dart
Formatted 55 files (1 changed) in 0.[7](https://github.com/zino-hofmann/graphql-flutter/actions/runs/11727989636/job/32682149439?pr=1463#step:5:8)8 seconds.
--------------------------------------------------------------------------------

$ melos exec
  └> dart format --set-exit-if-changed . && dart analyze . --fatal-infos
     └> FAILED (in 1 packages)
        └> graphql (with exit code 1)

melos run client_analyze
  └> melos exec -c 1 -- "dart format --set-exit-if-changed . && dart analyze . --fatal-infos"
     └> FAILED
ScriptException: The script client_analyze failed to execute
make: *** [Makefile:32: ci_fmt_client] Error 1

make 'melos run client_analyze' succeed
@hagen00 hagen00 force-pushed the hagen00-upgrade-packages branch from 35fede4 to 3d4ef96 Compare December 10, 2024 18:48
@hagen00
Copy link
Contributor Author

hagen00 commented Dec 10, 2024

@vincenzopalazzo melos run client_analyze now succeeds and the commit history is still as it was before (2 commits, with the specific messages).

Anything else that is required?

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks! and sorry for delay

@vincenzopalazzo vincenzopalazzo merged commit 651d75c into zino-hofmann:main Dec 13, 2024
4 checks passed
redDwarf03 pushed a commit to archethic-foundation/libdart that referenced this pull request Dec 13, 2024
redDwarf03 pushed a commit to archethic-foundation/libdart that referenced this pull request Dec 13, 2024
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.

3 participants