Remove unused property in response object. #61
Conversation
| function setDefaultProperty(data) { | ||
| var outObj = {}; | ||
| if (data.hasOwnProperty('jsonrpc')) { | ||
| outObj.jsonrpc = data.jsonrpc; |
There was a problem hiding this comment.
I'm not sure this is appropriate. multitransport-jsonrpc still is not JSON-RPC 2.0 compliant, so we shouldn't pretend it is.
There was a problem hiding this comment.
My pull requests will support JSON-RPC 2.0. And this PR should be last change.
There was a problem hiding this comment.
Oh really? I'll admit I only did a cursory search, but is there a good integration test suite for JSON-RPC 2.0 that we could run multitransport-jsonrpc through? (If not, I think I'll start working on such a project.)
And thank you for getting this past the last mile on 2.0 compatibility. :) It's unfortunately not been a very high priority internally, since the current code has worked just fine.
|
Sorry for just getting to this. You opened the diff right at the beginning of a major US holiday (Thanksgiving). ;) A few minor comments. I feel this is definitely the right direction. :) |
| if(error) { | ||
| outObj.result = null; | ||
| outObj.error = errorObject.internalError; |
There was a problem hiding this comment.
Because of this change here, the else branch here can never be hit. Can you remove that from the codebase?
|
I'm trying to figure out how it's possible, but somehow the test that hits this codepath is no longer working, but it's tests are passing. We can't merge this until one of us figures out why this regression has occurred. |
This reverts commit 2ac1355.
|
@horiuchi I just loaded the latest master and ran the unit tests and those three lines aren't hit on master, either. I have no idea why coveralls is saying they were recently lost. If you want to re-revert that "change response error format", feel free, I'll merge after your answer either way. :) |
|
@dfellis Thank you for checking on master branch. NodeJs may be changed effect? I think it's better not to change the commit. Please, merge this pull request. |
Remove unused property in response object.
|
Alright! Published as 0.8.0 :D |
|
👍 |
I made sure that only one of the
resultorerrorwill exist.via. http://www.jsonrpc.org/specification#response_object