Merge pull request swimlane#538 from gerhardboer/fix-orderable-unsubs…

…cribe

Fix for orderable unsubscribing

src/directives/orderable.directive.spec.ts

@@ -1,8 +1,10 @@
-import { async, TestBed, ComponentFixture } from '@angular/core/testing';
-import { Component, DebugElement } from '@angular/core';
-import { By } from '@angular/platform-browser';
+import {async, ComponentFixture, TestBed} from "@angular/core/testing";
+import {Component, ElementRef} from "@angular/core";
+import {By} from "@angular/platform-browser";
 
-import { OrderableDirective } from '.';
+import {OrderableDirective} from ".";
+import {DraggableDirective} from "./draggable.directive";
+import {id} from "../utils/id";
 
 @Component({
   selector: 'test-fixture-component',
@@ -21,7 +23,7 @@ describe('OrderableDirective', () => {
   // provide our implementations or mocks to the dependency injector
   beforeEach(() => {
     TestBed.configureTestingModule({
-      declarations: [ 
+      declarations: [
         OrderableDirective,
         TestFixtureComponent
       ]
@@ -45,7 +47,7 @@ describe('OrderableDirective', () => {
 
   describe('fixture', () => {
     let directive: OrderableDirective;
-    
+
     beforeEach(() => {
       directive = fixture.debugElement
         .query(By.directive(OrderableDirective))
@@ -59,5 +61,69 @@ describe('OrderableDirective', () => {
     it('should have OrderableDirective directive', () => {
       expect(directive).toBeTruthy();
     });
+
+   describe('when a draggable is removed', () => {
+
+      function checkAllSubscriptionsForActiveObservers() {
+        let subs = directive.draggables.map(d => {
+          expect(d.dragEnd.isStopped).toBe(false);
+          expect(d.dragStart.isStopped).toBe(false);
+
+          return {
+            dragStart: d.dragStart.observers,
+            dragEnd: d.dragEnd.observers
+          }
+        });
+
+        subs.forEach((sub) => {
+          expect(sub.dragStart.length).toBe(1);
+          expect(sub.dragEnd.length).toBe(1);
+
+        })
+      }
+
+      function newDraggable() {
+        const draggable = new DraggableDirective(<ElementRef>{});
+
+        // used for the KeyValueDiffer
+        draggable.dragModel = {
+          $$id: id()
+        };
+
+        return draggable;
+      }
+
+      let draggables;
+
+      beforeEach(() => {
+        draggables = [
+          newDraggable(),
+          newDraggable(),
+          newDraggable()
+        ];
+
+        directive.draggables.reset(draggables);
+
+        directive.updateSubscriptions();
+
+        checkAllSubscriptionsForActiveObservers();
+      });
+
+      it('then dragStart and dragEnd are unsubscribed from the removed draggable', () => {
+        let unsubbed = draggables.splice(0,1)[0];
+
+        expect(unsubbed.dragStart.isStopped).toBe(false);
+        expect(unsubbed.dragEnd.isStopped).toBe(false);
+
+        directive.draggables.reset(draggables);
+
+        directive.updateSubscriptions();
+
+        checkAllSubscriptionsForActiveObservers();
+
+        expect(unsubbed.dragStart.isStopped).toBe(true);
+        expect(unsubbed.dragEnd.isStopped).toBe(true);
+      });
+    });
   });
 });

src/directives/orderable.directive.ts

@@ -3,14 +3,14 @@ import {
   QueryList, KeyValueDiffers, AfterContentInit, OnDestroy
 } from '@angular/core';
 
-import { DraggableDirective } from './draggable.directive';
+import {DraggableDirective} from './draggable.directive';
 
-@Directive({ selector: '[orderable]' })
+@Directive({selector: '[orderable]'})
 export class OrderableDirective implements AfterContentInit, OnDestroy {
 
   @Output() reorder: EventEmitter<any> = new EventEmitter();
 
-  @ContentChildren(DraggableDirective, { descendants: true })
+  @ContentChildren(DraggableDirective, {descendants: true})
   draggables: QueryList<DraggableDirective>;
 
   positions: any;
@@ -24,7 +24,7 @@ export class OrderableDirective implements AfterContentInit, OnDestroy {
     // HACK: Investigate Better Way
     this.updateSubscriptions();
     this.draggables.changes.subscribe(
-        this.updateSubscriptions.bind(this));
+      this.updateSubscriptions.bind(this));
   }
 
   ngOnDestroy(): void {
@@ -35,20 +35,20 @@ export class OrderableDirective implements AfterContentInit, OnDestroy {
   }
 
   updateSubscriptions(): void {
-    const diffs = this.differ.diff(this.draggables.toArray());
+    const diffs = this.differ.diff(this.createMapDiffs());
 
-    if(diffs) {
-      const subscribe = ({ currentValue, previousValue }: any) => {
-        unsubscribe({ previousValue });
+    if (diffs) {
+      const subscribe = ({currentValue, previousValue}: any) => {
+        unsubscribe({previousValue});
 
-        if(currentValue) {
+        if (currentValue) {
           currentValue.dragStart.subscribe(this.onDragStart.bind(this));
           currentValue.dragEnd.subscribe(this.onDragEnd.bind(this));
         }
       };
 
-      const unsubscribe = ({ previousValue }: any) => {
-        if(previousValue) {
+      const unsubscribe = ({previousValue}: any) => {
+        if (previousValue) {
           previousValue.dragStart.unsubscribe();
           previousValue.dragEnd.unsubscribe();
         }
@@ -60,31 +60,39 @@ export class OrderableDirective implements AfterContentInit, OnDestroy {
     }
   }
 
+  private createMapDiffs(): { [key: string]: DraggableDirective } {
+    return this.draggables.toArray()
+      .reduce((acc, curr) => {
+        acc[curr.dragModel.$$id] = curr;
+        return acc;
+      }, {});
+  }
+
   onDragStart(): void {
     this.positions = {};
 
     let i = 0;
-    for(let dragger of this.draggables.toArray()) {
+    for (let dragger of this.draggables.toArray()) {
       let elm = dragger.element;
-      this.positions[dragger.dragModel.prop] =  {
+      this.positions[dragger.dragModel.prop] = {
         left: parseInt(elm.offsetLeft.toString(), 0),
         index: i++
       };
     }
   }
 
-  onDragEnd({ element, model }: any) {
+  onDragEnd({element, model}: any) {
     const newPos = parseInt(element.offsetLeft.toString(), 0);
     const prevPos = this.positions[model.prop];
 
     let i = 0;
-    for(let prop in this.positions) {
+    for (let prop in this.positions) {
       let pos = this.positions[prop];
 
       let movedLeft = newPos < pos.left && prevPos.left > pos.left;
       let movedRight = newPos > pos.left && prevPos.left < pos.left;
 
-      if(movedLeft || movedRight) {
+      if (movedLeft || movedRight) {
         this.reorder.emit({
           prevIndex: prevPos.index,
           newIndex: i,