Remove error from callbacks that don't return an error

[MasterOdin]

Looking at the source code, I noticed that for the execute function the following callbacks:

• state
• columns
• data

all document that their first argument is error. However, in all cases, they only ever receive null, and that if there was an error, it would have gone to the error/callback callback function, and then the above callbacks are never called.

My proposal would be to remove the first argument from those callbacks as it simplifies the call signature as well as better indicates to downstream users that they don't need to worry about any sort of error handling along those callbacks.

[tagomoris]

success callback also receives null always at the 1st argument.
It's a good idea to remove the unused callback argument error, but my request to the idea is to make it optional, instead of just removing the error, for the compatibility of users' code.

[MasterOdin]

My only thought to make it optional would be a flag to the Client constructor to use or not use this behavior. I'm not sure there's a programmatic way to detect if an end user expects the error argument or not in their callback.

Would you be fine with the flag being opt out, so that new users don't need to worry about the null argument and existing users just need to toggle the flag on when upgrading, or for the flag to be opt in? My preference would be on making it opt out since:

1. Ideally it'd be a flag could remove at some point in the future
2. It makes it easier to make the TS types

For (2), if the behavior was opt in, then I'm not sure there's a way in TS to properly model this behavior that it renders it a lot less useful to me personally that I'd probably not bother with a PR to do this, and would then just leave it at the current status quo.

[tagomoris]

callback.length returns the number of arguments. So the callback caller can handle the difference of callback function arguments.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/length

[MasterOdin]

This would only work in the case where a callback only has two arguments (just columns) or that they always defined all arguments, which is almost certainly not true. JS does not care if you pass more arguments to a function than it allows for, e.g.:

const callback = () => {};
callback(1, 2, 3, 4, 5);

is perfectly valid JS.

As a further example, my current usage of the library looks like this:

client.execute({
    query: statement,
    state: (_, query_id) => {
        // save query_id for later use
    },
    columns: (_, columns) => {
        // save columns for later use
    },
    data: (_, data) => {
        // buffer data until we hit success
    },
    error: (error) => {
        // raise an error
    },
    success: () => {
        // process our buffered data, return to caller
    }
});

So in this case, the callback data.length === 2, and it's not possible to tell in a user transparent way that I want the library to provide me the old style of callback ((error, data)) or the new style ((data, columns)).

[tagomoris]

Oh, wow, I missed the case to have fewer arguments than documents intentionally.
If we have a flag to skip error argument, it must be opt-in (its default value must be false) at first to not break compatibility of existing users' code.
My other idea is to have a set of callbacks with different names for fewer arguments.