Added the events for unsubscribe calls #351
Conversation
|
|
|
Hi @reyalpsirc. |
|
@diegolmello I as using the DDP.js file alone on another project and noticed that there was no subscription to the "unsub"/"nosub" messages. However, it seems that when unsubscribing from "stream-room-messages", this error happens: https://forums.rocket.chat/t/unsubscribe-from-stream-room-messages/1500 I can't provide the project itself where I found the issue since I'm not allowed to but, any call to a unsubscribe method with a log on on the Promise resolve for the "unsubscribe" should be enough to see that it is never called without this |
Corrected issue where events were not being removed when calling the unsubscribe of a stream like stream-room-messages
|
@diegolmello I just updated this because the problem was not totally corrected. I basically have a DDP typescript implementation of this file and noticed that when I called the unsubscribe method and then the subscribe again, the old events linked to the previous subscribe would be revived when they actually shouldn't. |
|
@reyalpsirc Lint didn't pass. Can you fix it? |
|
@diegolmello fixed the lint errors |
app/lib/ddp.js
Outdated
| @@ -290,9 +296,12 @@ export default class Socket extends EventEmitter { | |||
| id, | |||
| name, | |||
| params, | |||
| unsubscribe: () => this.unsubscribe(id) | |||
| unsubscribe: () => { | |||
| // Also remove the events attached to the subscription of the collection | |||
app/lib/ddp.js
Outdated
| unsubscribe: () => this.unsubscribe(id) | ||
| unsubscribe: () => { | ||
| // Also remove the events attached to the subscription of the collection | ||
| this.removeAllForEvent(name); |
There was a problem hiding this comment.
you should call this method removeAllListeners inside of unsubscribe
There was a problem hiding this comment.
to use "removeAllListeners" there then we need to pass the name as an extra parameter on unsubscribe. Otherwise there's no way to link them together.
Moved "removeAllForEvent" into the unsubscribe method
|
@ggazzo I just updated the file |
| @@ -39,6 +39,11 @@ class EventEmitter { | |||
| } | |||
| } | |||
| } | |||
| removeAllForEvent(event) { | |||
There was a problem hiding this comment.
We fixed some bugs at #379.
removeAllForEvent doesn't seem to be necessary anymore, since every subscribe closes its listener.
There was a problem hiding this comment.
Hmm, I made this patch from a version of ddp.js that I have in typescript (made for a specific React Native project). In my case, I needed it so that I could clear all events that were added while the stream 'stream-room-messages' was open
There was a problem hiding this comment.
Ok. Sounds like it's a different behaviour than ours.
In our case, we would clear events one by one.
For us, removeAllForEvent could cause unpredictable behaviour in future.
| @@ -113,6 +118,7 @@ export default class Socket extends EventEmitter { | |||
|
|
|||
| this.on('result', data => this.ddp.emit(data.id, { id: data.id, result: data.result, error: data.error })); | |||
| this.on('ready', data => this.ddp.emit(data.subs[0], data)); | |||
| this.on('nosub', data => this.ddp.emit(data.id, data)); | |||
|
@diegodorgam guys react native app is very good, we love it. hats off to the hard working team. |
@RocketChat/ReactNative
Closes #ISSUE_NUMBER
There was no code checking for when the unsubscription happened and so it was never removed.