search

uber-archive / multitransport-jsonrpc

JSON-RPC Client (Node.js & Browser) and Server (Node.js) aim at "natural looking" server and client code.
116 stars 22 forks source link

Doesn't appear to support JSON-RPC notifications? #77

Open gschmottlach-xse opened 7 years ago

gschmottlach-xse commented 7 years ago

Perhaps I'm missing something, but this module doesn't appear to support JSON-RPC notifications. I can see no way to instruct the client to set the "id" field to null. Are notifications supported by this module?

Thanks . . .

dfellis commented 7 years ago

Hi @gschmottlach-xse,

I don't think notifications were part of the spec when I started this project, and in the intervening years Uber has dropped JSON-RPC for TChannel, so there hasn't been much activity on this repository since.

It doesn't seem like it would be too difficult to add notification support to the library. Do you have a proposed API style you'd like to support them? The specification implies that this could be for any RPC call, but doing it on a read operation would be nonsensical, should the server constrain the client on which methods can be notifications, and should they be wholly separate or optional?

gschmottlach-xse commented 7 years ago

I don't have a strong opinion on how notifications should be added to this module. The Jayson implementation has a client supplied id parameter with their client.request() method that allows you to indicate whether the call is a request or notification. Basically, if id is undefined, it's a request but the request will generate an identifier for you. If it is explicitly 'null' then it is assumed to be a notification. On the server-side whether it's a request or notification does not matter. A server callback can return a value but if the original request does not have an id then nothing is returned. So it's transparent to a server implementation but would require you to implement some changes in the response handling.

Since the existing API does not have an id parameter with each requests, it might just be better to add an explicit notify method to the client. Internally it might call a private version of request (call it _request) which has a similar request signature as the similar Jayson client.request() method.

That's my proposal, perhaps you have something else in mind? I like this module due to it's performance improvements (e.g. persistent connections) over what Jayson currently offers. But the lack of notifications was a blocker for my application. Add this support and it might tempt me to switch to get the performance gains.

Thanks for your interest in addressing this short-coming of the current module. It would be nice if it were JSON-RPC V2 compliant.