From 3fe7dbf741fe94c07fe7a7e14c802bbe6d699c89 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Thu, 1 Jun 2017 20:41:19 +0200 Subject: [PATCH] refactor: switch to new approach without ReflectiveInjector --- .../block-scroll-strategy-e2e.ts | 6 +- src/lib/core/overlay/overlay-ref.ts | 7 +- src/lib/core/overlay/overlay.spec.ts | 94 ++++++++++++------- src/lib/core/overlay/overlay.ts | 54 ++--------- .../scroll/block-scroll-strategy.spec.ts | 1 - .../overlay/scroll/block-scroll-strategy.ts | 2 - .../scroll/close-scroll-strategy.spec.ts | 1 - .../overlay/scroll/close-scroll-strategy.ts | 2 - src/lib/core/overlay/scroll/index.ts | 4 +- .../overlay/scroll/noop-scroll-strategy.ts | 2 - .../scroll/reposition-scroll-strategy.spec.ts | 1 - .../scroll/reposition-scroll-strategy.ts | 2 - .../overlay/scroll/scroll-strategy-options.ts | 31 ++++++ .../core/overlay/scroll/scroll-strategy.md | 47 ++++++++-- 14 files changed, 146 insertions(+), 108 deletions(-) create mode 100644 src/lib/core/overlay/scroll/scroll-strategy-options.ts diff --git a/src/e2e-app/block-scroll-strategy/block-scroll-strategy-e2e.ts b/src/e2e-app/block-scroll-strategy/block-scroll-strategy-e2e.ts index a9e8f4b7e3e3..8a247c6331b2 100644 --- a/src/e2e-app/block-scroll-strategy/block-scroll-strategy-e2e.ts +++ b/src/e2e-app/block-scroll-strategy/block-scroll-strategy-e2e.ts @@ -1,5 +1,5 @@ import {Component} from '@angular/core'; -import {BlockScrollStrategy, ViewportRuler} from '@angular/material'; +import {ScrollStrategyOptions, ScrollStrategy} from '@angular/material'; @Component({ moduleId: module.id, @@ -8,6 +8,6 @@ import {BlockScrollStrategy, ViewportRuler} from '@angular/material'; styleUrls: ['block-scroll-strategy-e2e.css'], }) export class BlockScrollStrategyE2E { - constructor(private _viewportRuler: ViewportRuler) { } - scrollStrategy = new BlockScrollStrategy(this._viewportRuler); + constructor(private _scrollStrategyOptions: ScrollStrategyOptions) { } + scrollStrategy: ScrollStrategy = this._scrollStrategyOptions.get('block'); } diff --git a/src/lib/core/overlay/overlay-ref.ts b/src/lib/core/overlay/overlay-ref.ts index 760a53b94630..7e0953047fa1 100644 --- a/src/lib/core/overlay/overlay-ref.ts +++ b/src/lib/core/overlay/overlay-ref.ts @@ -23,8 +23,11 @@ export class OverlayRef implements PortalHost { private _scrollStrategy: ScrollStrategy, private _ngZone: NgZone) { - _scrollStrategy.attach(this, - typeof _state.scrollStrategy === 'string' ? null : _state.scrollStrategy.config); + let scrollStrategyConfig = typeof _state.scrollStrategy === 'string' ? + null : + _state.scrollStrategy.config; + + _scrollStrategy.attach(this, scrollStrategyConfig); } /** The overlay's HTML element */ diff --git a/src/lib/core/overlay/overlay.spec.ts b/src/lib/core/overlay/overlay.spec.ts index ddf460d0e5c3..74800054534c 100644 --- a/src/lib/core/overlay/overlay.spec.ts +++ b/src/lib/core/overlay/overlay.spec.ts @@ -8,7 +8,8 @@ import {OverlayState} from './overlay-state'; import {OverlayRef} from './overlay-ref'; import {PositionStrategy} from './position/position-strategy'; import {OverlayModule} from './overlay-directives'; -import {ScrollStrategy} from './scroll/scroll-strategy'; +import {ScrollStrategy, ScrollStrategyOptions, ScrollDispatcher} from './scroll/index'; +import {ViewportRuler} from './position/viewport-ruler'; describe('Overlay', () => { @@ -22,10 +23,20 @@ describe('Overlay', () => { TestBed.configureTestingModule({ imports: [OverlayModule, PortalModule, OverlayTestModule], providers: [ - {provide: OverlayContainer, useFactory: () => { - overlayContainerElement = document.createElement('div'); - return {getContainerElement: () => overlayContainerElement}; - }} + ScrollDispatcher, + ViewportRuler, + { + provide: ScrollStrategyOptions, + useClass: ScrollStrategyOptionsOverride, + deps: [ScrollDispatcher, ViewportRuler] + }, + { + provide: OverlayContainer, + useFactory: () => { + overlayContainerElement = document.createElement('div'); + return {getContainerElement: () => overlayContainerElement}; + } + } ] }).compileComponents(); })); @@ -337,51 +348,30 @@ describe('Overlay', () => { describe('scroll strategy', () => { let fakeScrollStrategy: FakeScrollStrategy; let config: OverlayState; - - class FakeScrollStrategy implements ScrollStrategy { - isEnabled = false; - overlayRef: OverlayRef; - - constructor() { - fakeScrollStrategy = this; - } - - attach(overlayRef: OverlayRef) { - this.overlayRef = overlayRef; - } - - enable() { - this.isEnabled = true; - } - - disable() { - this.isEnabled = false; - } - } + let overlayRef: OverlayRef; beforeEach(() => { + }); + + beforeEach(inject([ScrollStrategyOptions], (scrollOptions: ScrollStrategyOptionsOverride) => { config = new OverlayState(); - overlay.registerScrollStrategy('fake', FakeScrollStrategy); config.scrollStrategy = 'fake'; - }); + overlayRef = overlay.create(config); + fakeScrollStrategy = + scrollOptions.instances[scrollOptions.instances.length - 1] as FakeScrollStrategy; + })); it('should attach the overlay ref to the scroll strategy', () => { - let overlayRef = overlay.create(config); - expect(fakeScrollStrategy.overlayRef).toBe(overlayRef, 'Expected scroll strategy to have been attached to the current overlay ref.'); }); it('should enable the scroll strategy when the overlay is attached', () => { - let overlayRef = overlay.create(config); - overlayRef.attach(componentPortal); expect(fakeScrollStrategy.isEnabled).toBe(true, 'Expected scroll strategy to be enabled.'); }); it('should disable the scroll strategy once the overlay is detached', () => { - let overlayRef = overlay.create(config); - overlayRef.attach(componentPortal); expect(fakeScrollStrategy.isEnabled).toBe(true, 'Expected scroll strategy to be enabled.'); @@ -390,8 +380,6 @@ describe('Overlay', () => { }); it('should disable the scroll strategy when the overlay is destroyed', () => { - let overlayRef = overlay.create(config); - overlayRef.dispose(); expect(fakeScrollStrategy.isEnabled).toBe(false, 'Expected scroll strategy to be disabled.'); }); @@ -469,3 +457,37 @@ class FakePositionStrategy implements PositionStrategy { dispose() {} } + + +class FakeScrollStrategy implements ScrollStrategy { + isEnabled = false; + overlayRef: OverlayRef; + + attach(overlayRef: OverlayRef) { + this.overlayRef = overlayRef; + } + + enable() { + this.isEnabled = true; + } + + disable() { + this.isEnabled = false; + } +} + + +class ScrollStrategyOptionsOverride extends ScrollStrategyOptions { + constructor(scrollDispatcher: ScrollDispatcher, viewportRuler: ViewportRuler) { + super(scrollDispatcher, viewportRuler); + } + + // used for accessing the current instance in unit tests. + public instances: ScrollStrategy[] = []; + + get(strategy: string): ScrollStrategy { + let instance = strategy === 'fake' ? new FakeScrollStrategy() : super.get(strategy); + this.instances.push(instance); + return instance; + } +} diff --git a/src/lib/core/overlay/overlay.ts b/src/lib/core/overlay/overlay.ts index 067a30b2d1d1..55b21e922281 100644 --- a/src/lib/core/overlay/overlay.ts +++ b/src/lib/core/overlay/overlay.ts @@ -5,7 +5,6 @@ import { Injector, NgZone, Provider, - ReflectiveInjector, } from '@angular/core'; import {OverlayState} from './overlay-state'; import {DomPortalHost} from '../portal/dom-portal-host'; @@ -13,11 +12,7 @@ import {OverlayRef} from './overlay-ref'; import {OverlayPositionBuilder} from './position/overlay-position-builder'; import {VIEWPORT_RULER_PROVIDER} from './position/viewport-ruler'; import {OverlayContainer, OVERLAY_CONTAINER_PROVIDER} from './overlay-container'; -import {ScrollStrategy} from './scroll/scroll-strategy'; -import {RepositionScrollStrategy} from './scroll/reposition-scroll-strategy'; -import {BlockScrollStrategy} from './scroll/block-scroll-strategy'; -import {CloseScrollStrategy} from './scroll/close-scroll-strategy'; -import {NoopScrollStrategy} from './scroll/noop-scroll-strategy'; +import {ScrollStrategy, ScrollStrategyOptions} from './scroll/index'; /** Next overlay unique ID. */ @@ -37,19 +32,10 @@ let defaultState = new OverlayState(); */ @Injectable() export class Overlay { - // Create a child ReflectiveInjector, allowing us to instantiate scroll - // strategies without going throught the injector cache. - private _reflectiveInjector = ReflectiveInjector.resolveAndCreate([], this._injector); - private _scrollStrategies = { - reposition: RepositionScrollStrategy, - block: BlockScrollStrategy, - close: CloseScrollStrategy, - noop: NoopScrollStrategy - }; - constructor(private _overlayContainer: OverlayContainer, private _componentFactoryResolver: ComponentFactoryResolver, private _positionBuilder: OverlayPositionBuilder, + private _scrollStrategyOptions: ScrollStrategyOptions, private _appRef: ApplicationRef, private _injector: Injector, private _ngZone: NgZone) { } @@ -71,17 +57,6 @@ export class Overlay { return this._positionBuilder; } - /** - * Registers a scroll strategy to be available for use when creating an overlay. - * @param name Name of the scroll strategy. - * @param constructor Class to be used to instantiate the scroll strategy. - */ - registerScrollStrategy(name: string, constructor: Function): void { - if (name && constructor) { - this._scrollStrategies[name] = constructor; - } - } - /** * Creates the DOM element for an overlay and appends it to the overlay container. * @returns Newly-created pane element @@ -111,29 +86,14 @@ export class Overlay { * @param state */ private _createOverlayRef(pane: HTMLElement, state: OverlayState): OverlayRef { + let scrollStrategyName = typeof state.scrollStrategy === 'string' ? + state.scrollStrategy : + state.scrollStrategy.name; + + let scrollStrategy = this._scrollStrategyOptions.get(scrollStrategyName); let portalHost = this._createPortalHost(pane); - let scrollStrategy = this._createScrollStrategy(state); return new OverlayRef(portalHost, pane, state, scrollStrategy, this._ngZone); } - - /** - * Resolves the scroll strategy of an overlay state. - * @param state State for which to resolve the scroll strategy. - */ - private _createScrollStrategy(state: OverlayState): ScrollStrategy { - let strategyName = typeof state.scrollStrategy === 'string' ? - state.scrollStrategy : - state.scrollStrategy.name; - - if (!this._scrollStrategies.hasOwnProperty(strategyName)) { - throw new Error(`Unsupported scroll strategy "${strategyName}". The available scroll ` + - `strategies are ${Object.keys(this._scrollStrategies).join(', ')}.`); - } - - // Note that we use `resolveAndInstantiate` which will instantiate - // the scroll strategy without putting it in the injector cache. - return this._reflectiveInjector.resolveAndInstantiate(this._scrollStrategies[strategyName]); - } } /** Providers for Overlay and its related injectables. */ diff --git a/src/lib/core/overlay/scroll/block-scroll-strategy.spec.ts b/src/lib/core/overlay/scroll/block-scroll-strategy.spec.ts index 2ab9832e8009..895fbab5a797 100644 --- a/src/lib/core/overlay/scroll/block-scroll-strategy.spec.ts +++ b/src/lib/core/overlay/scroll/block-scroll-strategy.spec.ts @@ -4,7 +4,6 @@ import { ComponentPortal, OverlayModule, PortalModule, - BlockScrollStrategy, Platform, ViewportRuler, OverlayState, diff --git a/src/lib/core/overlay/scroll/block-scroll-strategy.ts b/src/lib/core/overlay/scroll/block-scroll-strategy.ts index c9aab0660a87..438db2efb560 100644 --- a/src/lib/core/overlay/scroll/block-scroll-strategy.ts +++ b/src/lib/core/overlay/scroll/block-scroll-strategy.ts @@ -1,11 +1,9 @@ -import {Injectable} from '@angular/core'; import {ScrollStrategy} from './scroll-strategy'; import {ViewportRuler} from '../position/viewport-ruler'; /** * Strategy that will prevent the user from scrolling while the overlay is visible. */ -@Injectable() export class BlockScrollStrategy implements ScrollStrategy { private _previousHTMLStyles = { top: null, left: null }; private _previousScrollPosition: { top: number, left: number }; diff --git a/src/lib/core/overlay/scroll/close-scroll-strategy.spec.ts b/src/lib/core/overlay/scroll/close-scroll-strategy.spec.ts index 8d87b512121b..ce4aa1074dd3 100644 --- a/src/lib/core/overlay/scroll/close-scroll-strategy.spec.ts +++ b/src/lib/core/overlay/scroll/close-scroll-strategy.spec.ts @@ -10,7 +10,6 @@ import { OverlayModule, ScrollStrategy, ScrollDispatcher, - CloseScrollStrategy, } from '../../core'; diff --git a/src/lib/core/overlay/scroll/close-scroll-strategy.ts b/src/lib/core/overlay/scroll/close-scroll-strategy.ts index fafeea4c97b0..7caca425cde3 100644 --- a/src/lib/core/overlay/scroll/close-scroll-strategy.ts +++ b/src/lib/core/overlay/scroll/close-scroll-strategy.ts @@ -1,4 +1,3 @@ -import {Injectable} from '@angular/core'; import {ScrollStrategy, getMdScrollStrategyAlreadyAttachedError} from './scroll-strategy'; import {OverlayRef} from '../overlay-ref'; import {Subscription} from 'rxjs/Subscription'; @@ -8,7 +7,6 @@ import {ScrollDispatcher} from './scroll-dispatcher'; /** * Strategy that will close the overlay as soon as the user starts scrolling. */ -@Injectable() export class CloseScrollStrategy implements ScrollStrategy { private _scrollSubscription: Subscription|null = null; private _overlayRef: OverlayRef; diff --git a/src/lib/core/overlay/scroll/index.ts b/src/lib/core/overlay/scroll/index.ts index ae291c6d5b9a..acf5039b6188 100644 --- a/src/lib/core/overlay/scroll/index.ts +++ b/src/lib/core/overlay/scroll/index.ts @@ -2,12 +2,14 @@ import {NgModule} from '@angular/core'; import {SCROLL_DISPATCHER_PROVIDER} from './scroll-dispatcher'; import {Scrollable} from './scrollable'; import {PlatformModule} from '../../platform/index'; +import {ScrollStrategyOptions} from './scroll-strategy-options'; export {Scrollable} from './scrollable'; export {ScrollDispatcher} from './scroll-dispatcher'; // Export pre-defined scroll strategies and interface to build custom ones. export {ScrollStrategy} from './scroll-strategy'; +export {ScrollStrategyOptions} from './scroll-strategy-options'; export {RepositionScrollStrategy} from './reposition-scroll-strategy'; export {CloseScrollStrategy} from './close-scroll-strategy'; export {NoopScrollStrategy} from './noop-scroll-strategy'; @@ -17,6 +19,6 @@ export {BlockScrollStrategy} from './block-scroll-strategy'; imports: [PlatformModule], exports: [Scrollable], declarations: [Scrollable], - providers: [SCROLL_DISPATCHER_PROVIDER], + providers: [SCROLL_DISPATCHER_PROVIDER, ScrollStrategyOptions], }) export class ScrollDispatchModule { } diff --git a/src/lib/core/overlay/scroll/noop-scroll-strategy.ts b/src/lib/core/overlay/scroll/noop-scroll-strategy.ts index 7f795f3532c0..3d50a8f7743a 100644 --- a/src/lib/core/overlay/scroll/noop-scroll-strategy.ts +++ b/src/lib/core/overlay/scroll/noop-scroll-strategy.ts @@ -1,10 +1,8 @@ -import {Injectable} from '@angular/core'; import {ScrollStrategy} from './scroll-strategy'; /** * Scroll strategy that doesn't do anything. */ -@Injectable() export class NoopScrollStrategy implements ScrollStrategy { enable() { } disable() { } diff --git a/src/lib/core/overlay/scroll/reposition-scroll-strategy.spec.ts b/src/lib/core/overlay/scroll/reposition-scroll-strategy.spec.ts index b8c4be1a2490..9591b3e9dc07 100644 --- a/src/lib/core/overlay/scroll/reposition-scroll-strategy.spec.ts +++ b/src/lib/core/overlay/scroll/reposition-scroll-strategy.spec.ts @@ -10,7 +10,6 @@ import { OverlayModule, ScrollStrategy, ScrollDispatcher, - RepositionScrollStrategy, } from '../../core'; diff --git a/src/lib/core/overlay/scroll/reposition-scroll-strategy.ts b/src/lib/core/overlay/scroll/reposition-scroll-strategy.ts index 9bb90db73870..c5107c333b72 100644 --- a/src/lib/core/overlay/scroll/reposition-scroll-strategy.ts +++ b/src/lib/core/overlay/scroll/reposition-scroll-strategy.ts @@ -1,4 +1,3 @@ -import {Injectable} from '@angular/core'; import {Subscription} from 'rxjs/Subscription'; import {ScrollStrategy, getMdScrollStrategyAlreadyAttachedError} from './scroll-strategy'; import {OverlayRef} from '../overlay-ref'; @@ -14,7 +13,6 @@ export interface RepositionScrollStrategyConfig { /** * Strategy that will update the element position as the user is scrolling. */ -@Injectable() export class RepositionScrollStrategy implements ScrollStrategy { private _scrollSubscription: Subscription|null = null; private _overlayRef: OverlayRef; diff --git a/src/lib/core/overlay/scroll/scroll-strategy-options.ts b/src/lib/core/overlay/scroll/scroll-strategy-options.ts new file mode 100644 index 000000000000..caac29a7ec87 --- /dev/null +++ b/src/lib/core/overlay/scroll/scroll-strategy-options.ts @@ -0,0 +1,31 @@ +import {Injectable} from '@angular/core'; +import {ScrollStrategy} from './scroll-strategy'; +import {RepositionScrollStrategy} from './reposition-scroll-strategy'; +import {CloseScrollStrategy} from './close-scroll-strategy'; +import {NoopScrollStrategy} from './noop-scroll-strategy'; +import {BlockScrollStrategy} from './block-scroll-strategy'; +import {ScrollDispatcher} from './scroll-dispatcher'; +import {ViewportRuler} from '../position/viewport-ruler'; + + +/** + * Factory that instantiates scroll strategies. Provides the built-in `reposition`, `close`, + * `noop` and `block` strategies by default. + */ +@Injectable() +export class ScrollStrategyOptions { + constructor( + private _scrollDispatcher: ScrollDispatcher, + private _viewportRuler: ViewportRuler) { } + + get(strategy: string): ScrollStrategy { + switch (strategy) { + case 'reposition': return new RepositionScrollStrategy(this._scrollDispatcher); + case 'close': return new CloseScrollStrategy(this._scrollDispatcher); + case 'noop': return new NoopScrollStrategy(); + case 'block': return new BlockScrollStrategy(this._viewportRuler); + } + + throw new Error(`Unsupported scroll strategy "${strategy}".`); + } +} diff --git a/src/lib/core/overlay/scroll/scroll-strategy.md b/src/lib/core/overlay/scroll/scroll-strategy.md index 003aa61c3fb6..ab359cf015a8 100644 --- a/src/lib/core/overlay/scroll/scroll-strategy.md +++ b/src/lib/core/overlay/scroll/scroll-strategy.md @@ -26,16 +26,47 @@ interface. There are three stages of a scroll strategy's life cycle: 3. When an overlay is detached from the DOM or destroyed, it'll call the `disable` method on its scroll strategy, allowing it to clean up after itself. -Afterwards the scroll strategy has to be registered with the `Overlay` service: +Afterwards you have to override the `ScrollStrategyOptions` provider, which is used to instantiate +the scroll strategies and to handle the dependency injection. ```ts -overlay.registerScrollStrategy('custom', CustomScrollStrategy); -``` +import {NgModule} from '@angular/core'; +import { + ScrollStrategy, + ScrollStrategyOptions, + ScrollDispatcher, + ViewportRuler, +} from '@angular/material'; -Finally, you can use the strategy by passing its name to the `OverlayState`: -```ts -let overlayState = new OverlayState(); +// Your custom scroll strategy. +export class CustomScrollStrategy implements ScrollStrategy { + // your implementation +} -overlayState.scrollStrategy = 'custom'; -this._overlay.create(overlayState).attach(yourPortal); +// Provider that'll instantiate your custom strategies, as well as the built-in ones from Material. +class ScrollStrategyOptionsOverride extends ScrollStrategyOptions { + constructor(scrollDispatcher: ScrollDispatcher, viewportRuler: ViewportRuler) { + super(scrollDispatcher, viewportRuler); + } + + get(strategy: string): ScrollStrategy { + if (strategy === 'custom') { + return new CustomScrollStrategy(); + } + + return super.get(strategy); + } +} + +// Register the provider with your module. +@NgModule({ + providers: [ + { + provide: ScrollStrategyOptions, + useClass: ScrollStrategyOptionsOverride, + deps: [ScrollDispatcher, ViewportRuler] + } + ] +}) +export class YourModule { } ```