fix(core): improve use of breakpoint priorities (#955)

Use breakpoint priority as the only sorting/scanning mechanism;
used to ensure correct MediaChange event notifications.

* prioritize breakpoints: non-overlaps hightest, lt- lowest
  * consistently sort breakpoints ascending by priority
  * highest priority === smallest range
  * remove hackery with reverse(), etc.
* memoize BreakPointRegistry findBy lookups
* fix MatchMedia::observe() to support lazy breakpoint registration
* fix fragile logic in MediaMarshaller
* fix breakpoint registration usage
* clarify update/clear builder function callbacks
* fix MediaObserver breakpoint registration usage
* cleanup tests: excessive use of `async()`.

Fixes #648, Fixes #426.

Author: ThomasBurleson

.gitignore

@@ -17,6 +17,7 @@ node_modules
 
 # misc
 .DS_Store
+/.firebase
 /.sass-cache
 /connect.lock
 /coverage/*

src/lib/core/breakpoints/break-point-registry.spec.ts

@@ -5,7 +5,7 @@
  * Use of this source code is governed by an MIT-style license that can be
  * found in the LICENSE file at https://angular.io/license
  */
-import {TestBed, inject, async} from '@angular/core/testing';
+import {TestBed, inject} from '@angular/core/testing';
 
 import {BreakPoint} from './break-point';
 import {BreakPointRegistry} from './break-point-registry';
@@ -52,11 +52,11 @@ describe('break-points', () => {
       });
     });
 
-    it('has the custom breakpoints', async(inject([BREAKPOINTS], (list: BreakPoint[]) => {
+    it('has the custom breakpoints', inject([BREAKPOINTS], (list: BreakPoint[]) => {
       expect(list.length).toEqual(CUSTOM_BPS.length);
       expect(list[0].alias).toEqual('ab');
       expect(list[list.length - 1].suffix).toEqual('Cd');
-    })));
+    }));
   });
 
 });

src/lib/core/breakpoints/break-point-registry.ts

@@ -9,7 +9,9 @@ import {Injectable, Inject} from '@angular/core';
 
 import {BreakPoint} from './break-point';
 import {BREAKPOINTS} from './break-points-token';
+import {sortAscendingPriority} from './breakpoint-tools';
 
