Repeater / templating / tests cleanup (#203)

* chore(tests): add utility for getting all property descriptors from a prototype chain

* test(create-element): add tests for creating PotentialRenderable based on tag

* test(create-element): add tests for creating PotentialRenderable based on type

* feat(potential-renderable): autodetect when build is required

* refactor(repeat): use templateController decorator and clean everything up

* chore(repeat): add default type

* test(repeat): start reworking the repeater tests

* wip

* fix(repeat): properly hook into attach/detach lifecycles

fix(repeat): synchronously bind child views
perf(repeat): basic utilization for indexMap to reduce unnecessary processing
chore(repeat): cleanup and try to improvate clarity a bit
test(repeat): improve test coverage and cleanup redundant tests

* chore(jit): fix repeater tests

* test(collection-observer): enable/disable observation at start of test

* test(repeat): add binding assertions

* test(if): improve if/else test coverage

* chore(repeat): make the view loops a bit saner

* test(all): remove .only

* test(repeat): add more batched assign+mutate tests

* test(repeat): add assertions that items mutations are batched but instance mutations are not

* test(templating-resources): add some initial integration tests for repeater and if/else

* chore(test): remove .only

* fix(repeat): use the correct lifecycle api for attach/detach

* fix(lifecycle): fix task variable shadowing issue

test(lifecycle): improve test coverage

* chore(all): enable no-shadowed-variable tslint rule

* chore(template): move CompiledTemplate to template.ts

* chore(template): move noViewTemplate to template.ts

* chore(rendering-engine): fix linting errors

* feat(render-context): throw more informative errors when the context is not prepared yet

test(compiled-template): improve test coverage

* test(view-factory): improve test coverage wrt caching

* test(view-factory): cleanup/add extra tests

* test(custom-element): improve decorator test coverage

* fix(view): check nextSibling instead of previousSibling in mount()

* fix(view): validate renderLocation and ensure $nodes.lastChild exists on mount

test(view): initial setup for test combinations
chore(test): add extra type params to cartesianJoinFactory

* test(view): improve test coverage

* test(custom-element): add preliminary projector tests

packages/jit/test/unit/templating/template-compiler.integration.spec.ts

@@ -413,9 +413,9 @@ describe('TemplateCompiler (integration)', () => {
       it(markup, () => {
         component = createCustomElement(markup);
         au.app({ host, component }).start();
-        expect(host.textContent).to.equal('');
+        expect(host.textContent.trim()).to.equal(''); // TODO: we kind of want to get rid of those spaces.. preferably
         initialize(component)
-        expect(host.textContent).to.equal('');
+        expect(host.textContent.trim()).to.equal('');
         cs.flushChanges();
         expect(host.textContent).to.equal(textContent);
       });
@@ -425,33 +425,72 @@ describe('TemplateCompiler (integration)', () => {
   it(`nested repeater - array`, () => {
     component = createCustomElement(`<template><div repeat.for="item of items"><div repeat.for="child of item">\${child}</div></div></template>`);
     au.app({ host, component: component }).start();
-    expect(host.textContent).to.equal('');
+    expect(host.textContent.trim()).to.equal('');
     component.items = [['1'], ['2'], ['3']];
-    expect(host.textContent).to.equal('');
+    expect(host.textContent.trim()).to.equal('');
     cs.flushChanges();
     expect(host.textContent).to.equal('123');
   });
 
   it(`repeater - sorted primitive array - asc`, () => {
     component = createCustomElement(`<template><div repeat.for="item of items | sort">\${item}</div></template>`);
     au.app({ host, component: component }).start();
-    expect(host.textContent).to.equal('');
+    expect(host.textContent.trim()).to.equal('');
     component.items = ['3', '2', '1'];
-    expect(host.textContent).to.equal('');
+    expect(host.textContent.trim()).to.equal('');
     cs.flushChanges();
     expect(host.textContent).to.equal('123');
   });
 
   it(`repeater - sorted primitive array - desc`, () => {
     component = createCustomElement(`<template><div repeat.for="item of items | sort:null:'desc'">\${item}</div></template>`);
     au.app({ host, component: component }).start();
-    expect(host.textContent).to.equal('');
+    expect(host.textContent.trim()).to.equal('');
     component.items = ['1', '2', '3'];
-    expect(host.textContent).to.equal('');
+    expect(host.textContent.trim()).to.equal('');
     cs.flushChanges();
     expect(host.textContent).to.equal('321');
   });
 
+  it(`repeater with nested if`, () => {
+    component = createCustomElement(`<template><div repeat.for="item of items"><div if.bind="$parent.show">\${item}</div></div></template>`);
+    au.app({ host, component }).start();
+    expect(host.textContent.trim()).to.equal('');
+    component.items = [['1'], ['2'], ['3']];
+    component.show = true;
+    expect(host.textContent.trim()).to.equal('');
+    cs.flushChanges();
+    expect(host.textContent).to.equal('123');
+    component.show = false;
+    expect(host.textContent.trim()).to.equal('');
+  });
+
+  it(`repeater with sibling if`, () => {
+    component = createCustomElement(`<template><div repeat.for="item of items" if.bind="$parent.show">\${item}</div></template>`);
+    au.app({ host, component }).start();
+    expect(host.textContent.trim()).to.equal('');
+    component.items = [['1'], ['2'], ['3']];
+    component.show = true;
+    expect(host.textContent.trim()).to.equal('');
+    cs.flushChanges();
+    expect(host.textContent).to.equal('123');
+    component.show = false;
+    expect(host.textContent.trim()).to.equal('');
+  });
+
+  it(`repeater with parent-sibling if`, () => {
+    component = createCustomElement(`<template><div if.bind="show" repeat.for="item of items">\${item}</div></template>`);
+    au.app({ host, component }).start();
+    expect(host.textContent.trim()).to.equal('');
+    component.items = [['1'], ['2'], ['3']];
+    component.show = true;
+    expect(host.textContent.trim()).to.equal('');
+    cs.flushChanges();
+    expect(host.textContent).to.equal('123');
+    component.show = false;
+    expect(host.textContent.trim()).to.equal('');
+  });
+
 
   // TODO: implement this in template compiler
   it(`if - shows and hides`, () => {

packages/runtime/src/binding/observation.ts

@@ -252,7 +252,7 @@ export type PropertyObserver = IPropertyObserver<any, PropertyKey>;
  */
 export type Collection = any[] | Set<any> | Map<any, any>;
 interface IObservedCollection {
-  $observer: CollectionObserver;
+  $observer?: CollectionObserver;
 }
 
 /**
@@ -280,27 +280,27 @@ export const enum CollectionKind {
   set     = 0b0111
 }
 
-type LengthPropertyName<T> =
+export type LengthPropertyName<T> =
   T extends any[] ? 'length' :
   T extends Set<any> ? 'size' :
   T extends Map<any, any> ? 'size' :
   never;
 
-type CollectionTypeToKind<T> =
+export type CollectionTypeToKind<T> =
   T extends any[] ? CollectionKind.array | CollectionKind.indexed :
   T extends Set<any> ? CollectionKind.set | CollectionKind.keyed :
   T extends Map<any, any> ? CollectionKind.map | CollectionKind.keyed :
   never;
 
-type CollectionKindToType<T> =
+export type CollectionKindToType<T> =
   T extends CollectionKind.array ? any[] :
   T extends CollectionKind.indexed ? any[] :
   T extends CollectionKind.map ? Map<any, any> :
   T extends CollectionKind.set ? Set<any> :
   T extends CollectionKind.keyed ? Set<any> | Map<any, any> :
   never;
 
-type ObservedCollectionKindToType<T> =
+export type ObservedCollectionKindToType<T> =
   T extends CollectionKind.array ? IObservedArray :
   T extends CollectionKind.indexed ? IObservedArray :
   T extends CollectionKind.map ? IObservedMap :

packages/runtime/src/binding/observer-locator.ts

@@ -7,7 +7,7 @@ import { IDirtyChecker } from './dirty-checker';
 import { CheckedObserver, SelectValueObserver, ValueAttributeObserver } from './element-observation';
 import { IEventManager } from './event-manager';
 import { getMapObserver } from './map-observer';
-import { AccessorOrObserver, CollectionKind, IBindingTargetAccessor, IBindingTargetObserver, ICollectionObserver, IObservable, IObservedArray, IObservedMap, IObservedSet } from './observation';
+import { AccessorOrObserver, CollectionKind, IBindingTargetAccessor, IBindingTargetObserver, ICollectionObserver, IObservable, IObservedArray, IObservedMap, IObservedSet, CollectionObserver } from './observation';
 import { PrimitiveObserver, SetterObserver } from './property-observation';
 import { getSetObserver } from './set-observer';
 import { ISVGAnalyzer } from './svg-analyzer';
@@ -240,3 +240,15 @@ export class ObserverLocator implements IObserverLocator {
     return new SetterObserver(obj, propertyName);
   }
 }
+
+export function getCollectionObserver(changeSet: IChangeSet, collection: IObservedMap | IObservedSet | IObservedArray): CollectionObserver {
+  switch (toStringTag.call(collection)) {
+    case '[object Array]':
+      return getArrayObserver(changeSet, <IObservedArray>collection);
+    case '[object Map]':
+      return getMapObserver(changeSet, <IObservedMap>collection);
+    case '[object Set]':
+      return getSetObserver(changeSet, <IObservedSet>collection);
+  }
+  return null;
+}

packages/runtime/src/templating/create-element.ts

@@ -36,7 +36,10 @@ export class PotentialRenderable {
       name: 'unnamed',
       templateOrNode: this.node,
       cache: 0,
-      build: {
+      build: typeof this.node === 'string' ? {
+        required: true,
+        compiler: 'default'
+      } : {
         required: false
       },
       dependencies: this.dependencies,

packages/runtime/src/templating/custom-element.ts

@@ -388,7 +388,7 @@ export class ContainerlessProjector implements IElementProjector {
 
   constructor(private customElement: ICustomElement, host: INode) {
     if (host.childNodes.length) {
-      this.childNodes = Array.from(host.childNodes);
+      this.childNodes = PLATFORM.toArray(host.childNodes);
     } else {
       this.childNodes = PLATFORM.emptyArray;
     }

packages/runtime/src/templating/lifecycle.ts

@@ -139,7 +139,7 @@ export interface IDetachLifecycle {
 }
 
 /*@internal*/
-class AttachLifecycleController implements IAttachLifecycle, IAttachLifecycleController {
+export class AttachLifecycleController implements IAttachLifecycle, IAttachLifecycleController {
   /*@internal*/
   public $nextAddNodes: LifecycleNodeAddable;
   /*@internal*/
@@ -179,13 +179,10 @@ class AttachLifecycleController implements IAttachLifecycle, IAttachLifecycleCon
     if (this.parent !== null) {
       this.parent.registerTask(task);
     } else {
-      let task = this.task;
-
-      if (task === null) {
-        this.task = task = new AggregateLifecycleTask();
+      if (this.task === null) {
+        this.task = new AggregateLifecycleTask();
       }
-
-      task.addTask(task);
+      this.task.addTask(task);
     }
   }
 
@@ -303,13 +300,10 @@ export class DetachLifecycleController implements IDetachLifecycle, IDetachLifec
     if (this.parent !== null) {
       this.parent.registerTask(task);
     } else {
-      let task = this.task;
-
-      if (task === null) {
-        this.task = task = new AggregateLifecycleTask();
+      if (this.task === null) {
+        this.task = new AggregateLifecycleTask();
       }
-
-      task.addTask(task);
+      this.task.addTask(task);
     }
   }
 

packages/runtime/src/templating/render-context.ts

@@ -1,4 +1,4 @@
-import { IContainer, IDisposable, Immutable, ImmutableArray, IResolver, IServiceLocator, PLATFORM } from '@aurelia/kernel';
+import { IContainer, IDisposable, Immutable, ImmutableArray, IResolver, IServiceLocator, PLATFORM, Reporter } from '@aurelia/kernel';
 import { DOM, INode, IRenderLocation } from '../dom';
 import { ITargetedInstruction, TemplateDefinition, TemplatePartDefinitions } from './instructions';
 import { IRenderable } from './renderable';
@@ -74,6 +74,9 @@ export class InstanceProvider<T> implements IResolver {
   }
 
   public resolve(handler: IContainer, requestor: IContainer): T {
+    if (this.instance === undefined) { // unmet precondition: call prepare
+      throw Reporter.error(50); // TODO: organize error codes
+    }
     return this.instance;
   }
 
@@ -95,8 +98,14 @@ export class ViewFactoryProvider implements IResolver {
   }
 
   public resolve(handler: IContainer, requestor: ExposedContext): IViewFactory {
-    const found = this.replacements[this.factory.name];
-
+    const factory = this.factory;
+    if (factory === undefined) { // unmet precondition: call prepare
+      throw Reporter.error(50); // TODO: organize error codes
+    }
+    if (!factory.name || !factory.name.length) { // unmet invariant: factory must have a name
+      throw Reporter.error(51); // TODO: organize error codes
+    }
+    const found = this.replacements[factory.name];
     if (found) {
       return this.renderingEngine.getViewFactory(found, requestor);
     }

packages/runtime/src/templating/rendering-engine.ts

@@ -3,16 +3,14 @@ import { IChangeSet } from '../binding/change-set';
 import { IEventManager } from '../binding/event-manager';
 import { IExpressionParser } from '../binding/expression-parser';
 import { IObserverLocator } from '../binding/observer-locator';
-import { DOM, INode, INodeSequence, NodeSequence } from '../dom';
 import { IResourceDescriptions, IResourceKind, IResourceType, ResourceDescription } from '../resource';
 import { ICustomAttribute, ICustomAttributeType } from './custom-attribute';
 import { ICustomElement, ICustomElementType } from './custom-element';
-import { ITemplateSource, TemplateDefinition, TemplatePartDefinitions } from './instructions';
-import { createRenderContext, ExposedContext, IRenderContext } from './render-context';
-import { IRenderable } from './renderable';
+import { ITemplateSource, TemplateDefinition } from './instructions';
+import { ExposedContext, IRenderContext } from './render-context';
 import { IRenderer, Renderer } from './renderer';
 import { RuntimeBehavior } from './runtime-behavior';
-import { ITemplate } from './template';
+import { CompiledTemplate, ITemplate, noViewTemplate } from './template';
 import { ITemplateCompiler } from './template-compiler';
 import { IViewFactory, ViewFactory } from './view';
 import { ViewCompileFlags } from './view-compile-flags';
@@ -22,30 +20,22 @@ export interface IRenderingEngine {
   getViewFactory(source: Immutable<ITemplateSource>, parentContext?: IRenderContext): IViewFactory;
 
   applyRuntimeBehavior(type: ICustomAttributeType, instance: ICustomAttribute): void;
-  applyRuntimeBehavior(type: ICustomElementType, instance: ICustomElement): void
+  applyRuntimeBehavior(type: ICustomElementType, instance: ICustomElement): void;
 
   createRenderer(context: IRenderContext): IRenderer;
 }
 
 export const IRenderingEngine = DI.createInterface<IRenderingEngine>()
   .withDefault(x => x.singleton(RenderingEngine));
 
-// This is an implementation of ITemplate that always returns a node sequence representing "no DOM" to render.
-const noViewTemplate: ITemplate = {
-  renderContext: null,
-  createFor(renderable: IRenderable): INodeSequence {
-    return NodeSequence.empty;
-  }
-};
-
 const defaultCompilerName = 'default';
 
 @inject(IContainer, IChangeSet, IObserverLocator, IEventManager, IExpressionParser, all(ITemplateCompiler))
 /*@internal*/
 export class RenderingEngine implements IRenderingEngine {
-  private templateLookup = new Map<TemplateDefinition, ITemplate>();
-  private factoryLookup = new Map<Immutable<ITemplateSource>, IViewFactory>();
-  private behaviorLookup = new Map<ICustomElementType | ICustomAttributeType, RuntimeBehavior>();
+  private templateLookup: Map<TemplateDefinition, ITemplate> = new Map();
+  private factoryLookup: Map<Immutable<ITemplateSource>, IViewFactory> = new Map();
+  private behaviorLookup: Map<ICustomElementType | ICustomAttributeType, RuntimeBehavior> = new Map();
   private compilers: Record<string, ITemplateCompiler>;
 
   constructor(
@@ -171,29 +161,6 @@ export function createDefinition(definition: Immutable<ITemplateSource>): Templa
   };
 }
 
-// This is the main implementation of ITemplate.
-// It is used to create instances of IView based on a compiled TemplateDefinition.
-// TemplateDefinitions are hand-coded today, but will ultimately be the output of the
-// TemplateCompiler either through a JIT or AOT process.
-// Essentially, CompiledTemplate wraps up the small bit of code that is needed to take a TemplateDefinition
-// and create instances of it on demand.
-/*@internal*/
-export class CompiledTemplate implements ITemplate {
-  public renderContext: IRenderContext;
-  private createNodeSequence: () => INodeSequence;
-
-  constructor(renderingEngine: IRenderingEngine, parentRenderContext: IRenderContext, private templateDefinition: TemplateDefinition) {
-    this.renderContext = createRenderContext(renderingEngine, parentRenderContext, templateDefinition.dependencies);
-    this.createNodeSequence = DOM.createFactoryFromMarkupOrNode(templateDefinition.templateOrNode);
-  }
-
-  public createFor(renderable: IRenderable, host?: INode, replacements?: TemplatePartDefinitions): INodeSequence {
-    const nodes = this.createNodeSequence();
-    this.renderContext.render(renderable, nodes.findTargets(), this.templateDefinition, host, replacements);
-    return nodes;
-  }
-}
-
 /*@internal*/
 export class RuntimeCompilationResources implements IResourceDescriptions {
   constructor(private context: ExposedContext) {}

packages/runtime/src/templating/resources/if.ts

@@ -15,11 +15,11 @@ export class If {
 
   public elseFactory: IViewFactory = null;
 
-  private ifView: IView = null;
-  private elseView: IView = null;
-  private coordinator: CompositionCoordinator;
+  public ifView: IView = null;
+  public elseView: IView = null;
+  public coordinator: CompositionCoordinator;
 
-  constructor(public ifFactory: IViewFactory, private location: IRenderLocation) {
+  constructor(public ifFactory: IViewFactory, public location: IRenderLocation) {
     this.coordinator = new CompositionCoordinator();
   }