Echo subscription driver: Bug, Inconsistency, Missing documentation

[ssi-anik]

Describe the bug
I was trying to figure out the subscription with the echo driver. I found the following things are wrongly done.

• Package generates a unique private channel to listen to as named private-*
  

  lighthouse/src/Subscriptions/Subscriber.php

  Lines 170 to 173 in dba839e

  	public static function uniqueChannelName(): string
  	{
  	return 'private-lighthouse-'.Str::random(32).'-'.time();
  	}

But when broadcasting, it then broadcasts to a presence channel. which prepends presensce- to the channel making presence-private-lighthouse-*. So, it should always be handled as a presence channel?

lighthouse/src/Subscriptions/Events/EchoSubscriptionEvent.php

Lines 29 to 32 in dba839e

	public function broadcastOn(): PresenceChannel
	{
	return new PresenceChannel($this->channel);
	}

• Is it intentional to keep separate event names?
  

  lighthouse/src/Subscriptions/Events/EchoSubscriptionEvent.php

  Line 41 in dba839e

  return 'lighthouse.subscription';

  
  

  lighthouse/src/Subscriptions/Broadcasters/PusherBroadcaster.php

  Line 14 in dba839e

  public const EVENT_NAME = 'lighthouse-subscription';

• Even if user wants to publish the subscription data in sync, it gets pushed to the queue.
  

  lighthouse/src/lighthouse.php

  Line 313 in dba839e

  'queue_broadcasts' => env('LIGHTHOUSE_QUEUE_BROADCASTS', true),

  
  Because, it uses ShouldBroadcast, which is pushed to the queue. It should be ShouldBroadcastNow to send it in sync manner.
  

  lighthouse/src/Subscriptions/Events/EchoSubscriptionEvent.php

  Line 8 in dba839e

  class EchoSubscriptionEvent implements ShouldBroadcast

• To broadcast a message through the echo driver, it requires to install illuminate/broadcasting package when using Laravel. If using pusher, it's not required. It's not mentioned in the documentation.

Lighthouse Version: * v5.1.0

[spawnia]

@stayallive @alberthaff what do you think?

[ssi-anik]

Implemented the echo driver here: https://github.com/ssi-anik/confession-wall/blob/graphql/graphql/schema.graphql#L22
and here: https://github.com/ssi-anik/lighthouse-laravel-websockets/blob/master/resources/js/echo.link.js

[stayallive]

It looks like there are some things going wrong in the Echo driver indeed.

The channel name and it using a presence channel are from what I can tell wrong indeed (the channel name with a private channel would also not work because it will prepand another private- to it so that needs some changing).

And the ShouldBroadcast causes the broadcast to be queued again basically double queuing Laravel Echo messages.

The event name is confusing but not a bug but would be nice to keep them in sync.

To broadcast a message through the echo driver, it requires to install illuminate/broadcasting package when using Laravel. If using pusher, it's not required. It's not mentioned in the documentation.

This one I'm not sure, because for Pusher you also need that package for it to register Pusher in the container, but you could do that manually I guess. But this is a Lumen "issue".

@ssi-anik I think we could all agree we'd be happy to get a PR solving the issues since you seem to be actively using it with Laravel Echo, would you be up for that?

[ssi-anik]

@stayallive Yes, I can help with it. But I lack the knowledge of testing. So, I can send a PR but if you're looking for testing, then you'll have to help me out.

[spawnia]

Just look at the existing tests related to subscriptions and consider https://github.com/nuwave/lighthouse/blob/master/CONTRIBUTING.md.

[ssi-anik]

I'm interested in working on this issue. I need some time to start working on it. If someone starts before me, then please give a heads-up here. And, the slack invitation has expired. I Will ask a few questions when I start working.
Thanks.

[stayallive]

This invite link should work: Slack invite.

Where did you find that expired link? The on in the readme works fine.

[spawnia]

Where did you find that expired link? The on in the readme works fine.

That is because i just updated it 😉

[alberthaff]

Sorry for the late reply.
I have no experience with the echo driver. But something definitely seems wrong with the channel names and queuing.

Regarding the docs - i actually can't find anything about the echo driver, except the client implementation. If we are supporting multiple drivers, maybe we should add a section about the individual drivers, their requirements and client-implementations.