perform skyhookZone's change detection in the angular zone, and not j…

…ust for microtasks

Before, it used ApplicationRef.tick() from when skyhookZone's microTaskQueue was empty.
Now, it switches back to angular via ngZone.run() and the associated onLeave handler, but also on macroTasks.
This means if we set up timers in the skyhook zone, they will fire and cause change detection. Useful if doing .listen(...).delay(1000) and the resulting asynchronous subscribers.
Appropriately, we run more setup handlers in skyhookZone now.
Proper event handlers (set up by the backend) don't trigger any directly, because skyhookZone only cares about # of evt handlers changing => 0. But if we care about them, it will be through listen(), updates to which will schedule a microTask.

packages/angular-skyhook/src/connection-types.ts

@@ -9,7 +9,7 @@ import { TypeOrTypeArray } from './type-ish';
 import { Observable } from 'rxjs/Observable';
 import { DragLayerMonitor } from './layer-monitor';
 import { DropTargetConnector, DragSourceConnector } from './connectors';
-import { Subscription, ISubscription } from 'rxjs/Subscription';
+import { Subscription, ISubscription, TeardownLogic } from 'rxjs/Subscription';
 
 /** @private */
 export interface ConnectionBase<TMonitor> extends ISubscription {
@@ -54,6 +54,32 @@ export interface ConnectionBase<TMonitor> extends ISubscription {
    */
   unsubscribe(): void;
 
+  /**
+   * Same as RxJS Subscription.add().
+   * Useful, for example, for writing wrappers for the [[SkyhookDndService]] methods,
+   * which might internally listen()/subscribe to [[DropTargetSpec.hover]] and provide
+   * a convenient callback after you hover without dropping or exiting for a specified
+   * duration. That would require the following pattern:
+   *
+   * ```typescript
+   * function wrapper(dndService, types, spec, callback) {
+   *     let subj = new Subject();
+   *     let dt = dndService.dropTarget(types, {
+   *         ...spec,
+   *         hover: monitor => {
+   *             subj.next();
+   *             spec.hover && spec.hover(monitor);
+   *         }
+   *     });
+   *     // runs the callback until the returned connection
+   *     // is destroyed via unsubscribe()
+   *     dt.add(subj.pipe( ... ).subscribe(callback))
+   *     return dt;
+   * }
+   * ```
+   */
+  add(teardown: TeardownLogic): Subscription;
+
 }
 
 /** @private */

packages/angular-skyhook/src/connector.service.ts

@@ -4,7 +4,7 @@
 /** a second comment */
 
 import { invariant } from './internal/invariant';
-import { Injectable, Inject, ElementRef, NgZone, ApplicationRef } from '@angular/core';
+import { Injectable, Inject, ElementRef, NgZone } from '@angular/core';
 import { DRAG_DROP_BACKEND, TYPE_DYNAMIC } from './tokens';
 import { DragDropManager } from 'dnd-core';
 
@@ -60,12 +60,38 @@ export class SkyhookDndService {
 
   private skyhookZone: Zone = Zone.root.fork({
     name: 'skyhookZone',
-    // arrow fn on purpose
     onHasTask: (parentZoneDelegate, currentZone, targetZone, state) => {
-      if (state.change === 'microTask' && !state.microTask) {
-        this.appRef.tick();
+
+      // when we've | drained the microTask queue; or                    | ... run a change detection cycle.
+      //            | executed or cancelled a macroTask (eg a timer); or |
+      //            | handled an event                                   |
+
+      // note: we must use ngZone.run() instead of ApplicationRef.tick()
+      // this is because
+      // 1. this callback runs outside the angular zone
+      // 2. therefore if you use appRef.tick(), the event handlers set up during the tick() are
+      //    not in the angular zone, even though anything set up during tick() should be
+      // 3. therefore you get regular (click) handlers from templates running in skyhookZone
+      //    and not causing change detection
+
+      // Also, now we watch for macroTasks as well.
+      // This means if we set up timers in the skyhook zone, they will fire and cause change
+      // detection. Useful if doing .listen(...).delay(1000) and the resulting asynchronous
+      // subscribers.
+      // Appropriately, we run more setup handlers in skyhookZone now.
+      //
+      // Proper event handlers (set up by the backend) don't trigger any, because skyhookZone
+      // only cares about # of handlers changing => 0. But if we care about them, it will be
+      // through listen(), updates to which will schedule a microTask.
+
+      if (!state[state.change]) {
+        this.ngZone.run(() => {
+          // noop, but causes change detection (i.e. onLeave)
+        });
       }
     },
+    // onInvokeTask: (zoneDelegate, currentZone, targetZone, task, applyThis, applyArgs) => {
+    // }
     // onScheduleTask(parentZoneDelegate, currentZone, targetZone, task) {
     //   return parentZoneDelegate.scheduleTask(targetZone, task);
     // },
@@ -74,7 +100,7 @@ export class SkyhookDndService {
   });
 
   /** @private */
-  constructor(private manager: DragDropManager, private ngZone: NgZone, private appRef: ApplicationRef) {
+  constructor(private manager: DragDropManager, private ngZone: NgZone) {
   }
 
   /**
@@ -85,8 +111,8 @@ export class SkyhookDndService {
    * [[DropTarget.setTypes]] in a lifecycle hook.
    */
   public dropTarget(types: TypeOrTypeArray | null, spec: DropTargetSpec, subscription?: Subscription): DropTarget {
-    return this.ngZone.runOutsideAngular(() => {
-    // return this.skyhookZone.run(() => {
+    // return this.ngZone.runOutsideAngular(() => {
+    return this.skyhookZone.run(() => {
       const createTarget: any = createTargetFactory(spec, this.skyhookZone);
       const Connection: any = targetConnectionFactory({
         createHandler: createTarget,
@@ -126,8 +152,8 @@ export class SkyhookDndService {
    * @param subscription See [[1-Top-Level]]
    */
   public dragSource(type: string|symbol|null, spec: DragSourceSpec, subscription?: Subscription): DragSource {
-    return this.ngZone.runOutsideAngular(() => {
-    // return this.skyhookZone.run(() => {
+    // return this.ngZone.runOutsideAngular(() => {
+    return this.skyhookZone.run(() => {
       const createSource = createSourceFactory(spec, this.skyhookZone);
       const Connection = sourceConnectionFactory({
         createHandler: createSource,
@@ -147,8 +173,8 @@ export class SkyhookDndService {
    * This method creates a [[DragLayer]] object
    */
   public dragLayer(subscription?: Subscription): DragLayer {
-    return this.ngZone.runOutsideAngular(() => {
-    // return this.skyhookZone.run(() => {
+    // return this.ngZone.runOutsideAngular(() => {
+    return this.skyhookZone.run(() => {
       const conn = new DragLayerConnectionClass(this.manager, this.skyhookZone);
       if (subscription) {
         subscription.add(conn);

packages/angular-skyhook/src/index.ts

@@ -33,3 +33,4 @@ export { DragSourceSpec } from './drag-source-spec';
 
 export { DragSourceDirective, DropTargetDirective, DragPreviewDirective } from './dnd.directive';
 
+export { Offset } from './type-ish';

packages/angular-skyhook/src/internal/connection-factory.ts

@@ -6,7 +6,7 @@
 import { NgZone } from '@angular/core';
 import { invariant } from './invariant';
 import { TypeOrTypeArray } from '../type-ish';
-import { Subscription } from 'rxjs/Subscription';
+import { Subscription, TeardownLogic } from 'rxjs/Subscription';
 import { Observable } from 'rxjs/Observable';
 import { ReplaySubject } from 'rxjs/ReplaySubject';
 import { BehaviorSubject } from 'rxjs/BehaviorSubject';
@@ -89,7 +89,6 @@ function connectionFactory<TMonitor extends DragSourceMonitor | DropTargetMonito
       private skyhookZone: Zone,
       initialType: TypeOrTypeArray | undefined,
     ) {
-
       invariant(
         typeof manager === 'object',
         // TODO: update this mini-documentation
@@ -118,35 +117,35 @@ function connectionFactory<TMonitor extends DragSourceMonitor | DropTargetMonito
       // This isn't 100% true, but there is no way of knowing (even if you ref-count it)
       // when a component no longer needs it.
       return this.resolvedType$.pipe(
-          // this ensures we don't start emitting values until there is a type resolved
-          take(1),
-          // switch our attention to the incoming firehose of 'something changed' events
-          switchMapTo(this.collector$),
-          // turn them into 'interesting state' via the monitor and a user-provided function
-          map(mapFn),
-          // don't emit EVERY time the firehose says something changed, only when the interesting state changes
-          distinctUntilChanged(areCollectsEqual),
-          // this schedules a single batch change detection run after all the listeners have heard their newest value
-          // thus all changes resulting from subscriptions to this are caught by the
-          // change detector.
-          scheduleMicroTaskAfter(this.skyhookZone)
+        // this ensures we don't start emitting values until there is a type resolved
+        take(1),
+        // switch our attention to the incoming firehose of 'something changed' events
+        switchMapTo(this.collector$),
+        // turn them into 'interesting state' via the monitor and a user-provided function
+        map(mapFn),
+        // don't emit EVERY time the firehose says something changed, only when the interesting state changes
+        distinctUntilChanged(areCollectsEqual),
+        // this schedules a single batch change detection run after all the listeners have heard their newest value
+        // thus all changes resulting from subscriptions to this are caught by the
+        // change detector.
+        scheduleMicroTaskAfter(this.skyhookZone)
       );
     }
 
     connect(fn: (connector: TConnector) => void): Subscription {
       return this.resolvedType$.pipe(take(1)).subscribe(() => {
-        // outside because we want the event handlers set up inside fn to
-        // fire outside the zone
-        this.ngZone.runOutsideAngular(() => {
+        // must run inside skyhookZone, so things like timers firing after a long hover with touch backend
+        // will cause change detection (via executing a macro or event task)
+        this.skyhookZone.run(() => {
           fn(this.handlerConnector.hooks);
         });
       });
     }
 
     setTypes(type: TypeOrTypeArray) {
-      // make super sure. I think this is mainly a concern when creating DOM
-      // event handlers, but it doesn't hurt here either.
-      this.ngZone.runOutsideAngular(() => {
+      // must run inside skyhookZone, so things like timers firing after a long hover with touch backend
+      // will cause change detection (via executing a macro or event task)
+      this.skyhookZone.run(() => {
         this.receiveType(type);
         this.resolvedType$.next(1);
       });
@@ -206,6 +205,10 @@ function connectionFactory<TMonitor extends DragSourceMonitor | DropTargetMonito
       this.subscriptionConnectionLifetime.unsubscribe();
     }
 
+    add(teardown: TeardownLogic): Subscription {
+      return this.subscriptionConnectionLifetime.add(teardown);
+    }
+
     get closed() {
       return this.subscriptionConnectionLifetime && this.subscriptionConnectionLifetime.closed;
     }

packages/angular-skyhook/src/internal/createSourceFactory.ts

@@ -18,7 +18,7 @@ export function createSourceFactory(spec: DragSourceSpec, zone: Zone): any {
     }
 
     withChangeDetection<T>(fn: () => T): T {
-      let x = fn();
+      let x = fn()
       zone.scheduleMicroTask('DragSource', () => {});
       return x;
     }
@@ -29,7 +29,7 @@ export function createSourceFactory(spec: DragSourceSpec, zone: Zone): any {
       }
 
       return this.withChangeDetection(() => {
-        return spec.canDrag && spec.canDrag(this.monitor);
+        return spec.canDrag(this.monitor);
       });
     }
 

packages/angular-skyhook/src/internal/createTargetFactory.ts

@@ -10,7 +10,6 @@ import { invariant } from './invariant';
 import { DropTargetSpec } from '../drop-target-spec';
 
 export function createTargetFactory(spec: DropTargetSpec, zone: Zone): any {
-  // zone = { run: (f) => { return f() } } as any;
 
   class Target {
     monitor: any;
@@ -44,9 +43,7 @@ export function createTargetFactory(spec: DropTargetSpec, zone: Zone): any {
         return;
       }
       this.withChangeDetection(() => {
-        if (spec.hover) {
-          spec.hover(this.monitor);
-        }
+        spec.hover(this.monitor);
       });
     }
 
@@ -56,7 +53,7 @@ export function createTargetFactory(spec: DropTargetSpec, zone: Zone): any {
       }
 
       return this.withChangeDetection(() => {
-        const dropResult = spec.drop && spec.drop(this.monitor);
+        const dropResult = spec.drop(this.monitor);
         return dropResult;
       });
     }

packages/angular-skyhook/src/internal/drag-layer-connection.ts

@@ -11,7 +11,7 @@ import { DragLayerMonitor } from '../layer-monitor';
 import { InternalMonitor } from './internal-monitor';
 import { areCollectsEqual } from '../utils/areCollectsEqual';
 import { map, distinctUntilChanged } from 'rxjs/operators';
-import { Subscription } from 'rxjs/Subscription';
+import { Subscription, TeardownLogic } from 'rxjs/Subscription';
 import { scheduleMicroTaskAfter } from './scheduleMicroTaskAfter';
 
 export class DragLayerConnectionClass implements DragLayer {
@@ -64,6 +64,10 @@ export class DragLayerConnectionClass implements DragLayer {
     this.subscription.unsubscribe();
   }
 
+  add(teardown: TeardownLogic): Subscription {
+    return this.subscription.add(teardown);
+  }
+
   get closed() {
     return this.subscription.closed;
   }