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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

hagen00
Copy link

@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
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
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
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.

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.

2 participants