Skip to content

Allow sending multiple responses for GroupMe - #109

Open
josephfrazier wants to merge 4 commits into
claudiajs:masterfrom
josephfrazier:groupme-multiple
Open

Allow sending multiple responses for GroupMe#109
josephfrazier wants to merge 4 commits into
claudiajs:masterfrom
josephfrazier:groupme-multiple

Conversation

@josephfrazier

Copy link
Copy Markdown

No description provided.

@simalexan

simalexan commented Dec 3, 2017

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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.

2 participants