Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(scroll-dispatcher): avoid triggering change detection on scroll #3687

Merged
merged 1 commit into from
Mar 27, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,9 @@ describe('ConnectedPositionStrategy', () => {
fakeElementRef,
{originX: 'start', originY: 'bottom'},
{overlayX: 'start', overlayY: 'top'});
strategy.withScrollableContainers([new Scrollable(new FakeElementRef(scrollable), null)]);

strategy.withScrollableContainers([
new Scrollable(new FakeElementRef(scrollable), null, null, null)]);

positionChangeHandler = jasmine.createSpy('positionChangeHandler');
onPositionChangeSubscription = strategy.onPositionChange.subscribe(positionChangeHandler);
Expand Down
38 changes: 30 additions & 8 deletions src/lib/core/overlay/scroll/scroll-dispatcher.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,32 +41,54 @@ describe('Scroll Dispatcher', () => {

it('should notify through the directive and service that a scroll event occurred',
fakeAsync(() => {
let hasDirectiveScrollNotified = false;
// Listen for notifications from scroll directive
let scrollable = fixture.componentInstance.scrollable;
scrollable.elementScrolled().subscribe(() => { hasDirectiveScrollNotified = true; });
const scrollable = fixture.componentInstance.scrollable;
const directiveSpy = jasmine.createSpy('directive scroll callback');
scrollable.elementScrolled().subscribe(directiveSpy);

// Listen for notifications from scroll service with a throttle of 100ms
const throttleTime = 100;
let hasServiceScrollNotified = false;
scroll.scrolled(throttleTime, () => { hasServiceScrollNotified = true; });
const serviceSpy = jasmine.createSpy('service scroll callback');
scroll.scrolled(throttleTime, serviceSpy);

// Emit a scroll event from the scrolling element in our component.
// This event should be picked up by the scrollable directive and notify.
// The notification should be picked up by the service.
dispatchFakeEvent(fixture.componentInstance.scrollingElement.nativeElement, 'scroll');

// The scrollable directive should have notified the service immediately.
expect(hasDirectiveScrollNotified).toBe(true);
expect(directiveSpy).toHaveBeenCalled();

// Verify that the throttle is used, the service should wait for the throttle time until
// sending the notification.
expect(hasServiceScrollNotified).toBe(false);
expect(serviceSpy).not.toHaveBeenCalled();

// After the throttle time, the notification should be sent.
tick(throttleTime);
expect(hasServiceScrollNotified).toBe(true);
expect(serviceSpy).toHaveBeenCalled();
}));

it('should not execute the global events in the Angular zone', () => {
const spy = jasmine.createSpy('zone unstable callback');
const subscription = fixture.ngZone.onUnstable.subscribe(spy);

scroll.scrolled(0, () => {});
dispatchFakeEvent(document, 'scroll');
dispatchFakeEvent(window, 'resize');

expect(spy).not.toHaveBeenCalled();
subscription.unsubscribe();
});

it('should not execute the scrollable events in the Angular zone', () => {
const spy = jasmine.createSpy('zone unstable callback');
const subscription = fixture.ngZone.onUnstable.subscribe(spy);

dispatchFakeEvent(fixture.componentInstance.scrollingElement.nativeElement, 'scroll');

expect(spy).not.toHaveBeenCalled();
subscription.unsubscribe();
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth also adding a unit test that somehow asserts that change detection wasn't run (which is the ultimate goal)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what the test is doing: If we didn't hit the zone, we can assume that change detection didn't run. I can also rework it so it uses a ngDoCheck hook with a spy instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair; I was more thinking a test that looked at something in the DOM not being updated, but it's all really the same.

});

describe('Nested scrollables', () => {
Expand Down
21 changes: 13 additions & 8 deletions src/lib/core/overlay/scroll/scroll-dispatcher.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {Injectable, ElementRef, Optional, SkipSelf} from '@angular/core';
import {Injectable, ElementRef, Optional, SkipSelf, NgZone} from '@angular/core';
import {Scrollable} from './scrollable';
import {Subject} from 'rxjs/Subject';
import {Observable} from 'rxjs/Observable';
Expand All @@ -17,6 +17,8 @@ export const DEFAULT_SCROLL_TIME = 20;
*/
@Injectable()
export class ScrollDispatcher {
constructor(private _ngZone: NgZone) { }

/** Subject for notifying that a registered scrollable reference element has been scrolled. */
_scrolled: Subject<void> = new Subject<void>();

Expand Down Expand Up @@ -69,10 +71,12 @@ export class ScrollDispatcher {
this._scrolledCount++;

if (!this._globalSubscription) {
this._globalSubscription = Observable.merge(
Observable.fromEvent(window.document, 'scroll'),
Observable.fromEvent(window, 'resize')
).subscribe(() => this._notify());
this._globalSubscription = this._ngZone.runOutsideAngular(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment explaining why we need to escape from the Angular zone here (and in the directive)?
(I generally like to document the "why" every time we do this)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess, although the unit tests could be considered a form of documentation.

return Observable.merge(
Observable.fromEvent(window.document, 'scroll'),
Observable.fromEvent(window, 'resize')
).subscribe(() => this._notify());
});
}

// Note that we need to do the subscribing from here, in order to be able to remove
Expand Down Expand Up @@ -118,13 +122,14 @@ export class ScrollDispatcher {
}
}

export function SCROLL_DISPATCHER_PROVIDER_FACTORY(parentDispatcher: ScrollDispatcher) {
return parentDispatcher || new ScrollDispatcher();
export function SCROLL_DISPATCHER_PROVIDER_FACTORY(parentDispatcher: ScrollDispatcher,
ngZone: NgZone) {
return parentDispatcher || new ScrollDispatcher(ngZone);
}

export const SCROLL_DISPATCHER_PROVIDER = {
// If there is already a ScrollDispatcher available, use that. Otherwise, provide a new one.
provide: ScrollDispatcher,
deps: [[new Optional(), new SkipSelf(), ScrollDispatcher]],
deps: [[new Optional(), new SkipSelf(), ScrollDispatcher], NgZone],
useFactory: SCROLL_DISPATCHER_PROVIDER_FACTORY
};
23 changes: 20 additions & 3 deletions src/lib/core/overlay/scroll/scrollable.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {Directive, ElementRef, OnInit, OnDestroy} from '@angular/core';
import {Directive, ElementRef, OnInit, OnDestroy, NgZone, Renderer} from '@angular/core';
import {Observable} from 'rxjs/Observable';
import {Subject} from 'rxjs/Subject';
import {ScrollDispatcher} from './scroll-dispatcher';
import 'rxjs/add/observable/fromEvent';

Expand All @@ -13,22 +14,38 @@ import 'rxjs/add/observable/fromEvent';
selector: '[cdk-scrollable]'
})
export class Scrollable implements OnInit, OnDestroy {
private _elementScrolled: Subject<Event> = new Subject();
private _scrollListener: Function;

constructor(private _elementRef: ElementRef,
private _scroll: ScrollDispatcher) {}
private _scroll: ScrollDispatcher,
private _ngZone: NgZone,
private _renderer: Renderer) {}

ngOnInit() {
this._scrollListener = this._ngZone.runOutsideAngular(() => {
return this._renderer.listen(this.getElementRef().nativeElement, 'scroll', (event: Event) => {
this._elementScrolled.next(event);
});
});

this._scroll.register(this);
}

ngOnDestroy() {
this._scroll.deregister(this);

if (this._scrollListener) {
this._scrollListener();
this._scrollListener = null;
}
}

/**
* Returns observable that emits when a scroll event is fired on the host element.
*/
elementScrolled(): Observable<any> {
return Observable.fromEvent(this._elementRef.nativeElement, 'scroll');
return this._elementScrolled.asObservable();
}

getElementRef(): ElementRef {
Expand Down
2 changes: 1 addition & 1 deletion src/lib/core/testing/dispatch-events.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
} from './event-objects';

/** Shorthand to dispatch a fake event on a specified node. */
export function dispatchFakeEvent(node: Node, type: string) {
export function dispatchFakeEvent(node: Node | Window, type: string) {
node.dispatchEvent(createFakeEvent(type));
}

Expand Down