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

Make all async functions return a Future #31

Closed
synw opened this issue Apr 12, 2020 · 11 comments · May be fixed by #42
Closed

Make all async functions return a Future #31

synw opened this issue Apr 12, 2020 · 11 comments · May be fixed by #42

Comments

@synw
Copy link
Contributor

synw commented Apr 12, 2020

Would it be possible to return a future for all async methods? I have noticed that some async methods return void and not Future<void> which make then non awaitable. Is this on purpose?

Ex: I would like to await connect but can't because it does not return a future:

@override
  void connect() async {
    return _connect();
  }

Here _connect returns a future, and connect would be great this way:

 Future<void> connect() async {
   return await _connect();
 }
// or the short way
 Future<void> connect() async => _connect();

Is it possible or the actual way is by design?

@FZambia
Copy link
Member

FZambia commented Apr 12, 2020

@synw hello, the reason is that there are streams with events which can happen many times during client lifetime. For example client.connectStream. If you call connect then I suppose you expect it to yield when connection successfully established or an error occurred. But only for the first time right?

But you can also want to do some work as soon as connection established (write sth to a screen). But suppose you also need to do this work after each reconnect. In this case you also register your work in connectStream handler. In this case you need a way to only do your work once. In general we can solve this by adding isReconnect flag to connect context but checking it seems a bit non-obvious thing for me.

I don't have strong opinion here, maybe you can describe an API you want with some examples and use cases? With understanding that events like connect, disconnect, subscribeSuccess, unsubscribe can happen many times throughout client lifetime. For me event model looks more natural to deal with but I can miss some scenarios.

@synw
Copy link
Contributor Author

synw commented Apr 12, 2020

I don't have strong opinion here, maybe you can describe an API you want with some examples and use cases?

Ok. The non awaitable connect was blocking for me when trying to implement a reconnect with my api experiments, trying to manage things manually somehow. I'll try to post more info about the use case if I dig more this

Please notice that an async function returning void is non idiomatic in Dart: there is a linting rule about this.

Btw I would recommend using more strict analysis options for this package. Pedantic is quite standard now as used by Google and for pub.dev packages evaluation (pana). I personally use and highly recommend Extra Pedantic, which is very strict. You already use implicit-dynamic: false in analysis options and it's good. Maybe you could add implicit-casts: false as well: it helps preventing bugs by being less cool about types

@FZambia
Copy link
Member

FZambia commented Apr 12, 2020

@synw thanks for suggestions. Since I maintain all Centrifuge libs and Dart is not my day to day language I hardly can find time and knowledge to refactor for idiomatic code. If you are using this library then maybe you can send pr with all your code style recommendations - will be happy to accept.

This was referenced Aug 4, 2020
vanelizarov added a commit to vanelizarov/centrifuge-dart that referenced this issue Nov 20, 2020
@vanelizarov vanelizarov mentioned this issue Nov 20, 2020
@SimonVillage
Copy link
Contributor

I was looking for an await connection / await subscription, too.

  1. initiate the web socket connection, on success
  2. call an API endpoint to fetch the current value of something.

Let's say due to the delay of connection, I call the API endpoint first. I receive value = true. Now the backend is publishing value = false but the connection gets established after this.

Now my value remains true but in the backend it already changed to false.

I know that there are some ways to solve the issue as example working with history and receiving the last messages from the channel, however, I believe that this is not optimal.

@FZambia
Copy link
Member

FZambia commented Oct 7, 2021

I tried to explain why connect and subscribe are not awaitable in #31 (comment). Again - the main concern about making connect and subscribe awaitable is that during connection lifetime onConnect and onSubscribe events can happen multiple times (due to network disconnects). Not sure what's the use case when you only need to do some onConnect work only once, but not need to do it after reconnect - i.e. when onConnect will happen second time.

Having onConnect theoretically allows to implement a Future on top of this library. But just not understand why this can be useful due to concerns above.

@PavelSmertin
Copy link

So what is a proper way to manage connection state on client? Sometimes app is in background and disconnected by server side, in this case disconnectStream listener wont handle disconnect event and a i can't use isConnected flag for check connection state. How should i call connect again?

What is proper way to subscribe channels initial? Im trying to subscribe on connect event in connectStream, but onSubscribe events happens multiple times, and each time subscribe recalls as result i have trash in subscriptions. What is proper way to resubscribe channels on reconnect?

@FZambia
Copy link
Member

FZambia commented Oct 16, 2021

@pashtetbezd hi, your question is off topic here. Try asking in community rooms. General answer - you better not use websockets in background - you should disconnect and use push notifications for important messages.

@PavelSmertin
Copy link

I wanted to say that if the function returned futures there would be no question caused streams hell. Please add the futures.

@FZambia
Copy link
Member

FZambia commented Oct 19, 2021

@pashtetbezd could you show what you mean on example? Also, how adding futures addresses concerns mentioned in #31 (comment) ?

@FZambia
Copy link
Member

FZambia commented Nov 14, 2021

Well, addressing this in #57 – since everybody asks about this, let's try and see how it goes. If anyone can test out branch from #57 – plz do and share feedback.

@FZambia
Copy link
Member

FZambia commented Nov 19, 2021

Addressed by v0.8.0

@FZambia FZambia closed this as completed Nov 19, 2021
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 a pull request may close this issue.

4 participants