Skip to content
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

repair socket io leave method #33

Closed
wants to merge 4 commits into from
Closed

repair socket io leave method #33

wants to merge 4 commits into from

Conversation

pderiy
Copy link

@pderiy pderiy commented Aug 29, 2016

Hi, Taylor

3 days ago I sent to you PR with unsubscribe socket.io method override.
Method "this.socket.removeAllListeners();" doesn't work like I expected. This method deletes all callbacks from all channels. For example I have a reconnector and channel that listening new orders. When I'll leave from orders channel, this method will also remove a reconnector callbacks.

I wrote a method, that saves a collection of channel callbacks. And when user will try to unsubscrube from a channel, then all callbacks will be also deleted.

Now channel collection looks like:

this.channels = {
'channel-name' => [
'channel' => channel_object,
'events' => {
"event-name" => [ functions.... ]
}
]
}

My PR for now working perfect with "underscore". Of course you can do refactoring and remove underscore. These methods tested and work as expected. Right now, method "leave" works good with 'socket.io'. When user is trying to leave from a channel it unsubscribes and removes all callbacks only from one channel, by using method from "socket.io" library - removeListener( event, callback).

P.S. Sorry for my bad english

@taylorotwell
Copy link
Member

Can you describe what you're trying to accomplish here in simple terms? Are you trying to unsubscribe from a single channel? From a single event on that channel?

@pderiy
Copy link
Author

pderiy commented Aug 30, 2016

Answer: I'm trying to unsubscribe from one channel and I'm trying to remove all related callbacks for this channel from "socket.io" callback list.

Additional information: I'm using angular routes. When i'm surfing between view states, I need to connect to another channels. And system has to manage callbacks correctly.

Without this patch: On unsubscribe from a channel system will remove all callbacks (callbacks from another channels too).

This patch gives you a correct way to manage your channel subscriptions and callbacks.

I'm not sure: This patch is using 'underscore', but laravel echo has no this dependency. Of course you can rewrite it or add underscore (or lodash) to echo dependencies.

If it's not a good answer, I can try to describe it better.

@taylorotwell
Copy link
Member

Can you rewrite it without using underscore?

@pderiy
Copy link
Author

pderiy commented Aug 31, 2016

Done.

@pderiy
Copy link
Author

pderiy commented Sep 3, 2016

@taylorotwell Can you accept this PR? If something wrong, just tell me, and I will fix it. Right now it works without underscore.

@@ -41,7 +41,7 @@ export class SocketIoPresenceChannel extends SocketIoChannel implements Presence
*/
leaving(callback): SocketIoPresenceChannel {
this.on('presence:leaving', (member) => {
callback(member.user_info, this.channel);
callback(member.user_info, this.channel.socket);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why you're changing all of these.

Copy link
Author

@pderiy pderiy Sep 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to send in callback full channel object. So I changed it to socket object.
On this screen You can see a full channel object, that includes event list with registered callbacks and socket:
image

Before this patch you was sending sockets and you will send sockets now too.

Of course, I can delete these changes in "socketio-presence-channel.ts" file.

@pderiy
Copy link
Author

pderiy commented Sep 7, 2016

Hi, @taylorotwell . I know you have another things to do. But I need this update, because when I'm trying to unsubscribe from a channel, it also removes callbacks from another channels. So this update is important for me and for another 'laravel-echo' users. It is tested in real project and it works good. If you have any questions, I'll be good to answer. Can you accept this PR?

@tlaverdure
Copy link
Contributor

I'm familiar with the situation you are dealing with when working with an SPA. I'll take a second look at this @taylorotwell.

@pderiy in the mean time, just fork the repo and add your patch, and include locally if it is blocking your progress.

@taylorotwell
Copy link
Member

This has merge conflicts.

@pderiy
Copy link
Author

pderiy commented Sep 9, 2016

@taylorotwell , @tlaverdure fixed merge conflicts.

  1. Latest version of laravel-echo(1.0.2) is not working with laravel-echo-server(0.9.65). I believe it was prepeared for laravel-echo-server(1.0.0).
  2. I fixed my merge conflicts fot latest update, but for now I have troubles with running laravel-echo-server v1.0.0 and I can't test it with laravel-echo-server v1.0.0.

@pderiy
Copy link
Author

pderiy commented Sep 10, 2016

@taylorotwell , @tlaverdure The problem is not fixed in laravel-echo-server v1.0.2.

U can simply test it.
Add script like.

  1. Subscribe 'orders' channel.
  2. Unsubscribe 'orders' channel.
  3. Subscribe 'orders' channel.
  4. Emit this any event.

You will receive data 2 times. Because it's still not fixed. But my PR will fix this problem.

@tlaverdure
Copy link
Contributor

@pderiy you are right, there is still a case here that needs to be addressed. I'm going to submit a new PR based on what you've submitted that is a bit more concise. There is another issue that I caught as well that comes with killing listeners from different channels with the same event name.

Will submit soon and I'll also check if we need this for Pusher, which I assume we will since Pusher doesn't automatically maintain the state of callbacks applied to a channel.

@pderiy
Copy link
Author

pderiy commented Sep 11, 2016

@tlaverdure My patch removes only one callback. This patch is not killing listeners from different channels!

Now laravel-echo have a list of channels and channel-listeners. When you need to unsubscribe from a channel it is going foreach loop and removes callbacks from used channel using "socket.io" method:
channel.removeListener(event, callback)

it doesn't remove listeners from another channels! It is what you need.
I think this is a good way to resolve this problem.

@tlaverdure you're right, this problem might be in Pusher. But Pusher might have another structure, so you have to write something else for pusher. But for socket.io this is a good way to resolve this problem. It remembers all callbacks for channel and then removes only needed callbacks from needed channels.

@tlaverdure you can test it from my fork laravel-echo. Now I have resolved this problem and it perfectly works with laravel-echo-server v1.0.0. And if you will see this as good solution, I can submit a new PR.

@tlaverdure Laravel-echo-server v1.0.* looks very good! Is much better than it was. Good job!

@pderiy
Copy link
Author

pderiy commented Sep 11, 2016

Thanks @tlaverdure . Your PR is fixing this problem!

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.

3 participants