Merge pull request #379 from bigopon/fix-matcher

fix(repeat): properly extract matcher binding

Author: EisenbergEffect

src/array-repeat-strategy.ts

@@ -1,5 +1,6 @@
 import {createFullOverrideContext, updateOverrideContexts, updateOverrideContext, indexOf} from './repeat-utilities';
 import {mergeSplice} from 'aurelia-binding';
+import { Repeat } from './repeat';
 
 /**
  * A strategy for repeating a template over an array.
@@ -20,29 +21,30 @@ export class ArrayRepeatStrategy {
    * @param items The new array instance.
    */
   instanceChanged(repeat, items) {
+    const $repeat = repeat as Repeat;
     const itemsLength = items.length;
 
     // if the new instance does not contain any items,
     // just remove all views and don't do any further processing
     if (!items || itemsLength === 0) {
-      repeat.removeAllViews(true, !repeat.viewsRequireLifecycle);
+      $repeat.removeAllViews(true, !$repeat.viewsRequireLifecycle);
       return;
     }
 
-    const children = repeat.views();
+    const children = $repeat.views();
     const viewsLength = children.length;
 
     // likewise, if we previously didn't have any views,
     // simply make them and return
     if (viewsLength === 0) {
-      this._standardProcessInstanceChanged(repeat, items);
+      this._standardProcessInstanceChanged($repeat, items);
       return;
     }
 
-    if (repeat.viewsRequireLifecycle) {
+    if ($repeat.viewsRequireLifecycle) {
       const childrenSnapshot = children.slice(0);
-      const itemNameInBindingContext = repeat.local;
-      const matcher = repeat.matcher();
+      const itemNameInBindingContext = $repeat.local;
+      const matcher = $repeat.matcher();
 
       // the cache of the current state (it will be transformed along with the views to keep track of indicies)
       let itemsPreviouslyInViews = [];
@@ -65,7 +67,7 @@ export class ArrayRepeatStrategy {
       let removePromise;
 
       if (itemsPreviouslyInViews.length > 0) {
-        removePromise = repeat.removeViews(viewsToRemove, true, !repeat.viewsRequireLifecycle);
+        removePromise = $repeat.removeViews(viewsToRemove, true, !$repeat.viewsRequireLifecycle);
         updateViews = () => {
           // update views (create new and move existing)
           for (let index = 0; index < itemsLength; index++) {
@@ -74,16 +76,16 @@ export class ArrayRepeatStrategy {
             let view;
 
             if (indexOfView === -1) { // create views for new items
-              const overrideContext = createFullOverrideContext(repeat, items[index], index, itemsLength);
-              repeat.insertView(index, overrideContext.bindingContext, overrideContext);
+              const overrideContext = createFullOverrideContext($repeat, items[index], index, itemsLength);
+              $repeat.insertView(index, overrideContext.bindingContext, overrideContext);
               // reflect the change in our cache list so indicies are valid
               itemsPreviouslyInViews.splice(index, 0, undefined);
             } else if (indexOfView === index) { // leave unchanged items
               view = children[indexOfView];
               itemsPreviouslyInViews[indexOfView] = undefined;
             } else { // move the element to the right place
               view = children[indexOfView];
-              repeat.moveView(indexOfView, index);
+              $repeat.moveView(indexOfView, index);
               itemsPreviouslyInViews.splice(indexOfView, 1);
               itemsPreviouslyInViews.splice(index, 0, undefined);
             }
@@ -95,12 +97,12 @@ export class ArrayRepeatStrategy {
 
           // remove extraneous elements in case of duplicates,
           // also update binding contexts if objects changed using the matcher function
-          this._inPlaceProcessItems(repeat, items);
+          this._inPlaceProcessItems($repeat, items);
         };
       } else {
         // if all of the items are different, remove all and add all from scratch
-        removePromise = repeat.removeAllViews(true, !repeat.viewsRequireLifecycle);
-        updateViews = () => this._standardProcessInstanceChanged(repeat, items);
+        removePromise = $repeat.removeAllViews(true, !$repeat.viewsRequireLifecycle);
+        updateViews = () => this._standardProcessInstanceChanged($repeat, items);
       }
 
       if (removePromise instanceof Promise) {
@@ -110,7 +112,7 @@ export class ArrayRepeatStrategy {
       }
     } else {
       // no lifecycle needed, use the fast in-place processing
-      this._inPlaceProcessItems(repeat, items);
+      this._inPlaceProcessItems($repeat, items);
     }
   }
 

src/aurelia-templating-resources.ts

@@ -37,6 +37,7 @@ import {
 } from './repeat-utilities';
 import {viewsRequireLifecycle} from './analyze-view-factory';
 import {injectAureliaHideStyleAtHead} from './aurelia-hide-style';
+import './interfaces';
 
 function configure(config: any) {
   injectAureliaHideStyleAtHead();

src/interfaces.ts

@@ -0,0 +1,17 @@
+import { BindingExpression } from 'aurelia-binding';
+import { TargetInstruction } from 'aurelia-templating';
+
+/**@internal */
+declare module 'aurelia-templating' {
+  interface ViewFactory {
+    instructions: Record<string, TargetInstruction>;
+    template: DocumentFragment;
+  }
+}
+
+/**@internal */
+declare module 'aurelia-binding' {
+  interface BindingExpression {
+    targetProperty: string;
+  }
+}

src/repeat.ts

@@ -1,6 +1,6 @@
 /*eslint no-loop-func:0, no-unused-vars:0*/
 import {inject} from 'aurelia-dependency-injection';
-import {ObserverLocator} from 'aurelia-binding';
+import {ObserverLocator, BindingExpression} from 'aurelia-binding';
 import {
   BoundViewFactory,
   TargetInstruction,
@@ -9,7 +9,8 @@ import {
   customAttribute,
   bindable,
   templateController,
-  View
+  View,
+  ViewFactory
 } from 'aurelia-templating';
 import {RepeatStrategyLocator} from './repeat-strategy-locator';
 import {
@@ -28,6 +29,16 @@ import {AbstractRepeater} from './abstract-repeater';
 @templateController
 @inject(BoundViewFactory, TargetInstruction, ViewSlot, ViewResources, ObserverLocator, RepeatStrategyLocator)
 export class Repeat extends AbstractRepeater {
+
+  /**
+   * Setting this to `true` to enable legacy behavior, where a repeat would take first `matcher` binding
+   * any where inside its view if there's no `matcher` binding on the repeated element itself.
+   *
+   * Default value is true to avoid breaking change
+   * @default true
+   */
+  static useInnerMatcher = true;
+
   /**
    * List of items to bind the repeater to.
    *
@@ -259,24 +270,34 @@ export class Repeat extends AbstractRepeater {
   }
 
   /**
+   * Capture and remove matcher binding is a way to cache matcher binding + reduce redundant work
+   * caused by multiple unnecessary matcher bindings
    * @internal
    */
   _captureAndRemoveMatcherBinding() {
-    if (this.viewFactory.viewFactory) {
-      const instructions = this.viewFactory.viewFactory.instructions;
-      const instructionIds = Object.keys(instructions);
-      for (let i = 0; i < instructionIds.length; i++) {
-        const expressions = instructions[instructionIds[i]].expressions;
-        if (expressions) {
-          for (let ii = 0; ii < expressions.length; ii++) {
-            if (expressions[ii].targetProperty === 'matcher') {
-              const matcherBinding = expressions[ii];
-              expressions.splice(ii, 1);
-              return matcherBinding;
-            }
-          }
-        }
+    const viewFactory: ViewFactory = this.viewFactory.viewFactory;
+    if (viewFactory) {
+      const template = viewFactory.template;
+      const instructions = viewFactory.instructions;
+      // legacy behavior enabled when Repeat.useInnerMathcer === true
+      if (Repeat.useInnerMatcher) {
+        return extractMatcherBindingExpression(instructions);
+      }
+      // if the template has more than 1 immediate child element
+      // it's a repeat put on a <template/> element
+      // not valid for matcher binding
+      if (template.children.length > 1) {
+        return undefined;
       }
+      // if the root element does not have any instruction
+      // it means there's no matcher binding
+      // no need to do any further work
+      const repeatedElement = template.firstElementChild;
+      if (!repeatedElement.hasAttribute('au-target-id')) {
+        return undefined;
+      }
+      const repeatedElementTargetId = repeatedElement.getAttribute('au-target-id');
+      return extractMatcherBindingExpression(instructions, repeatedElementTargetId);
     }
 
     return undefined;
@@ -286,7 +307,12 @@ export class Repeat extends AbstractRepeater {
   viewCount() { return this.viewSlot.children.length; }
   views() { return this.viewSlot.children; }
   view(index) { return this.viewSlot.children[index]; }
-  matcher() { return this.matcherBinding ? this.matcherBinding.sourceExpression.evaluate(this.scope, this.matcherBinding.lookupFunctions) : null; }
+  matcher() {
+    const matcherBinding = this.matcherBinding;
+    return matcherBinding
+      ? matcherBinding.sourceExpression.evaluate(this.scope, matcherBinding.lookupFunctions)
+      : null;
+  }
 
   addView(bindingContext, overrideContext) {
     let view = this.viewFactory.create();
@@ -332,3 +358,28 @@ export class Repeat extends AbstractRepeater {
     }
   }
 }
+
+/**
+ * Iterate a record of TargetInstruction and their expressions to find first binding expression that targets property named "matcher"
+ */
+const extractMatcherBindingExpression = (instructions: Record<string, TargetInstruction>, targetedElementId?: string): BindingExpression | undefined => {
+  const instructionIds = Object.keys(instructions);
+  for (let i = 0; i < instructionIds.length; i++) {
+    const instructionId = instructionIds[i];
+    // matcher binding can only works when root element is not a <template/>
+    // checking first el child
+    if (targetedElementId !== undefined && instructionId !== targetedElementId) {
+      continue;
+    }
+    const expressions = instructions[instructionId].expressions as BindingExpression[];
+    if (expressions) {
+      for (let ii = 0; ii < expressions.length; ii++) {
+        if (expressions[ii].targetProperty === 'matcher') {
+          const matcherBindingExpression = expressions[ii];
+          expressions.splice(ii, 1);
+          return matcherBindingExpression;
+        }
+      }
+    }
+  }
+};

test/map-repeat-strategy.spec.ts

@@ -45,57 +45,64 @@ describe('MapRepeatStrategy', () => {
       view3.bindingContext = { item: ['john', 'doe'] };
       view3.overrideContext = {};
       viewSlot.children = [view1, view2, view3];
-      viewFactorySpy = spyOn(viewFactory, 'create').and.callFake(() => {});
+      viewFactorySpy = spyOn(viewFactory, 'create').and.callFake(() => {/**/});
     });
-  
+
     it('should correctly handle adding item (i.e Map.prototype.set())', () => {
-      repeat = new Repeat(new ViewFactoryMock(), instructionMock, viewSlot, viewResourcesMock, new ObserverLocator());
+      repeat = new Repeat(
+        new ViewFactoryMock(),
+        instructionMock,
+        viewSlot,
+        viewResourcesMock,
+        new ObserverLocator(),
+        null
+      );
       let bindingContext = {};
       repeat.scope = { bindingContext, overrideContext: createOverrideContext(bindingContext) };
       records = [
-        {"type": "add", "object": {}, "key": 'norf'}
-      ]
+        {'type': 'add', 'object': {}, 'key': 'norf'}
+      ];
       items = new Map([['foo', 'bar'], ['qux', 'qax'], ['john', 'doe'], ['norf', 'narf']]);
-      spyOn(viewSlot, 'removeAt').and.callFake(() => { return new ViewMock();});
+      spyOn(viewSlot, 'removeAt').and.callFake(() => { return new ViewMock(); });
       strategy.instanceMutated(repeat, items, records);
-    
+
       expect(viewSlot.children.length).toBe(4);
       expect(viewSlot.children[3].bindingContext.key).toBe('norf');
       expect(viewSlot.children[3].overrideContext.$index).toBe(3);
       expect(viewSlot.children[3].overrideContext.$first).toBe(false);
       expect(viewSlot.children[3].overrideContext.$last).toBe(true);
     });
-  
+
     it('should correctly handle clear items (i.e Map.prototype.clear())', () => {
       let view4 = new ViewMock();
       view4.bindingContext = { item: ['norf', 'narf'] };
       view4.overrideContext = {};
       let viewSlotMock = new ViewSlotMock();
       viewSlotMock.children = [view1, view2, view3, view4];
-      repeat = new Repeat(new ViewFactoryMock(), instructionMock, viewSlotMock, viewResourcesMock, new ObserverLocator());
+      repeat = new Repeat(new ViewFactoryMock(), instructionMock, viewSlotMock, viewResourcesMock, new ObserverLocator(), null);
       let bindingContext = {};
       repeat.scope = { bindingContext, overrideContext: createOverrideContext(bindingContext) };
       records = [
-        {"type": "clear", "object": {}}
-      ]
+        {'type': 'clear', 'object': {}}
+      ];
       items = new Map();
       strategy.instanceMutated(repeat, items, records);
-    
+
       expect(viewSlotMock.children.length).toBe(0);
     });
-  
+
     it('should correctly handle adding items after clear (issue 287)', () => {
       viewSlot.children = [view1, view2, view3];
-      repeat = new Repeat(new ViewFactoryMock(), instructionMock, viewSlot, viewResourcesMock, new ObserverLocator());
+      repeat = new Repeat(new ViewFactoryMock(), instructionMock, viewSlot, viewResourcesMock, new ObserverLocator(), null);
       let bindingContext = {};
       repeat.scope = { bindingContext, overrideContext: createOverrideContext(bindingContext) };
       records = [
-        {"type": "clear", "object": {}},
-        {"type": "add", "object": {}, "key": 'foo'},
-        {"type": "add", "object": {}, "key": 'qux'},
-        {"type": "add", "object": {}, "key": 'john'},
-        {"type": "add", "object": {}, "key": 'norf'}
-      ]
+        {'type': 'clear', 'object': {}},
+        {'type': 'add', 'object': {}, 'key': 'foo'},
+        {'type': 'add', 'object': {}, 'key': 'qux'},
+        {'type': 'add', 'object': {}, 'key': 'john'},
+        {'type': 'add', 'object': {}, 'key': 'norf'}
+      ];
       items = new Map([['foo', 'bar'], ['qux', 'qax'], ['john', 'doe'], ['norf', 'narf']]);
       strategy.instanceMutated(repeat, items, records);