allow ids as string types #17
Conversation
| * @param {Object} request | ||
| * @return {Boolean} | ||
| */ | ||
| hasId : function (request){ | ||
| return request && typeof request['id'] !== 'undefined' && /^\-?\d+$/.test(request['id']); | ||
| return request && typeof request['id'] !== 'undefined' && |
There was a problem hiding this comment.
it's missing checking against null (the typeof !== 'undefined' includes null, and all other types), so I think in the end it would be
return request && typeof request['id'] !== 'undefined' && (typeof(request['id']) === 'number' && /^\-?\d+$/.test(request['id']) || typeof(request['id']) === 'string' || request['id'] === null);Please notice that your typeof in string check is wrong. It's checking for the boolean if typeof(request['id'] === 'string') which will be always false, and it will always return truthy
There was a problem hiding this comment.
good catch on the string. fixed it.
|
Although the current implementation does permit strings, numbers and nulls, this change might be a breaking change, since people usually don't read the specifications before setting their projects / calls up. the in and out of JSON payload from different languages might return numbers for IDs that look like string but aren't, like octal and scientific notation strings, like |
|
as to your point of a breaking change, if they give input as a string, it will still return true (plus that's not valid spec). if we believe it is a large enough breaking concern, then we should rev the version accordingly. |
|
ping on this - are we going to fix it? |
|
merged and bumped to 1.0, there was another PR that was doing the same thing in a different part of the code, so merged both and updated the dependencies |
according to the json-rpc specification at http://www.jsonrpc.org/specification:
id
An identifier established by the Client that MUST contain a String, Number, or NULL value if included. If it is not included it is assumed to be a notification. The value SHOULD normally not be Null [1] and Numbers SHOULD NOT contain fractional parts [2]
this fix makes it possible to use a string as an id