openwhisk-package-kafka icon indicating copy to clipboard operation
openwhisk-package-kafka copied to clipboard

Improve Promise usage by using chaining

Open markusthoemmes opened this issue 6 years ago • 2 comments

Promises in the actions are plumbed together in a non-consistent way. Sometimes they use then/catch chaining, sometimes they are constructed globally and then resolve/reject is used. Sometimes, both ways are even mixed together.

We should review the code and fix all usages of Promises to use chaining. That's more readable in general and gives more guarantees on the Promise being actually resolved at some point.

markusthoemmes avatar Jul 16 '18 09:07 markusthoemmes

After doing a quick review, the only place I found where this can likely be tidied up is the main Promise in the web actions, for example: https://github.com/apache/incubator-openwhisk-package-kafka/blob/457f76dbd930a573fcfb427890a6c84645c8b61a/action/kafkaFeedWeb.js#L17

It seems that all paths within this function already return a Promise, and so the manually-constructed Promise is not needed.

However, all other instances of manually constructed Promises I found appear to be necessary as they are needed:

  • to return a Promise from a non-asynchronous function for consistent usage by the caller, for example: https://github.com/apache/incubator-openwhisk-package-kafka/blob/457f76dbd930a573fcfb427890a6c84645c8b61a/action/kafkaFeedWeb.js#L150
  • to return a rejected Promise after logging during error handling, for example: https://github.com/apache/incubator-openwhisk-package-kafka/blob/457f76dbd930a573fcfb427890a6c84645c8b61a/action/lib/common.js#L38
  • to return a Promise from an asynchronous function that makes calls to callback-based API, for example: https://github.com/apache/incubator-openwhisk-package-kafka/blob/457f76dbd930a573fcfb427890a6c84645c8b61a/action/lib/Database.js#L10

Of these, the only one I can think could be changed would be the last one, and that would require using nano in a way that natively returns Promises - not sure if that is possible, but it is worth investigating.

jberstler avatar Aug 20 '18 12:08 jberstler

@jberstler thanks for having a look!

To the first needed example: Couldn't every return path return an explicit Promise.resolved/Promise.reject instead of wrapping it? Dunno if that helps readability though.

The second is indeed how I expected to find things.

The third one could use https://nodejs.org/api/util.html#util_util_promisify_original (if that's available in the node version used).

The wrapping promise particularly nasty though, I think that should vanish as you pointed out.

markusthoemmes avatar Aug 20 '18 12:08 markusthoemmes