chr15m / bugout

Back end web app services over WebRTC.
https://chr15m.github.io/bugout
MIT License
612 stars 59 forks source link

Automatically JSON.stringify b.send #24

Closed davidaknowles closed 4 years ago

davidaknowles commented 4 years ago

This is just a suggestion. There's probably some reason this doesn't make sense but: whenever I'm using b.send to send a non-string message I want to stringify it so I end up doing e.g. b.send(JSON.stringify([message_type,[cool_data,cooler_data]])); And then obviously I end up using JSON.parse at the receiving end. Is there some reason b.send and b.on('message',... couldn't/shouldn't do this under the hood?

chr15m commented 4 years ago

@davidaknowles I wanted to make sure people could use whatever encoding they wanted - json or msgpack etc. but what you are saying also makes sense. What if there was a convenience method called sendjson()?

rfestag commented 4 years ago

I just started using this, so take my opinion with a grain of salt - but I would personally opt for configuration or some sort of middleware over growing the API for convenience functions like that. By extension, if half of people like JSON and the other half like msgpack, does the API grow to have send, sendjson, and sendmsgpack? Seems cleaner to let the user specify their preferred serialize/deserialize functions when they instantiate the Bugout instance. This would keep the API small, while still allowing people to use whatever serialization they prefer. Personally, I suspect most people will want to use JSON, so even making JSON.stringify/JSON.parse the default functions would probably be fine.

davidaknowles commented 4 years ago

Gotcha, didn't know about msgpack, I'm a js noob. I was going to say what about having the encoding as a parameter (defaulting to "JSON", if you're ok with assuming ES6) on send but then you also need to specify that for on("message",...). Maybe that's ok? Can one auto-detect JSON vs msgpack encoding?

davidaknowles commented 4 years ago

Ryan - I missed your suggestion of config. The bugout instance remembering the preferred encoding sounds v reasonable to me - hopefully people don't mix JSON and msgpack in one app?

rfestag commented 4 years ago

I doubt they would mix serializations (it would be unnecessarily complex), but even if they did they could use different Bugout instances (one for each serialization format). That, or it would be a config when defining the RPC (but at that point, you might as well just do the manual serialization/deserialization anyways...you could always write a wrapper function to act as middleware for that). At a minimum, it doesn't make sense to mix serialization formats for the same call (whether that is to send/message or rpc/register). Personally, I think doing it for the instance is sufficiently grainular.

Regarding auto-detection: While you likely could automatically detect on receive (I don't think you can craft a msgpack that is also valid json and vice versa), it would probably be unnecessarily expensive, and it wouldn't help when sending. The pair of send/recieve (whether that is rpc/register or send/message) need to use the same serialization. If you just pass a JS object/array/etc, Bugout would have no idea how you want it serialized, so it couldn't determine automatically whether to send msgpack/json/anything else. Thus, you're better off knowing ahead of time what the convention is for that send/receive pair rather than trying to detect at runtime.

chr15m commented 4 years ago

Settable encoder/decoder at instantiation time sounds good to me. Defaulting to JSON will require updating all of the examples. I don't like auto-detect at all as it is bound to frustrate people (computers are dumber than humans). What do we do with decoding errors? Same as what happens with decryption errors (silent drop)?

rfestag commented 4 years ago

Personally I never like silent anything...it would be nice to have an event emitted when something goes wrong (and I can choose whether to set up a handler for that event or not). Even if all errors were sent to the same handler (for decrypt and decode), that would be nice. For simple cases like JSON, where we can expect users to use stringify/encode, it probably isn't a big deal. However, it could be useful when somebody does something more esoteric (say, with msgpack, if they have custom types that aren't available at both ends for whatever reason), having the error would make debugging easier.

chr15m commented 4 years ago

You can debug at the moment using the debug logging facility, but emitting an error event is a great idea so that people can catch errors at runtime. Thank you.

chr15m commented 4 years ago

@davidaknowles I've just realized that data is infact already serialized and de-serialized to and from JSON format at both ends: https://github.com/chr15m/bugout/blob/master/index.js#L170

I guess you are double-serializing in your own code with that extra JSON call.

davidaknowles commented 4 years ago

Oh ha, good to know!

chr15m commented 4 years ago

Closing this. If somebody really wants messagepack or other transport encoding, we can open a new ticket and add it.