+type OptionalBreakPoint = BreakPoint | null;
 
 /**
  * Registry of 1..n MediaQuery breakpoint ranges
@@ -18,55 +20,36 @@ import {BREAKPOINTS} from './break-points-token';
  */
 @Injectable({providedIn: 'root'})
 export class BreakPointRegistry {
+  readonly items: BreakPoint[];
 
-  constructor(@Inject(BREAKPOINTS) private _registry: BreakPoint[]) {
-  }
-
-  /**
-   * Accessor to raw list
-   */
-  get items(): BreakPoint[] {
-    return [...this._registry];
-  }
-
-  /**
-   * Accessor to sorted list used for registration with matchMedia API
-   *
-   * NOTE: During breakpoint registration, we want to register the overlaps FIRST
-   *       so the non-overlaps will trigger the MatchMedia:BehaviorSubject last!
-   *       And the largest, non-overlap, matching breakpoint should be the lastReplay value
-   */
-  get sortedItems(): BreakPoint[] {
-    let overlaps = this._registry.filter(it => it.overlapping === true);
-    let nonOverlaps = this._registry.filter(it => it.overlapping !== true);
-
-    return [...overlaps, ...nonOverlaps];
+  constructor(@Inject(BREAKPOINTS) list: BreakPoint[]) {
+    this.items = [...list].sort(sortAscendingPriority);
   }
 
   /**
    * Search breakpoints by alias (e.g. gt-xs)
    */
-  findByAlias(alias: string): BreakPoint | null {
-    return this._registry.find(bp => bp.alias == alias) || null;
+  findByAlias(alias: string): OptionalBreakPoint {
+    return this.findWithPredicate(alias, (bp) => bp.alias == alias);
   }
 
-  findByQuery(query: string): BreakPoint | null {
-    return this._registry.find(bp => bp.mediaQuery == query) || null;
+  findByQuery(query: string): OptionalBreakPoint {
+    return this.findWithPredicate(query, (bp) => bp.mediaQuery == query);
   }
 
   /**
    * Get all the breakpoints whose ranges could overlapping `normal` ranges;
    * e.g. gt-sm overlaps md, lg, and xl
    */
   get overlappings(): BreakPoint[] {
-    return this._registry.filter(it => it.overlapping == true);
+    return this.items.filter(it => it.overlapping == true);
   }
 
   /**
    * Get list of all registered (non-empty) breakpoint aliases
    */
   get aliases(): string[] {
-    return this._registry.map(it => it.alias);
+    return this.items.map(it => it.alias);
   }
 
   /**
@@ -75,6 +58,26 @@ export class BreakPointRegistry {
    * for property layoutGtSM.
    */
   get suffixes(): string[] {
-    return this._registry.map(it => !!it.suffix ? it.suffix : '');
+    return this.items.map(it => !!it.suffix ? it.suffix : '');
   }
+
+  /**
+   * Memoized lookup using custom predicate function
+   */
+  private findWithPredicate(key: string,
+      searchFn: (bp: BreakPoint) => boolean): OptionalBreakPoint {
+    let response = this.findByMap.get(key);
+    if (!response) {
+      response = this.items.find(searchFn) || null;
+      this.findByMap.set(key, response);
+    }
+    return response || null;
+
+  }
+
+  /**
+   * Memoized BreakPoint Lookups
+   */
+  private readonly findByMap = new Map<String, OptionalBreakPoint>();
 }
+

src/lib/core/breakpoints/break-point.ts

@@ -9,7 +9,6 @@ export interface BreakPoint {
   mediaQuery: string;
   alias: string;
   suffix?: string;
-  overlapping?: boolean;
-  // The priority of the individual breakpoint when overlapping another breakpoint
-  priority?: number;
+  overlapping?: boolean;  // Does this range overlap with any other ranges
+  priority?: number;      // determine order of activation reporting: higher is last reported
 }

src/lib/core/breakpoints/breakpoint-tools.spec.ts

@@ -5,30 +5,36 @@
  * Use of this source code is governed by an MIT-style license that can be
  * found in the LICENSE file at https://angular.io/license
  */
-import {fakeAsync} from '@angular/core/testing';
-
 import {BreakPoint} from './break-point';
 import {validateSuffixes, mergeByAlias} from './breakpoint-tools';
 import {DEFAULT_BREAKPOINTS} from './data/break-points';
 import {ORIENTATION_BREAKPOINTS} from './data/orientation-break-points';
 
 describe('breakpoint-tools', () => {
-  let all: BreakPoint[];
-  let findByAlias = (alias: string): BreakPoint|null => all.reduce((pos: BreakPoint | null, it) => {
-    return pos || ((it.alias == alias) ? it : null);
-  }, null);
+  let allBreakPoints: BreakPoint[] = [];
+  const findByAlias = (alias: string): BreakPoint => {
+    let result = {mediaQuery: '', alias: ''};
+    allBreakPoints.forEach(bp => {
+      if (bp.alias === alias) {
+        result = {...bp};
+      }
+    });
+    return result;
+  };
 
   beforeEach(() => {
-    all = DEFAULT_BREAKPOINTS.concat(ORIENTATION_BREAKPOINTS);
+    allBreakPoints = validateSuffixes([...DEFAULT_BREAKPOINTS, ...ORIENTATION_BREAKPOINTS]);
   });
 
-  describe('validation', () => {
+  describe('validation ', () => {
+
     it('should not replace an existing suffix', () => {
       let validated = validateSuffixes([
         {alias: 'handset-portrait', suffix: 'Handset', mediaQuery: 'screen'}
       ]);
       expect(validated[0].suffix).toEqual('Handset');
     });
+
     it('should add valid suffixes to breakpoints', () => {
       let validated = validateSuffixes([
         {alias: 'xs', mediaQuery: 'screen and (max-width: 599px)'},
@@ -43,21 +49,19 @@ describe('breakpoint-tools', () => {
       expect(validated[3].suffix).toEqual('GtXs');
       expect(validated[4].suffix).toEqual('HandsetPortrait');
     });
-    it('should auto-validate the DEFAULT_BREAKPOINTS', () => {
-      fakeAsync(() => {
-        let xsBp: BreakPoint = findByAlias('xs')!;
-        let gtLgBp: BreakPoint = findByAlias('gt-lg')!;
-        let xlBp: BreakPoint = findByAlias('xl')!;
 
-        expect(xsBp.alias).toEqual('xs');
-        expect(xsBp.suffix).toEqual('Xs');
+    it('should auto-validate the DEFAULT_BREAKPOINTS', () => {
+      let xsBp: BreakPoint = findByAlias('xs');
+      expect(xsBp.alias).toEqual('xs');
+      expect(xsBp.suffix).toEqual('Xs');
 
-        expect(gtLgBp.alias).toEqual('gt-lg');
-        expect(gtLgBp.suffix).toEqual('GtLg');
+      let gtLgBp: BreakPoint = findByAlias('gt-lg');
+      expect(gtLgBp.alias).toEqual('gt-lg');
+      expect(gtLgBp.suffix).toEqual('GtLg');
 
-        expect(xlBp.alias).toEqual('xl');
-        expect(xlBp.suffix).toEqual('Xl');
-      });
+      let xlBp: BreakPoint = findByAlias('xl');
+      expect(xlBp.alias).toEqual('xl');
+      expect(xlBp.suffix).toEqual('Xl');
     });
   });
 
@@ -67,19 +71,19 @@ describe('breakpoint-tools', () => {
         {alias: 'sm', mediaQuery: 'screen'},
         {alias: 'md', mediaQuery: 'screen'},
       ];
-      all = mergeByAlias(defaults, custom);
+      allBreakPoints = mergeByAlias(defaults, custom);
 
-      expect(all.length).toEqual(2);
+      expect(allBreakPoints.length).toEqual(2);
       expect(findByAlias('sm')!.suffix).toEqual('Sm');
       expect(findByAlias('md')!.suffix).toEqual('Md');
     });
     it('should add custom breakpoints with unique aliases', () => {
       let defaults = [{alias: 'xs', mediaQuery: 'screen and (max-width: 599px)'}],
-        custom = [{alias: 'sm', mediaQuery: 'screen'}, {alias: 'md', mediaQuery: 'screen'}];
+          custom = [{alias: 'sm', mediaQuery: 'screen'}, {alias: 'md', mediaQuery: 'screen'}];
 
-      all = mergeByAlias(defaults, custom);
+      allBreakPoints = mergeByAlias(defaults, custom);
 
-      expect(all.length).toEqual(3);
+      expect(allBreakPoints.length).toEqual(3);
       expect(findByAlias('xs')!.suffix).toEqual('Xs');
       expect(findByAlias('sm')!.suffix).toEqual('Sm');
       expect(findByAlias('md')!.suffix).toEqual('Md');
@@ -88,9 +92,9 @@ describe('breakpoint-tools', () => {
       let defaults = [{alias: 'xs', mediaQuery: 'screen and (max-width: 599px)'}];
       let custom = [{alias: 'xs', mediaQuery: 'screen and none'}];
 
-      all = mergeByAlias(defaults, custom);
+      allBreakPoints = mergeByAlias(defaults, custom);
 
-      expect(all.length).toEqual(1);
+      expect(allBreakPoints.length).toEqual(1);
       expect(findByAlias('xs')!.suffix).toEqual('Xs');
       expect(findByAlias('xs')!.mediaQuery).toEqual('screen and none');
     });

src/lib/core/breakpoints/breakpoint-tools.ts

@@ -65,8 +65,14 @@ export function mergeByAlias(defaults: BreakPoint[], custom: BreakPoint[] = []):
 }
 
 /** HOF to sort the breakpoints by priority */
-export function prioritySort(a: BreakPoint, b: BreakPoint): number {
+export function sortDescendingPriority(a: BreakPoint, b: BreakPoint): number {
   const priorityA = a.priority || 0;
   const priorityB = b.priority || 0;
   return priorityB - priorityA;
 }
+
+export function sortAscendingPriority(a: BreakPoint, b: BreakPoint): number {
+  const pA = a.priority || 0;
+  const pB = b.priority || 0;
+  return pA - pB;
+}

src/lib/core/breakpoints/data/break-points.spec.ts

@@ -5,7 +5,8 @@
  * Use of this source code is governed by an MIT-style license that can be
  * found in the LICENSE file at https://angular.io/license
  */
-import {TestBed, inject, async} from '@angular/core/testing';
+import {TestBed, inject} from '@angular/core/testing';
+import {sortDescendingPriority} from '../breakpoint-tools';
 
 import {BreakPoint} from '../break-point';
 import {BREAKPOINTS} from '../break-points-token';
@@ -21,14 +22,14 @@ describe('break-point-provider', () => {
         providers: [{provide: BREAKPOINTS, useValue: DEFAULT_BREAKPOINTS}]
       });
     });
-    beforeEach(async(inject([BREAKPOINTS], (bps: BreakPoint[]) => {
-      breakPoints = bps;
-    })));
+    beforeEach(inject([BREAKPOINTS], (bps: BreakPoint[]) => {
+      breakPoints = [...bps].sort(sortDescendingPriority);
+    }));
 
     it('has the standard breakpoints', () => {
       expect(breakPoints.length).toEqual(DEFAULT_BREAKPOINTS.length);
       expect(breakPoints[0].alias).toEqual('xs');
-      expect(breakPoints[breakPoints.length - 1].alias).toEqual('xl');
+      expect(breakPoints[breakPoints.length - 1].alias).toEqual('gt-xs');
     });
   });
 
@@ -57,9 +58,9 @@ describe('break-point-provider', () => {
       });
     });
     // tslint:disable-next-line:no-shadowed-variable
-    beforeEach(async(inject([BREAKPOINTS], (breakPoints: BreakPoint[]) => {
+    beforeEach(inject([BREAKPOINTS], (breakPoints: BreakPoint[]) => {
       bpList = breakPoints;
-    })));
+    }));
 
     it('has the custom breakpoints', () => {
       expect(bpList.length).toEqual(CUSTOM_BPS.length);