Bug with single param Subscription.listAsync call. #20
Description
When using Subscription.listAsync and using Client.globalOptions, you only pass in one Param into the function, the query. Because this lib uses multiple params instead of a single param as an object, there is a chance for missing param bugs. In this case the query param is undefined, the callback param is set to the query though technically this is the client param.
Subscription.list = function(client, query, callback){
if(arguments.length === 1){
callback = client;
client = new Client();
}
else if(arguments.length === 2){
callback = query;
query = null;
}
...
}I made a simple test to check what is going on:
function test(client: string, query?: string | null, callback?: string | null) {
if (arguments.length === 1) {
callback = client
client = 'new client'
} else if (arguments.length === 2) {
callback = query
query = null
}
console.log(client, query, callback)
}
test('first param')
> 'new client' undefined 'first param'This then causes the following type error:
Unhandled error TypeError: client.concatAccountPath is not a function
Other places in the lib use the following, and if you monkey patch the function with this, it works:
if(arguments.length === 2){
callback = query;
query = client;
client = new Client();
}I see three possible solutions ranked from easiest to hardest:
-
Fix this one function with the above solution.
-
Write a helper function that manages the params, use that with all the functions. Key here is that you won't have this same problem in the future.
-
Use an object param model:
Subscription.list = function({client, query, callback}) { ... }
The beauty of single param functions that take an object is you don't have to worry about param order or even absence. The second awesome thing about it is that any future changes to methods in this lib become really easy, want to add another param and not have it break peoples code? Done. Here is an example that resolves the biggest issue I have with the lib:
Subscription.list = function({client = new Client(), query, callback}) { ... }
Now you don't have mutation on the params, and you have a true optional param with a safe default.
Would love to know thoughts on the three solution options, and then don't mind opening a PR to either of them. If there is no push back, I'm more than happy to convert the methods over to object params with a default value for client. Probably can also make a codemod to help auto upgrade everyone's code.
Link to file:
node-numbers/lib/subscription.js
Line 27 in 8a765bd