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

Promisify the Slack Web API #278

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@mjesuele

mjesuele commented Jun 15, 2016

This pull request uses Bluebird to add promise support to the Slack Web API while retaining support for the original callback style.

If a callback is passed as the last argument to a Slack API call then it is executed as before. Otherwise, a promise is returned.

Feedback is very welcome. If there's interest, I can try my hand at adding promise support to the Facebook and/or Twilio interfaces as well.

@mjesuele

This comment has been minimized.

Show comment
Hide comment
@mjesuele

mjesuele Jul 7, 2016

Anyone have any interest in merging this?

mjesuele commented Jul 7, 2016

Anyone have any interest in merging this?

@colestrode

This comment has been minimized.

Show comment
Hide comment
@colestrode

colestrode Jul 11, 2016

Contributor

I'd like to hear some more discussion from the community. In other projects I've found maintaining both promises and callbacks gets pretty tricky (especially if there are multiple error states or early returns). Personally my favored approach is to support callbacks, since that's idiomatic for Node, and then users can promisify the API using bluebird or Q.

Contributor

colestrode commented Jul 11, 2016

I'd like to hear some more discussion from the community. In other projects I've found maintaining both promises and callbacks gets pretty tricky (especially if there are multiple error states or early returns). Personally my favored approach is to support callbacks, since that's idiomatic for Node, and then users can promisify the API using bluebird or Q.

@mjesuele

This comment has been minimized.

Show comment
Hide comment
@mjesuele

mjesuele Jul 12, 2016

@colestrode, bear with me if I'm missing something about your response, but I'm actually just using Bluebird's promisify method (e8520ac#diff-e55a35b8c5887b6a2e0f96549d9a053fR42). If that's the approach you're suggesting the end user take (which I initially tried but couldn't figure out a reasonable way to do) then I'm not sure what the issue is with incorporating it directly into Botkit.

If you know of a way that I can promisify the API on my end, let me know, because then regardless of whether this PR gets merged, I can resume using the official Botkit package rather than my own fork as I'm doing now.

mjesuele commented Jul 12, 2016

@colestrode, bear with me if I'm missing something about your response, but I'm actually just using Bluebird's promisify method (e8520ac#diff-e55a35b8c5887b6a2e0f96549d9a053fR42). If that's the approach you're suggesting the end user take (which I initially tried but couldn't figure out a reasonable way to do) then I'm not sure what the issue is with incorporating it directly into Botkit.

If you know of a way that I can promisify the API on my end, let me know, because then regardless of whether this PR gets merged, I can resume using the official Botkit package rather than my own fork as I'm doing now.

@colestrode

This comment has been minimized.

Show comment
Hide comment
@colestrode

colestrode Jul 12, 2016

Contributor

Hey @mjesuele :) Yeah, what I'm proposing is that the end user create their own promsified version of the lib. I did something similar with the storage module: https://github.com/colestrode/botkit-promise-storage (though I used Q instead of bluebird).

I have two concerns: the first is that, IMO, promisifying the slack api will create an inconsistent user experience since the rest of the botkit library only supports callbacks. I love promises, so if that's the way the community wants to go, I think we should go all in and support a promise-based interface. But having promises only in some places but not others leads to weird expectations and pitfalls for future users.

My second concern is around maintenance. In previous libraries I've contributed to we've attempted to support both promises and callbacks and it lead to a lot of complex code that wasn't easy to abstract, particularly around error handling or if there were multiple return points.

Again, I dig promises so I think this is really cool work and am really eager to hear what other people think. I just want to make sure we're thoughtful whatever we do :)

Contributor

colestrode commented Jul 12, 2016

Hey @mjesuele :) Yeah, what I'm proposing is that the end user create their own promsified version of the lib. I did something similar with the storage module: https://github.com/colestrode/botkit-promise-storage (though I used Q instead of bluebird).

I have two concerns: the first is that, IMO, promisifying the slack api will create an inconsistent user experience since the rest of the botkit library only supports callbacks. I love promises, so if that's the way the community wants to go, I think we should go all in and support a promise-based interface. But having promises only in some places but not others leads to weird expectations and pitfalls for future users.

My second concern is around maintenance. In previous libraries I've contributed to we've attempted to support both promises and callbacks and it lead to a lot of complex code that wasn't easy to abstract, particularly around error handling or if there were multiple return points.

Again, I dig promises so I think this is really cool work and am really eager to hear what other people think. I just want to make sure we're thoughtful whatever we do :)

