gilgen / ember-pusher

A proper Ember / Pusher integration.
MIT License
99 stars 35 forks source link

Pusher Unwire #40

Closed BradleyGoulding closed 7 years ago

BradleyGoulding commented 8 years ago

The Issue: Pusher unwire was never triggering the pusher.unsubscribe(channelName); (line 194) to unsubscribe form the channel. This is due to keys(bindings[channelName].eventBindings).length (line 193) not being equal to zero. This in turn is caused by the eventBindings.forEach in line 180 being broken by the splice eventBindings.splice(index, 1); in line 185 resetting the index of the array. This means that for an array of length n the foreach loop runs (n-1) times resulting in a single item array every time.

Current Code:

unwire(target, channelName, eventsToUnwire) {
    let pusher = this.pusher,
        bindings = this.get('bindings'),
        targetId = target._pusherEventsId(),
        channel = bindings[channelName].channel,
        eventBindings = bindings[channelName].eventBindings[targetId];

    if(typeof eventsToUnwire === 'string') {
      eventsToUnwire = [eventsToUnwire];
    }

    eventBindings.forEach((binding, index) => {
      if(eventsToUnwire && !eventsToUnwire.contains(binding.eventName)) {
        return;
      }
      channel.unbind(binding.eventName, binding.handler);
      eventBindings.splice(index, 1);
    });

    if (Ember.isEmpty(eventBindings)) {
      delete bindings[channelName].eventBindings[targetId];
    }

    // Unsubscribe from the channel if this is the last thing listening
    if (keys(bindings[channelName].eventBindings).length === 0) {
      pusher.unsubscribe(channelName);
      delete bindings[channelName];
      return true;
    }
    return false;
 },

Proposed Fix

The general consensus on stackoverflow is that this kind of array loop needs to be done backwards, so that the item popped off the end does not affect the next index. As Ember foreach does not operate backwards, regular JS is needed.

the quickest method is using a while(index--) loop, after setting index to the length of the array, thus the following solution is ideal

unwire(target, channelName, eventsToUnwire) {
    let pusher = this.pusher,
        bindings = this.get('bindings'),
        targetId = target._pusherEventsId(),
        channel = bindings[channelName].channel,
        eventBindings = bindings[channelName].eventBindings[targetId];

    if(typeof eventsToUnwire === 'string') {
      eventsToUnwire = [eventsToUnwire];
    }

    let index = eventBindings.length
    while (index--) {
      let binding = eventBindings[index];
      if(eventsToUnwire && !eventsToUnwire.contains(binding.eventName)) {
        return;
      }
      step++
      channel.unbind(binding.eventName, binding.handler);
      eventBindings.splice(index, 1);
    }

    if (Ember.isEmpty(eventBindings)) {
      delete bindings[channelName].eventBindings[targetId];
    }

    // Unsubscribe from the channel if this is the last thing listening
    if (keys(bindings[channelName].eventBindings).length === 0) {
      pusher.unsubscribe(channelName);
      delete bindings[channelName];
      return true;
    }
    return false;
 },

I will look into submitting a PR to resolve this issue

csprocket777 commented 8 years ago

Thanks for this fix. I was pulling my hair out as to why events such as "member_added" and "member_removed" weren't firing. this solved it.

gilgen commented 8 years ago

Merged https://github.com/jamiebikies/ember-pusher/commit/2127bf246b523bb00dc0f8102401cf1a37bbee53.

arthur5005 commented 7 years ago

Hey, this has affected me pretty badly, can we get a release out?

gilgen commented 7 years ago

Sorry about the delay, done!

brand-it commented 5 years ago

@jamiebikies you mind pushing v1.1.1 to NPM. We are using a workaround in our code right now but I would rather not use the hacky code but rather just use this.

gilgen commented 5 years ago

Done @newdark

brand-it commented 5 years ago

Wow thanks that so much