-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve Promise usage by using chaining #276
Comments
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:
Of these, the only one I can think could be changed would be the last one, and that would require using |
@jberstler thanks for having a look! To the first needed example: Couldn't every 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. |
Promises in the actions are plumbed together in a non-consistent way. Sometimes they use
then
/catch
chaining, sometimes they are constructed globally and thenresolve
/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.
The text was updated successfully, but these errors were encountered: