search

laravel / reverb

Laravel Reverb provides a real-time WebSocket communication backend for Laravel applications.
https://reverb.laravel.com
MIT License
1.05k stars 76 forks source link

[1.x] Sends data property when empty #124

Closed jabools closed 5 months ago

jabools commented 5 months ago

This PR fixes missing data property for pusher_internal events.

Fixes #123

jabools commented 5 months ago

Working on test fixes.

joedixon commented 5 months ago

@jabools I'm pretty happy with the implmentation after a couple of tweaks, but interested if you can point me to the part of the Pusher docs which says this is a requirement. I can only see it in the Presence channel docs which we already handle and I'm reluctant to send more data across the wire than necessary.

jabools commented 5 months ago

Unfortunately, I am not able to find anything that would make that property a requirement. As Pusher's docs state:

All events received and sent by clients can contain a data field. [...] In order to keep the protocol consistent, Pusher Channels tries to send the data field as a string.

it is not mandatory for events to have data property and there are only information about data property for presence channels. However, I believe it's worth considering providing it in private channels as the official Pusher API provides it and also their official Swift package ignores subscription_succeeded event if there is no data provided (https://github.com/pusher/pusher-websocket-swift/blob/master/Sources/Services/PusherConnection.swift#L448). The latter may be (of course) a bug, but for now they are complementary for one another.

It also looks like every pusher internal event contains data property, which is reasonable - they contain the actual data. This may be some kind of common algorithm and they do the same also for subscription_succeeded event - but this is just an allegation :)

joedixon commented 5 months ago

Thanks @jabools - would you be able to test this branch and see if it resolves all of your issues?

szymjab commented 5 months ago

@joedixon I'm responding from my work account :) I've just tested this branch and it works like a charm - iOS application works as expected, web application also works well and no side effects were noticed.