Refactor: use explicit preserialization methods rather than overriding toJSON()

[mattheworiordan]

trafficstars

See https://jsbin.ably.io/ebemoj/4/edit

Why are the members returned in the get method missing all the attributes of Presence member such as connectionId. This is sorely needed for anyone trying to work out if they are present or not upon reconnection.

┆Issue is synchronized with this Jira Task by Unito

[SimonWoolf]

@mattheworiordan they're not, they're just hidden from the default toJSON(), see See https://jsbin.ably.io/ebemoj/6/ that uses toString() instead. See past discussion at https://github.com/ably/ably-js/issues/44

[SimonWoolf]

(FWIW I wouldn't be against making internal library use use a custom method rather than toJSON, so end-users' JSON.stringify() calls aren't affected, but Paddy prefers this way)

[paddybyers]

The only reason for having a toJSON() is because that's how you control what gets packed when sending on a connection (both with JSON and msgpack transports). Since we have custom handling for data (ie base64-encoding) then we need to do this. I suspect the current toJSON() methods omit those fields because they're not required to be sent but I'm fine with adding them.

[SimonWoolf]

that's how you control what gets packed when sending on a connection (both with JSON and msgpack transports). Since we have custom handling for data (ie base64-encoding) then we need to do this.

Do we really, though? Why can't we just do it explicitly, rather than overriding toJSON? Surely what fields we need to send to Ably shouldn't have to be linked to what the users sees with a stringify?

Code speaks louder than words, so quick-and-dirty suggestion: https://github.com/ably/ably-js/compare/prepareforsending?expand=1

(I know you said you're fine with re-adding them, but a year ago I was asked to remove them, and don't really want to get into an infinite loop :smile: )

[paddybyers]

Of course the reason for using the toJSON() hack was to avoid having to write code not only for the types in question but also each container type that might embed them.

[mattheworiordan]

I like your change @SimonWoolf, overload toJSON is a bit hacky, it's the kind of thing I'd expect in my code not that of @paddybyers :)

BTW. I don't like the name prepareForSending given it's not preparing but instead bundling and generating a new payload, so perhaps packageForSending or toAblyPayload() etc?

[SimonWoolf]

TBH I don't feel very strongly about this; if Paddy believes there's value in the toJSON way, I'm ok with that. I'll revert https://github.com/ably/ably-js/pull/117/commits/c7081323613c2242c64c2024dccdbfbbf4dcbbe4

[mattheworiordan]

@paddybyers WDYT?

[paddybyers]

I agree toJSON(), whilst theoretically the most efficient, comes with some baggage and constraints, and it only just works because of the hack to detect whether it was invoked via JSON.stringify() or msgpack.

So I'm not opposed to changing it.

As for naming, in realtime for the same functionality we have pack() and unpack(), which would be fine, or serialise()/deserialise(), transportEncode() etc.

[mattheworiordan]

Is this covered in 0.9 now @SimonWoolf?

[SimonWoolf]

Nope, as of now it's still using the toJSON hack. (There was some issue with the prepareforsending approach, can't remember what now, that made it not mergeable as-is, then other things took priority). Would still be nice to refactor that at some point, so leaving this issue open. Don't think it's high priority though

[mattheworiordan]

Don't think it's high priority though

Ok, thanks for clarifying