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

Fix presence implementation according to reference one (ably-go) #1890

Closed
Tracked by #1848
maratal opened this issue Mar 15, 2024 · 2 comments · Fixed by #1891
Closed
Tracked by #1848

Fix presence implementation according to reference one (ably-go) #1890

maratal opened this issue Mar 15, 2024 · 2 comments · Fixed by #1891

Comments

@maratal
Copy link
Collaborator

maratal commented Mar 15, 2024

Looks like a lot of outdated code in the presence, needs to be updated in according to reference implementation/specs.

Related issue - #1889

┆Issue is synchronized with this Jira Task by Unito

@sacOO7
Copy link
Contributor

sacOO7 commented Mar 15, 2024

TODOs

  1. Refactor onMessage implementation with relevant newerThan checks => https://github.com/ably/ably-go/blob/dd8236fc301eb078803e83efee7615f3ba133341/ably/realtime_presence.go#L291
  2. Remove use of _syncSessionId. Currently, it's being used for marking members as updated. It causes field to be initialized in ARTPresenceMessage.m ( doesn't go as per spec ). We don't have syncSessionId in any other SDK's, seems this is there since very old spec. I would recommend to use beforeSyncMembers as a list instead.
  3. Self-review ably-cocoa presence code with respect to ably-go implementation => https://github.com/ably/ably-go/blob/main/ably/realtime_presence.go

@maratal
Copy link
Collaborator Author

maratal commented Mar 18, 2024

  1. I think that checking nil in isNewerThan is okay and lets to avoid this check earlier. I've introduced additional check on whether the member exists prior to removing it. Also I think that the code duplication here can be also avoided, so I left cocoa implementation intact here, except renaming methods to something that I think makes more sense to reduce confusion.
  2. I think that introducing additional array to track what was updated is unnecessary complication. Instead simple flag wasUpdated within a message can be used. I tried that (didn't rename the var though) and it works.
  3. WIP

WDYT @sacOO7

@maratal maratal changed the title Fix presence implementation according to reference one (ably-go) [WIP] Fix presence implementation according to reference one (ably-go) Mar 18, 2024
@maratal maratal changed the title [WIP] Fix presence implementation according to reference one (ably-go) Fix presence implementation according to reference one (ably-go) Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants