ToJson bug

[mattheworiordan]

See https://github.com/ably/ably-js/blob/master/common/lib/types/presencemessage.js#L28, shouldn't this be action?

[mattheworiordan]

In fact, it looks like a number of supported fields are missing from the toJson and toString functions

[paddybyers]

You'll see that action is already there, so the bug is that name is included.

Other fields are intentionally omitted because toJSON() is only used when sending messages, and this is the subset of fields that are sent by the client.

[paddybyers]

b1632c3

[mattheworiordan]

Thanks, but I think id is missing too and connectionId from the string representation

[paddybyers]

toString() is intended to provide a useful human-readable summary containing the most relevant fields.

[mattheworiordan]

Seems a bit arbitrary that an action enum integer value and time in milliseconds is considered useful human-readable value, but connectionId is not IMHO.

[mattheworiordan]

Matt to remove unneeded fields such as timestamp, connectionId

[SimonWoolf]

Is the action here to remove the timestamp & connectionId fields from the presencemessage object entirely?

[mattheworiordan]

Is the action here to remove the timestamp & connectionId fields from the presencemessage object entirely?

Yes, plus if clientId matches the authenticated clientId, or if clientId is empty, then we don't need to send that field.

[SimonWoolf]

I don't see how that can be right. We use the timestamp and connectionId in the presence message to decide whether incoming presence messages should supersede presenceMap items with the same clientId + connectionId: https://github.com/ably/ably-js/blob/master/common/lib/client/realtimepresence.js#L256:L261. If we're removing those two from the object entirely (not just from the fromJSON() and toString() calls), then that problem'll be unsolved

[SimonWoolf]

PR that only changes the json/string rep: #117

[paddybyers]

timestamp is server-assigned, not client-assigned, so doesn't need to be sent by the client.

connectionIdis implicit on a realtime connection, so also doesn't need to be sent by the client. Since the client never sends PresenceMessages via REST, then there is no need to include it. Messages are of course different in that respect.

[mattheworiordan]

@paddybyers and you agree that clientId is not needed unless the client is not authenticated yes?

[paddybyers]

you agree that clientId is not needed unless the client is not authenticated yes?

It's correct that if a clientId is set on the library then it doesn't need to be populated in each Message. That's a different matter however from whether or not it needs to be supported in toJSON().

[mattheworiordan]

@paddybyers well perhaps, but then the client library somewhere needs to move clientId if the user explicitly sets it. Also, from what I can see, I think we'll by default always be sending clientId with a null value which also is unnecessary?

[paddybyers]

the client library somewhere needs to move clientId if the user explicitly sets it.

it doesn't do that yet. But that's not in toJSON().

I think we'll by default always be sending clientId with a null value which also is unnecessary?

If unset it will be undefined, and therefore will not be in the encoded result.

This toJSON() would not be here if it wasn't for the need to base64-encode binary data. If an undefined member is present to begin with, it's fine for that same member to be present with undefined value in the result.

[mattheworiordan]

Ok, makes sense.