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

Add optional callback argument to stopListening() #292

Merged
merged 2 commits into from
Dec 19, 2020

Conversation

larsnystrom
Copy link
Contributor

(Second attempt, see #291)

This makes it possible to unregister a single callback for an event, without unregistering all other callbacks for that event on that channel. This is very helpful for example when using react hooks, where you need to stop listening as the component is unmounted. Here's an example React component using this new functionality:

const MyEventMessagesList = () => {
    const [messages, setMessages] = useState([]);

    useEffect(() => {
        const pushMessage = message => setMessages(prev => [...prev, message]);
        const channel = Echo.private("my.channel")
            .listen("MyEvent", pushMessage);

        // It used to be impossible to unregister a single callback
        // for an event, but now you can do this:
        return () => channel.stopListening("MyEvent", pushMessage); // Unregister only the pushMessage callback for this event
    }, [setMessages]);

    return (
        <ul>
            {messages.map((m, i) =>
                <li key={i}>{m}</li>
            )}
        </ul>
    )
}

Prior to this PR it was not possible to unregister a callback for an event. This is because the callback was wrapped inside the channel class to filter out events from other channels (see

let listener = (channel, data) => {
). It was therefore impossible to map the callback the user provided when calling channel.listen() to the callback which was actually registered with the socket. This in turn meant it was not possible to selectively remove just that callback from the socket.

I have solved this by registering just one callback per event, which in turn iterates over a list of user-provided callbacks. The list of user-provided callbacks can therefore be modified, and a single user-provided callback can be selectively unregistered.

I've added a few tests for the socket.io channel because the new code is a bit more complex than it used to be.

I have tried to not introduce any backwards incompatible changes here. That's why there's still an channel.on() function, despite it just proxying to the new channel.bindChannelEvent. All new functions has been marked as protected to discourage consumers of this package to call them directly. New properties have been marked as private.

This PR also fixes a bug in the socket.io channel, where calling stopListening(event) would stop the socket from listening to that event for all channels, not just the current one. If you look at this line:

this.socket.removeListener(name);
you'll see that all listeners are removed for that event on the underlying socket, not just the listeners specific to this channel (since all channels share the same socket). This PR solves that by always keeping track of which callbacks have been registered with the socket, and unregistering just those.

@taylorotwell
Copy link
Member

What is an event and what is a "channel event"?

@taylorotwell
Copy link
Member

The order of the methods in the PR seems all weird. Can the methods be placed in a more logical order?

@larsnystrom
Copy link
Contributor Author

What is an event and what is a "channel event"?

I have changed the PR now, so there are two props on SocketIoChannel: events and listeners. The events are callbacks applied to the socket given some event name. This means they are executed for every event with that name, regardless of which channel the event was sent to. The listeners are user-supplied callbacks, and they are only executed for the given event and on the given channel.

The order of the methods in the PR seems all weird. Can the methods be placed in a more logical order?

I have now split the PR in two commits, which has simplified the SocketIoChannel by moving some responsibility to SocketIoConnector. The result is that there is just one new method at the end of SocketIoChannel. I hope this makes the changes I've made more clear.

The 'reconnect' event is not sent on a channel, but on the socket, so
it makes more sense to deal with it on a socket-wide basis instead of
on a channel-by-channel basis.
This makes it possible to unregister a single callback for an event,
without unregistering all other callbacks for that event on that channel.
callback(data);
}
};
on(event: string, callback: Function): SocketIoChannel {
Copy link
Contributor Author

@larsnystrom larsnystrom Dec 19, 2020

Choose a reason for hiding this comment

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

I changed the return type of this method to be analogous to PusherChannel.on(). I'm not sure if that was a good idea or not.

@taylorotwell
Copy link
Member

I guess I'm confused - why this difference between events and listeners is even needed at all? Does Echo let you listen to events with a given name across all channels?

@taylorotwell taylorotwell merged commit ee7a93e into laravel:master Dec 19, 2020
@taylorotwell
Copy link
Member

I think I understand the reasoning now. I have released this as Echo 1.10.0. Thanks.

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.

2 participants