-
Notifications
You must be signed in to change notification settings - Fork 7
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
then should accept a function rather than Promise.all directly #2
base: master
Are you sure you want to change the base?
Conversation
I for one think you're right and I'm using the same fix. I was writing my tests today and making sure the handler could fail and leave the item in the queue wasn't happening until I applied this. |
Cool, yeah I think it's because the |
I've been using this in a production lambda, it's working well. Let me know what you think about my latest changes. Basically I pass all the items to the item callback, and only delete the items that resolved successfully. Do you think it'd be helpful to have some type of error callback? That way you handle caught promise rejections further down the chain if desired. Also, what's the use case for handling the whole list of messages vs one message at a time? I.e. |
@geoffdutton thanks for this! I just finished doing nearly the exact same thing myself and was coming to make a PR when I saw this! One thing that I might suggest, as you are now tracking failed requests in the metrics, I added another count for success in order to present a clear picture of the results. eg |
If this works can we get it on master, so people that are using NPM don't need to hunt this down :) |
Agreed with @achadee. |
Just checking in on the status of the code merge for this PR. Very useful so looking forward to seeing this merged. Thanks! |
Potential fix for #1 (if #1 is even an issue).
Is this the intended behavior? If any one of the Item callbacks rejects, it should quit?