Bug with single param `Subscription.listAsync` call.

[izakfilmalter]

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:

1. Fix this one function with the above solution.
2. 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.

3. 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: https://github.com/Bandwidth/node-numbers/blob/8a765bd6bedad03d9bc5b892075a6f5300f7ab66/lib/subscription.js#L27

[izakfilmalter]

Bump, this is still an issue....

[izakfilmalter]

This is still a bug...... any response?

[jmulford-bw]

Hi @izakfilmalter,

We have plans to rewrite this package entirely, so we will not be moving forward on this issue. If you run into a blocker feel free to reach out to our support team.

Thanks!

[izakfilmalter]

@jmulford-bw Is this gonna get resolved? It's been over a year and no movement. The promised rewrite of everything that solves every bug is always a lie. Do you want me to open a pr that fixes the issue?

It shouldn't be that hard to add in the monkey patch solution.