core icon indicating copy to clipboard operation
core copied to clipboard

Inconsistent behavior between callback and promise styles in `json-rpc-engine`

Open mcmire opened this issue 3 years ago • 0 comments

You can use engine.handle two ways:

  • If you pass a callback, then the callback will be called with two arguments: error and response. Assuming that the request is valid, the request will be passed through the middleware stack. If an error is thrown anywhere along the way, the callback will be called with the error as the first argument; otherwise it will be called with the response as the second argument. An exception is made for notifications.

  • If you don't pass a callback, then engine.handle returns a promise. Again, the request is passed through the middleware stack. If an error is thrown along the way, then it will be added to the response object. Regardless, the promise will resolve with the response object. (Again, an exception is made for notifications.)

Said another way, when an error occurs, in the callback style, error will be populated, even if res.error will also be populated; but in the promise style, the promise will always resolve (and res.error will be populated). This is where the inconsistency lies. Why is this undesirable? In our various projects, we very frequently make use of pify or util.promisify to convert a function that takes an errback to promise style. However, if you take engine.handle and promisify it, it will have different behavior than the promise form of engine.handle. To me, that's a bit surprising.

To address this, it seems like it might be beneficial to go one of two routes:

  • Change the callback form so that we set error to undefined after we set it on res.error
  • Change the promise form so that we reject when we have an error rather than resolving.

mcmire avatar Sep 26 '22 17:09 mcmire