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

set initial value for BehaviorSubject<SocketConnectionState>, update connectivity_plus #1433

Closed
wants to merge 0 commits into from

Conversation

hantrungkien
Copy link
Contributor

  • update connectivity_plus: ^6.0.3

  • set initial value for BehaviorSubject<SocketConnectionState>

When use toggleConnection will throw at

[ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: ValueStream has no value. You should check ValueStream.hasValue before accessing ValueStream.value, or use ValueStream.valueOrNull instead.
#0      BehaviorSubject.value (package:rxdart/src/subjects/behavior_subject.dart:146:5)
#1      SocketClient._listenToToggleConnection.<anonymous closure> (package:graphql/src/links/websocket_link/websocket_client.dart:264:40)
#2      _RootZone.runUnaryGuarded (dart:async/zone.dart:1594:10)
#3      _BufferingStreamSubscription._sendData (dart:async/stream_impl.dart:339:11)
#4      _BufferingStreamSubscription._add (dart:async/stream_impl.dart:271:7)
#5      _MultiStreamController.addSync (dart:async/stream_impl.dart:1101:36)
#6      _MultiControllerSink.add (package:rxdart/src/utils/forwarding_stream.dart:130:35)
#7      _TakeUntilStreamSink.onData (package:rxdart/src/transformers/take_until.dart:13:31)

@vincenzopalazzo
Copy link
Collaborator

the crash that you are reporting is something that it is caused by connectivity_plus?

This is unclear to me, sorry!

In addition, can you add the stacktrace in the commit body as out dev guide explain?

@hantrungkien
Copy link
Contributor Author

@vincenzopalazzo I'm so sorry for confusing you. My PR includes two changes:

  1. Upgrate: connectivity_plus: ^6.0.3 for graphql_flutter package.
  2. Resolve: error log is throwed from graphql package althought it doesn't make any crash.

I think it's simple PR so want to merge into one commit.

Thanks for answering!

@vincenzopalazzo
Copy link
Collaborator

Can you add them inside the commit body message?

You should be able to edit it with git commit --amend

@hantrungkien
Copy link
Contributor Author

@vincenzopalazzo I've updated. Please review it for me.

Thank you very much!

@vincenzopalazzo
Copy link
Collaborator

It is missing the stacktrace inside the commit body as suggested before

@hantrungkien
Copy link
Contributor Author

@vincenzopalazzo please review help me again. Many thanks!

@@ -15,7 +15,7 @@ dependencies:
meta: ^1.7.0
path_provider: ^2.0.1
path: ^1.8.0
connectivity_plus: ^6.0.1
connectivity_plus: ^6.0.3
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not super convinced about this, there is any reason to add this constraint?

if you want to use version 6.0.3 also with the current mainline version you should be able to do right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vincenzopalazzo sorry for late reply. I'm using

dependencies:
  graphql: ^5.2.0-beta.7
  graphql_flutter: ^5.2.0-beta.6
  connectivity_plus: ^6.0.3

but get a error when flutter pub get

Resolving dependencies... 
Because * depends on graphql_flutter ^5.2.0-beta.6 which depends on connectivity_plus ^5.0.0, connectivity_plus ^5.0.0 is required.
So, because * depends on connectivity_plus ^6.0.3, version solving failed.
Screenshot 2024-05-26 at 13 40 18

and this is from pub_cache dir on my local

Screenshot 2024-05-26 at 13 53 29

Maybe you don't release yet after this commit was merged

fb0f708

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo May 26, 2024

Choose a reason for hiding this comment

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

Ah ok, Because * depends on graphql_flutter ^5.2.0-beta.6 which depends on connectivity_plus ^5.0.0, connectivity_plus ^5.0.0 is required. from this I do not think you need to bump the connectivity_plus again.

We already did in #1428 I just need to release the new beta

Can you revert this change and leave the connectivity_plus: ^6.0.1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vincenzopalazzo yes of course but I think 6.0.3 is better because it has some fixes.

Sooner or later we still need to upgrade it because if app doesn't depend directly it will use by transitive. How do you about think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Make sense so, but this needs to be justified with another commit that links the changelog that you are now pointing to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these are 2 important commits:

v6.0.2: fluttercommunity/plus_plugins#2763

v6.0.3: fluttercommunity/plus_plugins#2836

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, divide your PR into two separate commits, wherein the first one you fix the crash and in the second one you upgrade the connectivity_plus

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vincenzopalazzo I got it. Many thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vincenzopalazzo Please check help me again!

@vincenzopalazzo
Copy link
Collaborator

Thanks @hantrungkien now the last thing is dropping the commit that we do not need

https://stackoverflow.com/a/1338758/10854225

You should drop the following commits:

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