From a5ec19108b17dcb75ef823e5f4e922fdf764e083 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Mon, 15 Jan 2018 10:21:30 +1100 Subject: [PATCH] perform skyhookZone's change detection in the angular zone, and not just 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. --- .../angular-skyhook/src/connection-types.ts | 28 ++++++++++- .../angular-skyhook/src/connector.service.ts | 48 ++++++++++++++----- packages/angular-skyhook/src/index.ts | 1 + .../src/internal/connection-factory.ts | 43 +++++++++-------- .../src/internal/createSourceFactory.ts | 4 +- .../src/internal/createTargetFactory.ts | 7 +-- .../src/internal/drag-layer-connection.ts | 6 ++- 7 files changed, 97 insertions(+), 40 deletions(-) diff --git a/packages/angular-skyhook/src/connection-types.ts b/packages/angular-skyhook/src/connection-types.ts index 224f1d03..41c445bc 100644 --- a/packages/angular-skyhook/src/connection-types.ts +++ b/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 extends ISubscription { @@ -54,6 +54,32 @@ export interface ConnectionBase 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 */ diff --git a/packages/angular-skyhook/src/connector.service.ts b/packages/angular-skyhook/src/connector.service.ts index ba43c273..b0c0e17e 100644 --- a/packages/angular-skyhook/src/connector.service.ts +++ b/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); diff --git a/packages/angular-skyhook/src/index.ts b/packages/angular-skyhook/src/index.ts index c807b4b4..65d92efe 100644 --- a/packages/angular-skyhook/src/index.ts +++ b/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'; diff --git a/packages/angular-skyhook/src/internal/connection-factory.ts b/packages/angular-skyhook/src/internal/connection-factory.ts index ec5c3982..9ac8127f 100644 --- a/packages/angular-skyhook/src/internal/connection-factory.ts +++ b/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 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(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); }); } diff --git a/packages/angular-skyhook/src/internal/createTargetFactory.ts b/packages/angular-skyhook/src/internal/createTargetFactory.ts index ef120268..fe3d32ca 100644 --- a/packages/angular-skyhook/src/internal/createTargetFactory.ts +++ b/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; }); } diff --git a/packages/angular-skyhook/src/internal/drag-layer-connection.ts b/packages/angular-skyhook/src/internal/drag-layer-connection.ts index 4b4736bd..71e1edbc 100644 --- a/packages/angular-skyhook/src/internal/drag-layer-connection.ts +++ b/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; }