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

Subscription stream is not closed on SocketException #1183

Closed qbx2 closed 2 years ago

qbx2 commented 2 years ago

Hello. I want to display connection status to the graphql server. I created WebSocketLink with autoReconnect: false as following because I want to display red circle on disconnected/connecting state. I had no luck to get any control when SocketException is raised (I start the app without letting server up, or take server down after the app started)

var wsLink = WebSocketLink(args['wsEndpoint']!,
      config: const SocketClientConfig(
        autoReconnect: false,
        inactivityTimeout: null,
      ));
  void subscribeReadiness() {
    debugPrint('subscribeReadiness');

    var subscription = client
        .subscribe(SubscriptionOptions(
            document: gql(
      r'''subscription { isReady }''',
    )))
        .listen(
      (result) {
        debugPrint('onData:' + result.toString());

        if (result.hasException) {
          print(result.exception.toString());
          return;
        }

        if (result.isLoading || result.data == null) {
          print('awaiting result');
          return;
        }

        setState(() {
          isReady = result.data!['isReady']! as bool;
        });
      },
      onDone: () {
        debugPrint('onDone');
        setState(() {
          isReady = false;
        });
        Future.delayed(
            const Duration(seconds: 1), () => {subscribeReadiness()});
      },
      onError: (error) {
        debugPrint(error.toString());
      },
      cancelOnError: false,
    );
  }

Expected behavior onError, or at least onDone must be called. I've inspected websocket_client.dart code, but it seems not to have any code to close all streams in onConnectionLost().

device / execution context macos 12.4

qbx2 commented 2 years ago

May I submit a PR?

qbx2 commented 2 years ago

I think that I should not have added if (!config.autoReconnect) because of https://github.com/zino-hofmann/graphql-flutter/pull/1186/files#diff-c0e52f76bbabe11b61f02c62cfe93beb8d140080cf59cc24c6bf2a91b8487108R422 . However, without this, that's a breaking change.

vincenzopalazzo commented 2 years ago

Do you look inside it?

and I'm not scared of breaking change, I'm scared from bugs!

N.B: We do not have breaking change inside the library and the workflow, please read this https://github.com/zino-hofmann/graphql-flutter/blob/main/docs/dev/MAINTAINERS.md#types

The general rules is you can keep an old and new one use the deprecated flag and start the remove live cycle, if you can not keep the behaviours put a remove prefix in the commit, and remove the bug from the library.

We have a very long beta process to find as much bug we can in beta to put a clean code in production.