@mjesuele

This comment has been minimized.

Show comment
Hide comment
@mjesuele

mjesuele Jul 12, 2016

@colestrode, I understand now about building a promisified version as a separate module, thanks for the link 😎

I see your point regarding the inconsistent user experience. Based on the number of thumbs-up this PR has gotten, I assume that there's a lot of interest in promisifying more of Botkit, and I'd be happy to contribute to that effort. I can see why we'd want to avoid introducing promise support in a piecemeal fashion -- at the same time, this is useful now.

It actually looks like the Twilio API is already promise-based, incidentally, and the Facebook adapter doesn't have any comparable API, so adding promise support to the Slack API might be a move towards greater internal coherence after all, even if the rest of Botkit remains callback-based.

I'm curious what the issue with maintenance is, at least for the simple case of the Slack API. My impression was that as long as the callbacks are indeed idiomatic Node, Bluebird's promisification should work pretty much automatically. With this last point, I might be asking for a little schooling, so I appreciate your time in responding.

Thanks!

mjesuele commented Jul 12, 2016

@colestrode, I understand now about building a promisified version as a separate module, thanks for the link 😎

I see your point regarding the inconsistent user experience. Based on the number of thumbs-up this PR has gotten, I assume that there's a lot of interest in promisifying more of Botkit, and I'd be happy to contribute to that effort. I can see why we'd want to avoid introducing promise support in a piecemeal fashion -- at the same time, this is useful now.

It actually looks like the Twilio API is already promise-based, incidentally, and the Facebook adapter doesn't have any comparable API, so adding promise support to the Slack API might be a move towards greater internal coherence after all, even if the rest of Botkit remains callback-based.

I'm curious what the issue with maintenance is, at least for the simple case of the Slack API. My impression was that as long as the callbacks are indeed idiomatic Node, Bluebird's promisification should work pretty much automatically. With this last point, I might be asking for a little schooling, so I appreciate your time in responding.

Thanks!

@colestrode

This comment has been minimized.

Show comment
Hide comment
@colestrode

colestrode Jul 12, 2016

Contributor

Good point about the Twilio API already supporting promises! I didn't realize that, in that case I take back what I said about inconsistencies :) Promisfying the Slack API would bring more consistency.

As for maintenance, I like your approach of writing a "private" callback-style method and then conditionally promisfying that. That definitely makes the early exit and error cases easier to manage.

I think that would introduce some maintenance overhead if applied to the wider botkit framework, but honestly I'm not as concerned as I was :) So I'm for this if other people are :)

Contributor

colestrode commented Jul 12, 2016

Good point about the Twilio API already supporting promises! I didn't realize that, in that case I take back what I said about inconsistencies :) Promisfying the Slack API would bring more consistency.

As for maintenance, I like your approach of writing a "private" callback-style method and then conditionally promisfying that. That definitely makes the early exit and error cases easier to manage.

I think that would introduce some maintenance overhead if applied to the wider botkit framework, but honestly I'm not as concerned as I was :) So I'm for this if other people are :)

@mjesuele

This comment has been minimized.

Show comment
Hide comment
@mjesuele

mjesuele Jul 21, 2016

@benbrown, is there anything addressable in the way of this being merged?

mjesuele commented Jul 21, 2016

@benbrown, is there anything addressable in the way of this being merged?

@tlvenn

This comment has been minimized.

Show comment
Hide comment
@tlvenn

tlvenn Oct 23, 2016

Any update on this ?

tlvenn commented Oct 23, 2016

Any update on this ?

@benbrown benbrown added the DONT_MERGE label Nov 7, 2016

@ohbarye

This comment has been minimized.

Show comment
Hide comment
@ohbarye

ohbarye Dec 24, 2016

to maintainers
This PR seems not to break compatibility and to give a great feature which also people (including me) really want.
What's getting on the way of merge?

ohbarye commented Dec 24, 2016

to maintainers
This PR seems not to break compatibility and to give a great feature which also people (including me) really want.
What's getting on the way of merge?

@GauSim

This comment has been minimized.

Show comment
Hide comment
@GauSim

GauSim commented Dec 30, 2016

+1

@tlvenn

This comment has been minimized.

Show comment
Hide comment
@tlvenn

tlvenn Mar 17, 2017

@benbrown any update beside the do not merge label plz ?

tlvenn commented Mar 17, 2017

@benbrown any update beside the do not merge label plz ?

@mjesuele mjesuele closed this Apr 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment