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

feat: add option of specifying resolveMode #173

Merged
merged 3 commits into from
Aug 16, 2018
Merged

Conversation

brettstack
Copy link
Collaborator

@brettstack brettstack commented Aug 14, 2018

Allows resolving using Promise, callback, or the default context.succeed.

API decisions were made to support backwards compatibility. We will clean up the interface in a future major version bump

exports.handler = async (event, context) => awsServerlessExpress.proxy(server, event, context, 'PROMISE').promise

Please ensure all pull requests are made against the develop branch.

Issue #134, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Allows resolving using Promise, callback, or the default context.succeed.

API decisions were made to support backwards compatibility. We will clean up the interface in a future major version bump
@brettstack brettstack changed the base branch from master to develop August 14, 2018 17:31
Copy link

@keetonian keetonian left a comment

Choose a reason for hiding this comment

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

Would it be possible to add in a test with an async handler like was given in #134 ? It seems like that was the reason behind this code change; it would be nice to see a test that explicitly checks against that use case. Let me know if you already do test this- I can see some references to promise in your tests which might cover it; I don't know enough javascript yet to be able to tell.

@brettstack
Copy link
Collaborator Author

brettstack commented Aug 16, 2018

Covered here https://github.com/awslabs/aws-serverless-express/pull/173/files#diff-971abd8c6c8db00bc212414ea19e1807R183. Async is syntactic sugar for Promise. Can't use async in the test as it will fail in our test runs for earlier versions of Node.js

@keetonian
Copy link

Ok, thank you for the clarification!

@brettstack brettstack merged commit 582b88d into develop Aug 16, 2018
@brettstack brettstack deleted the promise-resolution branch August 16, 2018 17:31
brettstack pushed a commit that referenced this pull request Aug 16, 2018
<a name="3.3.0"></a>
# [3.3.0](v3.2.0...v3.3.0) (2018-08-16)

### Bug Fixes

* run npm audit fix ([85cbae6](85cbae6))

### Features

* add option of specifying resolveMode ([#173](#173)) ([582b88d](582b88d))
@zoellner
Copy link

Looks like this feature is missing from the documentation.

@adrai
Copy link

adrai commented Feb 23, 2019

@brettstack what is the current suggested way to call the proxy function? like in the readme (no callback, only context)? or with callback and how?

@brettstack
Copy link
Collaborator Author

Open to PRs updating the docs. I haven't performed a performance benchmark on context.succeed vs Promise resolution which is the main reason I didn't change the example

zoellner added a commit to zoellner/aws-serverless-express that referenced this pull request Feb 23, 2019
@zoellner
Copy link

Added a PR for the docs: #212

brettstack pushed a commit that referenced this pull request Apr 29, 2019
OneDev0411 added a commit to OneDev0411/serverless-express that referenced this pull request Mar 16, 2023
<a name="3.3.0"></a>
# [3.3.0](CodeGenieApp/serverless-express@v3.2.0...v3.3.0) (2018-08-16)

### Bug Fixes

* run npm audit fix ([85cbae6](CodeGenieApp/serverless-express@85cbae6))

### Features

* add option of specifying resolveMode ([#173](CodeGenieApp/serverless-express#173)) ([582b88d](CodeGenieApp/serverless-express@582b88d))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants