search

centrifugal / centrifuge-dart

Dart (Flutter) client SDK for bidirectional communication with Centrifugo and Centrifuge-based server over WebSocket
https://pub.dartlang.org/packages/centrifuge
MIT License
102 stars 29 forks source link

Batching #42

Open vanelizarov opened 3 years ago

vanelizarov commented 3 years ago
vanelizarov commented 3 years ago

@FZambia

FZambia commented 3 years ago

@vanelizarov Hello!

I have some questions to this pr:

1) There was a discussion in #31 - how this pr addresses my comments (specifically this one) 2) Tests are not passing here.

Subscription batching is a good feature – but I can't start reviewing this in current pr form - it's too heavy and has non-obvious changes.

In general it's better to decouple different things to different pull requests. But before doing this let's elaborate more on my point 1 above.

vanelizarov commented 3 years ago

@FZambia about #31 and your discussion with @synw, IMHO the only reason to have awaitable connect, disconnect, etc is what a developer expects functions/methods marked as async return - Future. I can't see another reason to have it, because, as you've said, connect and some other events may occur more than once during the client's lifecycle, so the awaitable methods are not completely necessary in this case. In the context of this PR, I can revert these changes back to the original and left this rework for the future

About tests, I've started rewriting them but at some point, I got stuck and put this activity on the back burner. I can push the changes, so you can review them in the current state. It would be also great if you tell me if I am doing it the right way or not.

About decoupling, I was thinking about it during the process but all in all, figured out that it would be more correct to suggest this feature as one PR because all its parts are tightly connected to each other. As I mentioned above, mostly it repeats the logic from the web library but adjusted for Dart language specificity, and the structure of this library. Also, I can provide some explanations if needed

SimonVillage commented 2 years ago

Is this still considered to be merged at some point?

FZambia commented 2 years ago

@SimonHausdorf hello, this pr was too heavy and contained some non-obvious changes, so I have not looked closely. If you are interested in having batching (are you? or you need sth else from this pr?) then better to start from discussing a use case and make a minimal improvement that will help your use case.