Skip to content
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

Handler.js can return a promise #94

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

padiazg
Copy link
Contributor

@padiazg padiazg commented Oct 6, 2018

Signed-off-by: Patricio Díaz padiazg@gmail.com

Index.js can handle is handler.js returns either a final value or a promise

Description

Added code to index.js tu verify if the value returned by handler is a promise, if so first resolves it to get the final value to return.

Motivation and Context

Many users ask for using promises or async/await to handle asynchrony.

  • I have raised an issue to propose this change (required)

Which issue(s) this PR fixes

Fixes #92

How Has This Been Tested?

For normal behavior, handler returning final value:

While in the templates folder created a function with

$ faas-cli new promise-test --lang node

Deployed with

$ faas-cli up -f promise-test.yml

Checked that it works using Portal

For cheking when handler returns a promise:

At handler.js, commented the actual module.exports and uncommented the sugested one that returns a promise.

Deployed again and test with Portal

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Signed-off-by: Patricio Díaz <padiazg@gmail.com>
Signed-off-by: Patricio Diaz <padiazg@gmail.com>
@derek derek bot added the new-contributor label Oct 6, 2018
} // cb ...

const result = handler(val, cb);
if (result instanceof Promise) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're missing a branch where the result is not a Promise. I think that will break backwards compatibility where someone updates their existing function to the new template or they write a new function the old way expecting it to still work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. The portion of code responsible of returning the result of the function is now packet in cb, which is passed to handler. If you are running a "legacy" function cb will be called, the result sent to the GW and all ends there.
But if in turn you are running a function that returns a Promise, it will be "unresolved" in index.js and that's why we check if it's a Promise, and resolve it to return a result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

handler.js can return a Promise
2 participants