-
Notifications
You must be signed in to change notification settings - Fork 6
H4C-247: JSON RPC Client implementation #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
The head ref may contain hidden characters: "goce/H4C-247_\u0458son-rpc-client"
Conversation
- added JSONRPCClient class - added new service for testing the JSONRPCClient
Merge branch 'master' into goce/H4C-247_јson-rpc-client # Conflicts: # src/index.ts
src/client-proxy.ts
Outdated
| return axios | ||
| .post(url, { method: namespace + "." + prop.toString(), params, jsonrpc: "2.0" }) | ||
| .then(res => Promise.resolve(res)); | ||
| .then(res => Promise.resolve(res)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bit is redundant
src/client-proxy.ts
Outdated
| method: namespace + "." + prop.toString(), | ||
| params, | ||
| jsonrpc: "2.0", | ||
| id: ++id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will only increment the counter locally. this.counter will not be affected. We need to increment the counter itself
src/index.spec.ts
Outdated
| } | ||
| } | ||
| }; | ||
| return service.testError({ data: "hi" }).then(res => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client should automatically unwrap the content, recreating and rethrowing the error. You should not be aware of any json-rpc implementation detail here.
src/client-proxy.ts
Outdated
| const { code, message, data } = err.response.data; | ||
| let resp = { code, message, data }; | ||
|
|
||
| return { jsonrpc, error: resp, id }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should recreate the CodedRpcException and throw it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @alantreadway-hfour whether we want to deal with different errors in a different way here.
One option is to just rethrow any error type. The other is to differentiate between client failures and server failures.
I personally think rethrowing is just fine for the basic client, then other clients can be built on top of it that catch errors and decide what to do with them based on the error code and error message (maybe retry if idempotent request and if the code indicates a retry might work out - otherwise rethrow)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's start simple, we can add more advanced (retry) behaviours later? So rethrow seems fine here to me.
- throw error in rpc client
src/index.spec.ts
Outdated
|
|
||
| it(`should throw an error on /rpc/v1/ test.testError (POST)`, () => { | ||
| it(`should return an error and check error data from JSONRPCClient call`, async () => { | ||
| const errorObj = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now an unused variable?
src/index.spec.ts
Outdated
| .expect(403) | ||
| .expect(errorObj); | ||
| const resp = service.testError({ data: "hi" }); | ||
| return expect(resp).rejects.toThrowError(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to specify that the error should have a certain code/error/message here?
JSON RPC Client implementation