Presence docs to discourage building of presence sets

[mattheworiordan]

trafficstars

A partner of ours has been building an app which has erratic presence behaviour. Upon inspecting the code, we see the same pattern of developers continuing to try and build their own presence sets based on enter, leave, update events, which is flawed and should never be done, they should instead rely on the presence get method to get the complete presence set. We suffered this same issue ourselves with our Spaces SDK development. If our own internal staff are making these mistakes, and partners are making these mistakes, customers will certainly be making these mistakes.

Here is the sample code for ref:

We really need to rethink our presence docs to encourage the right behaviours. When I read https://ably.com/docs/presence-occupancy/presence, I see no mention of why users should not handle enter, leave, update events and build their own presence set, and in fact the get method to get the presence set only appears a couple of (virtual) pages down the page.

┆Issue is synchronized with this Jira Task by Unito

[mattheworiordan]

@paddybyers @SimonWoolf @owenpearson @ttypic I was just looking at that presence code again, and it got me thinking about whether this is a documentation issue, or in fact an SDK / API issue.

⚠️ Note I got a lot of this wrong. The SDKs do in fact emit enter, leave and update events correctly, as defined in https://sdk.ably.com/builds/ably/specification/main/features/#RTP2g, so a lot of below is wrong ⚠️

~Right now, SDKs emit the raw protocol message presence events for enter, leave, update (and sync), which is where the problem lies. There is not really any good reason for us to emit these raw events from a consumer of the API's perspective. Our raw protocol messages are not all that useful to customers given they need to understand that messages can arrive out of order and they need to understand how to apply CRDT-like logic to get the intended result set~

If we focus on what the customer would want instead of how this works internally, I would change things as following:

• ~We only ever emit enter, leave, and update events in a way that makes sense to the consuming user i.e. if they wrote code like above, it would work~
• We include an argument in the callback with a reference to the complete presence set anyway to encourage developers to use that when they need the entire presence set as opposed to knowing what has changed

WDYT?

[sacOO7]

This seems to be a react example. where they want to update the view when the dataset changes. More specifically, they want to get updated data set whenever any change to the dataset happens. I think get only returns a snapshot of members at a particular moment. That said, laravel-echo devs exposed minimum required methods for presence.

https://github.com/laravel/echo/blob/master/src/channel/presence-channel.ts#L10

In the Echo.join().here(members => {}) implementation, the callback is called with updated members when a new member joins, updates, or leaves the channel.

To expose this we had to update the code in ably laravel echo fork => https://github.com/ably-forks/laravel-echo/blob/cee1b7b51196dfab9f56bc95d5deb5880eb5411d/src/channel/ably-presence-channel.ts#L27-L35

    here(callback: Function): AblyPresenceChannel {
        this.channel.presence.subscribe(['enter', 'update', 'leave'], () =>
            this.channel.presence.get((err, members) =>
                callback(members.map(({data}) => data), err)
            )
        );
        return this;
    }

[mattheworiordan]

I think get only returns a snapshot of members at a particular moment.

Yup, why's that an issue?

To expose this we had to update the code in ably laravel echo fork =>

Yup, and it just calls presence.get which IS the recommended behaviour because trying to build your own presence set from the events is painfully difficult and error prone.

[sacOO7]

I think it will be better if we can just expose a method like here to avoid adding complex documentation in all SDK's : (

[mattheworiordan]

I think it will be better if we can just expose a method like here to avoid adding complex documentation in all SDK's : (

here => get though? I don't follow

Or do you mean an equivalent here_now callback? If so, that's what I'm loosely proposing above.. although perhaps we could go with an event / method name that is painfully simple

[sacOO7]

Almost all of the mobile/web UI frameworks update components based on state change, so we expose a method on presence channel like

channel.presence.onMembersUpdated( updatedMembers => {
})

instead of them doing

// This can get complex based on each SDK api style
        this.channel.presence.subscribe(['enter', 'update', 'leave'], () =>
            this.channel.presence.get((err, members) =>
                callback(members.map(({data}) => data), err)
            )
        );

[mattheworiordan]

FWIW. My thinking on my comment:

We include an argument in the callback with a reference to the complete presence set anyway to encourage developers to use that when they need the entire presence set as opposed to knowing what has changed

Was that we could introduce a change potentially that is not breaking as follows:

channel.presence.subscribe(function(member, allMembers) {
  console.log(member.data); // => the user who has entered, left, etc
  console.log(allMembers); // => the presence set from `presence.get`
});

[sacOO7]

Yeah, introducing any breaking/non-breaking change that can make it easier for users to subscribe to changed members 👍

[SimonWoolf]

We do document this: https://ably.com/docs/best-practice-guide#use-existing-presence-set . I know because I wrote most of that entry. The problem is that it's in the best practice guide, which is hard to discover and no-one ever reads. The solution is to move it into the normal presence docs, which is a change that was agreed some time ago.

We only ever emit enter, leave, and update events in a way that makes sense to the consuming user i.e. if they wrote code like above, it would work.

The main reason the above code is broken is that it assumes the presence set is uniquified by clientId, which it isn't, it's uniquified by clientId+connectionId. This is not a thing we can change just with a client library api change, it would require a completely different presence model, client and serverside. I don't think this is a change it would be sensible to make at this point, to put it mildly.

We include an argument in the callback with a reference to the complete presence set anyway to encourage developers to use that when they need the entire presence set as opposed to knowing what has changed

Sure, no objection to this. Trivial and backwards-compatible change for ably-js.

[mattheworiordan]

The main reason the above code is broken is that it assumes the presence set is uniquified by clientId, which it isn't, it's uniquified by clientId+connectionId

That's the main reason, but not the only reason. The former is easy to explain to a developers and they can reason about (users can be present more than once with different connection IDs), but the latter is much harder (enter events don't necessarily mean the user has entered because a leave event can arrive after but with an earlier timestamp). I think we should fix that latter problem in our SDK API because emitting the raw protocol events is not the API a developer would expect to see. We should have an abstraction over our underlying protocol instead of exposing it. That's not a big change (and requires not realtime system changes), but would materially reduce the likelihood of developers mistakenly using this API. WDYT?

[mattheworiordan]

Apologies for the noise in this issue (I made a mistake). I've corrected my first follow on comment

Saying all that, I do think we should:

• Improve the docs, as mentioned by @SimonWoolf
• We should include the presence set in the presence event callback, as suggested in https://github.com/ably/docs/issues/2073#issuecomment-1839251011

[sacOO7]

Link to internal discussion -> https://ably-real-time.slack.com/archives/CURL4U2FP/p1701711106209279

[maratal]

Why not just expose current members into class RealtimePresence as a property from PresenceMap?

[sacOO7]

They need a callback to be executed everytime it's updated. Just exposing members as a property won't notify the change : (