-
-
Notifications
You must be signed in to change notification settings - Fork 628
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
Upgrade packages in graphql pubspec.yaml and graphql_flutter pubspec.yaml #1463
Conversation
probably there is some regression? |
If I remove the line I'm unable to figure out why |
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. |
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 |
@vincenzopalazzo I have had a look at the source code and Hence, I have removed The tests that check disconnect still have
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:
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. |
There was a problem hiding this 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:
- Upgrade the pubspec version and test in graphql package
- Updagade the pubspec version in graphql_flutter
Thanks!
I have changed the dependency in The version in |
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
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/ |
288481c
to
568e4d8
Compare
@vincenzopalazzo I was trying to address this 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. |
d577c8e
to
cd7fe08
Compare
Using zino-hofmann/graphql-flutter#1463 for now.
Using zino-hofmann/graphql-flutter#1463 for now.
Using zino-hofmann/graphql-flutter#1463 for now.
…te tests to ignore missing closeCode
76b76a0
to
dd844b4
Compare
@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? |
Hello. |
Mh sorry for delay here
|
make 'melos run client_analyze' succeed
35fede4
to
3d4ef96
Compare
@vincenzopalazzo Anything else that is required? |
There was a problem hiding this 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
Using zino-hofmann/graphql-flutter#1463 for now.
Using zino-hofmann/graphql-flutter#1463 for now.
Upgrade packages in
graphql
pubspec.yaml andgraphql_flutter
pubspec.yaml to latest, where possible.