Skip to content

json rpc 2.0 named parameters support + error code support #1

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kotoMJ
Copy link

@kotoMJ kotoMJ commented Nov 21, 2013

Hi Gimmy,

I am using your jsonrpcjs, it is great and simple. They are two things, they are missing in this code.

  1. absence of support of named parameters, in your version they are passed using order
  2. possibility to work with the code of json rpc error.

I am succesfully using your jsonrpcjs extended with above 2 mentioned features.
I created fork and I would like to ask you to merge these fork to your trunk in the next version of jsonrpcjs, if possible. I guess it could be helpful for many users.

Kind regads,
Michal

@gimmi
Copy link
Owner

gimmi commented Nov 21, 2013

Hi misak, did just a quick check on the pull request, but seems that you are removing support for positional parameters to add named parameter support. Take this for example:

var param1 = { value1: 'foo', value2: 'bar' };
var param2 = { value2: 'baz', value3: 'toz' };
rpc.call('aMethod', param1, param2, function (result) { });

The server is expected to receive 2 parameters of object type, but instead receives 3 named parameters of type string. This is not what is expected IMO.

@kotoMJ
Copy link
Author

kotoMJ commented Nov 22, 2013

Hi Gimmy,

you are right. I changed the way of passing parameters:

rpc.call("aMethod", {
param1:{value1: 'foo', value2: 'bar'},
param2:{value2: 'baz', value3: 'toz'} },
function (result) {});

I agree, I forced all parameters to be named and I removed support of positional parameters.
Maybe it is hard solution for current users.

Despite of this, I believe named parameters (as significant power of json rpc 2.0) are currently missing in jsonrpcjs and it could be stop for many potential users. Positional parameters are sometimes strange trap for real application (especially for future maintenance and small enhancement). I am personally using mentioned fork on quite big project and it would be not possible to use positional parameters at all.

I appreciate your quick response. If above described way is not in compatible with your vision of jsonrpcjs, consider please at least support of named parameters. Since there will be no need to create forks ;-)

Thanks for base of jsonrpcjs.

Misak

@gimmi
Copy link
Owner

gimmi commented Nov 26, 2013

Well, the current jsonrpcjs API assumes positional parameters. This is OK for most implementations because almost every server-side programming language is able to naturally bind parameters by position, but not everyone has a natural way to match parameters by name. Think about Java: parameters name are lost during the compilation process, and so a JSON-RPC java implementaiton need to use some magic feature like annotation to support named parameters. This is just to explain the rationale behind my deliberate decision to use support primarily positional parameters, but I also don't consider this a fair argument for not supporting positional named at all.

Back to implementaiton detail: the current API assume positional parameters, and i don't think there's a way to know if the call uses positional or named parameter, so i think the best option would be to have 2 new methods:

rpc.ncall('aMethod', { a: 1, b: 2 }, function (result) { }); // ncall stand for 'named call'
rpc.pcall('aMethod', 1, 2, function (result) { }); // pcall stand for 'positional call'

now the question is: what to do with call methon? For backward compatibility the best would be to treat it as an alias for pcall, but i think a better API would be to not have it at all or consider it a raw call that just send the parameter passed, so:

rpc.call('aMethod', { a: 1, b: 2 }, function (result) { });
rpc.call('aMethod', [1, 2], function (result) { });

@kotoMJ
Copy link
Author

kotoMJ commented Dec 2, 2013

I am glad to see variant NCALL and PCALL. Actually in case of new version, I would prefer to somehow depricate the CALL method (to be removed for another version). Depricated call method should do the same, like PCALL, as you wrote.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants