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

Allow sending multiple responses for GroupMe #109

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

Conversation

josephfrazier
Copy link

No description provided.

@simalexan
Copy link
Member

simalexan commented Dec 3, 2017

Hey, this contribution is great!
It's very appreciated and thank you for implementing it. We've looked into the code and would only suggest instead of the added dependency for promiseEach, maybe implement it using a similar solution mentioned here https://stackoverflow.com/questions/37916064/sequential-iteration-using-es6-promises

Because Lambda uploads take longer with more dependencies, we tend to keep as few as possible. What do you think?

Because Lambda uploads take longer with more dependencies,
we tend to keep as few as possible.

See claudiajs#109 (comment)
@josephfrazier
Copy link
Author

Thanks for the quick feedback! I copied the promise-each implementation into lib/promise-each.js, modified it to remove dependencies and linting errors, and used it instead. What do you think?

@josephfrazier
Copy link
Author

Hey @simalexan, have you had a chance to review the most recent changes? I just stumbled across this again and thought I'd check in.

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.

None yet

2 participants