tagomoris / presto-client-node

Distributed query engine Presto client library for node.js
MIT License
126 stars 57 forks source link

TypeError if missing error callback when using success callback #71

Closed MasterOdin closed 12 months ago

MasterOdin commented 1 year ago

Right now, the code only requires that either success or callback be defined:

https://github.com/tagomoris/presto-client-node/blob/42a7ca05220a8b6476c68dbecb1a510ed1be5139/lib/presto-client/index.js#L197-L198

However, if I define success and an error happens I get:

/foo/node_modules/presto-client/lib/presto-client/index.js:340
                    error_callback(error || response.error);
                    ^

TypeError: error_callback is not a function

The current documentation doesn't really make this clear that'll happen (to me), especially combined with #70 where success is documented to have an error as part of its callback signature.

Would it make sense to have error also be required if success callback is used and callback is not defined? Or perhaps just provide a no-op function when defining error_callback, e.g. error_callback = opts.error || opts.callback || () => {};? Or just update README documentation?

tagomoris commented 1 year ago

I understood that it's misleading, from both viewpoints of documents and callback arguments. My original intention was, users should specify callback, and users can specify additional callbacks (success and error) for specific cases.

But specifying both success and error (without callback) makes sense to me (and users, probably). So, it looks good to me to add an argument check about the existence of opts.error and opts.callback, just like success, and update README.