sourcegraph / jsonrpc2

Package jsonrpc2 provides a client and server implementation of JSON-RPC 2.0 (http://www.jsonrpc.org/specification)
MIT License
195 stars 62 forks source link

Add message serializer #38

Closed xsadegh closed 4 years ago

xsadegh commented 4 years ago

Problem summary

Recently I tried to use Binance API. The problem was that anyMessage could not unmarshal the payload and I came across this error: jsonrpc2: unable to determine message type (request or response) Well, anyMessage unmarshaller worked fine but the problem was that there was no method in the payload!

An example of payload:

{
  "e": "kline",
  "E": 123456789,
  "s": "BNBBTC",
  "k": {
    "t": 123400000,
    "T": 123460000,
    "s": "BNBBTC",
    ...
  }
}

Solution

We need a serializer func that call before unmarshal anyMessage data. I tried to put this serializer in anyMessage struct but it was not possible :d That's why I defined a global serializer and the user can override it.

keegancsmith commented 4 years ago

Hi thank you for the contribution.

Well, anyMessage unmarshaller worked fine but the problem was that there was no method in the payload!

This sounds like it isn't jsonrpc2. Given this package is for jsonrpc2 I'm not sure if this is an appropriate addition. Is there maybe some more context you can give to show how this is useful for jsonrpc2 applications?

xsadegh commented 4 years ago

Hi thank you for the contribution.

Well, anyMessage unmarshaller worked fine but the problem was that there was no method in the payload!

This sounds like it isn't jsonrpc2. Given this package is for jsonrpc2 I'm not sure if this is an appropriate addition. Is there maybe some more context you can give to show how this is useful for jsonrpc2 applications?

Yup, this response isn't jsonrpc2. So, I'm going to use my own fork. And you're right 😄 It's an inappropriate addition for merge.