Support presence message extras

[lmars]

As per RTP8a and TP3i, presence messages support the extras field, but there is currently no way to set presence extras in the API (even though the IDL indicates the enter function signature as enter(Data?, extras?: JsonObject) => io).

Rather than making a breaking change to the enter function, this PR introduces an enterMessage function which supports passing a PresenceMessage, similar to how publish supports passing a Message, for example:

channel.presence.enterMessage(PresenceMessage.fromValues({
  extras: { headers: { key: 'value' } },
}));

This functionality is required for an upcoming release of the Spaces SDK.

MMB-212

[paddybyers]

It's unfortunate to have the asymmetry of having publish(Message) but no enter(PresenceMessage), but I don't see an easy way around that whilst remaining compatible.

[owenpearson]

According to the features spec, extras should be the second argument to enter, leave, update, similarly it's supposed to be the final argument to enterClient et al, is there a reason for breaking from the spec here?

[lmars]

extras should be the second argument to enter, leave, update, similarly it's supposed to be the final argument to enterClient et al, is there a reason for breaking from the spec here?

A few reasons:

1. I don't think the spec should be so prescriptive about function signatures, I think it's enough to say "there should be a function which supports entering a channel with data and/or extras" and leave it up to each SDK to implement that in whatever way seems reasonable (this was discussed internally in ADR57), so coming from working on ably-go and ably-rust, I personally ignore the IDL in the spec
2. I think the API should be closer to publish which either takes name+data or a Message, and I also don't like the function overloading pattern in general
3. I wanted to avoid a breaking change, unless you think this can be supported in a non-breaking way?

[owenpearson]

@lmars fair enough, i don't disagree with (1) and (2). As for supporting this without a breaking change we could support both signatures for enter using instanceof?

enter(data: any): Promise<void>;
enter(presenceMessage: PresenceMessage): Promise<void>;

enter(dataOrPresenceMessage: any): Promise<void> {
  if (dataOrPresenceMessage instanceof PresenceMessage) {
    // normalise arguments
  }
  // ...
}

i guess the only way this would be breaking would be if someone is already using a PresenceMessage instance as presence data but i think we can safely assume no one would actually do that.

[owenpearson]

also - any changes/additions to the public api need to be added to ably.d.ts, so with the changes in their current state we would need to add the enterMessage signature.

[lmars]

@owenpearson ah thanks, I wasn't aware you could do x instanceof y, updated to use that approach in 76624d4, meaning this now supports:

presence.enter(PresenceMessage.fromValues({
  data,
  extras: { headers: { key: 'value' } },
}));

Given that data has type any in ably.d.ts, I assume nothing needs to change there?

[owenpearson]

Given that data has type any in ably.d.ts, I assume nothing needs to change there?

Yeah I guess this means we can't add a new signature, would be good to update the docstrings to mention this though

[lmars]

@owenpearson

would be good to update the docstrings to mention this though

Done in 5baf94d.

Comments addressed, PTAL.

[owenpearson]

LGTM, thanks!

[SimonWoolf]

We should not be exposing things like a stringifyAction param in the public api. There is no reason a user of the lib should have to know or care that we use numerical encoding in our internal wire protocol, and this doc string is meaningless to such a user.

I'm not a fan of having the one method to do conceptually quite different actions controlled by that param anyway, so in #1923 I've split it into two different methods, fromValues(values: Properties<PresenceMessage>): Message suitable for public use, and fromWireProtocol(values: WireProtocolPresenceMessage): PresenceMessage as an internal method.

[lmars]

Ok, I didn't introduce the argument, I just needed the function to be included here to be able to call it from Spaces, I paid very little attention to whether or not the argument is suitable, what you suggest sounds good to me 👍