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

Fix response ordering in https://github.com/serverless/examples/pull/81 #125

Closed
wants to merge 1 commit into from

Conversation

kaihendry
Copy link
Contributor

Sorry for the earlier error on #81

@christophgysin
Copy link
Contributor

Fixed in #124

@kaihendry
Copy link
Contributor Author

Thanks, the next thing that bothers me, is if the fetch fails, the callback returns {"errorMessage":"[object Object]"}

I'm sure sure if the Promise error or the Lambda callback is to blame.

@christophgysin
Copy link
Contributor

@kaihendry How about #126?

@kaihendry
Copy link
Contributor Author

kaihendry commented Mar 31, 2017

Looks good. How do I quickly pull in christophgysin:improve-error-message on my cli to test I wonder??

Btw this is sage advice from a better JS programmer than I @timoxley regarding the callback return:

in the first example (current state) with the .then(v => callback(null, v)).catch(callback), control goes:

  1. work
  2. callback
  3. work (bad: control should never be passed back to the async function after it calls callback)
  4. callback (bad: callbacks should never be executed twice)
  5. crash

while in the second example (proposal) with .then(v => callback(null, v), callback), control goes:

  1. work
  2. callback
  3. crash

in the second case, control flows correctly and the callback is run only once.

@christophgysin
Copy link
Contributor

How do I quickly pull in christophgysin:improve-error-message on my cli to test

git pull https://github.com/christophgysin/examples improve-error-message

@christophgysin
Copy link
Contributor

@kaihendry As for the advice, I'm not entirely following what you are referring to. Could you comment directly on the relevant lines in the code/PR you are referring to?

@kaihendry
Copy link
Contributor Author

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.

2 participants