Support listener context on Pusher subscriptions#490
Support listener context on Pusher subscriptions#490jlswanson28694 wants to merge 1 commit intolaravel:2.xfrom
Conversation
|
Thanks for the PR and the detailed write-up. I went back and forth on this one, but I think we're going to pass for now. A couple of reasons:
If you find this comes up a lot in practice and you can show real-world usage that |
|
@joetannenbaum Let me add more context by attempting to explain my use-case. I have a Vue component I'm allowing the user to generate multiple reports like this simultaneously. As a result, if one report completes, I can't run new Promise<ReportReadyData>((resolve, reject) => {
this.channel = this.echo.private(BroadcastChannels.reports(user.value.id))
.listen(
events.ReportReady,
(e: ReportReadyData) => {
if (e.report_id === this.report_id) resolve(e);
},
this.report_id
)
.listen(
events.ReportFailed,
(e: ReportIdentifier) => {
if (e.report_id === this.report_id) reject();
},
this.report_id
)
.error(reject, this.report_id);
})
.finally(() => {
this.channel.stopListeningForContext(this.report_id);
});Thought it looked pretty clean. As you said, Looking at this again, I suppose I could open a new channel for each report instead, but this is just to show you where my head was when I wrote the PR. No worries if you don't think it's worth adding. |
Pusher has the ability to add context as the third argument when binding and unbinding callbacks, which conveniently lets me remove multiple bound callbacks at once using
channel.unbind(undefined, undefined, context). This PR adds support for context when using Pusher channels, and adds a methodstopListeningForContextto remove all listeners for the given context.I've also added a
PusherChanneltest for this feature. In order to properly testPusherChannel, the test starts a bare-bones WebSocketServer with thewslibrary. I'm no expert on frontend testing, so feel free to clean it up as you see fit.