braverhealth / phoenix-socket-dart

Cross-platform and stream-based implementation of Phoenix Sockets
https://pub.dev/packages/phoenix_socket
BSD 3-Clause "New" or "Revised" License
74 stars 37 forks source link

Basic functional Presence #11

Closed ol88 closed 3 years ago

ol88 commented 3 years ago

Disclaimer: I'm not an experienced dart/flutter dev or dev in general. So I'm sure this PR needs a fair bit of work from more experienced devs.

This PR enables basic Phoenix Presence functionalities as per the Phoenix Presence tutorial: [https://hexdocs.pm/phoenix/presence.html]

What's new:

What works:

Problems:

I don't believe I'll have a lot of time to work more on this but hopefully this can serve at a base for other to improve. Feel free to edit/improve at will.

matehat commented 3 years ago

@ol88 thanks for your contribution! Have you tested this extensively?

ol88 commented 3 years ago

No that's one of the problems:

Problems:

  • No tests at the moment.
  • Outside of basic connect/disconnect/reconnect use case, this has not been extensively tested. My understanding of the Phoenix protocol is still quite limited.

I don't have the expertise nor time for extensive testing at this stage.

Ideally someone else can assist with testing.

matehat commented 3 years ago

@ol88 understood, I'll give it some time if I find some in the next few weeks, if no one else picks it up. Thanks!

hworld commented 3 years ago

Just throwing my hat in the ring to test this as well since we need presence in our app. So far this code is working great for us except for the metadata. We don't have updatedAt in our codebase being returned, so it was breaking. I switched the AppPhoenixPresenceMeta to instead mostly be a simple wrapper around Map<String, dynamic> with a known phxRef since I believe that's the only field required. Here's my version:

class AppPhoenixPresenceMeta {
  AppPhoenixPresenceMeta.fromJson(Map<String, dynamic> meta)
      : data = {...meta},
        phxRef = meta['phx_ref'];

  /// The raw data associated with the meta.
  final Map<String, dynamic> data;

  /// The [AppPresence] event reference on the backend Phoenix server.
  final String phxRef;

  /// Clones a [AppPhoenixPresenceMeta] object
  AppPhoenixPresenceMeta clone() {
    final json = _toJson();
    return AppPhoenixPresenceMeta.fromJson(json);
  }

  Map<String, dynamic> _toJson() => data;
}

I figure people can cast the map into whatever data structure they prefer in their application code and that the library should keep things pretty dynamic since it doesn't need access to any of it.

I'm still pretty early in my testing of presence though, so I can hopefully give an update in the coming days with more information.

matehat commented 3 years ago

@hworld please feel free to build on @ol88's work and make the PR more mature! I don't have much bandwidth to tackle Presence support, as we don't use Presence at our company for now, but I'll be more than happy to review and accept PRs to push Presence support through the door!

ol88 commented 3 years ago

I just updated this and the example to null-safety.

I have been running the null-safe code for a while on my own app without issue so thought it was time to update it here too.

I have also followed @hworld recommendation and made PhoenixPresenceMeta generic other than for phx_ref. Users can now optionally extend PhoenixPresenceMeta in their code to add any custom field they'd like without impacting the package logic. I updated the example with how to do that using the 'online_at' custom field.

@matehat even though I don't have time to write tests for this right now, I think this should be merged in master because the current implementation of Presence in master is basically broken so merging this cannot break it more. The changes are limited to the presence.dart file and a new example app so it cannot affect the rest of the package as it's all isolated changes.

If anything, once in master, more people might jump in and create pull requests to improve it if need be.

I've left the below warning untouched so people know what to expect from the functionality.

// TODO: This is a very much a non-tested port of the javascript code!
//       Feel free to test, improve and make a pull request.
matehat commented 3 years ago

Thanks @ol88 a lot for your hard work! I agree we should merge this, so long as users understand that the API may change and the functionality is not yet fully tested.