Merge pull request #845 from glimmerjs/more-modifier-tests

Add more modifier tests

packages/@glimmer/compiler/lib/javascript-compiler.ts

@@ -12,7 +12,6 @@ import {
   Expression,
   Expressions,
   Ops,
-  isModifier,
   isFlushElement,
   isArgument,
   isAttribute,
@@ -85,9 +84,7 @@ export class ComponentBlock extends Block {
 
   push(statement: Statement) {
     if (this.inParams) {
-      if (isModifier(statement)) {
-        throw new Error('Compile Error: Element modifiers are not allowed in components');
-      } else if (isFlushElement(statement)) {
+      if (isFlushElement(statement)) {
         this.inParams = false;
       } else if (isArgument(statement)) {
         this.arguments.push(statement);
@@ -269,6 +266,9 @@ export default class JavaScriptCompiler
   }
 
   closeComponent(_element: AST.ElementNode) {
+    if (_element.modifiers.length > 0) {
+      throw new Error('Compile Error: Element modifiers are not allowed in components');
+    }
     let [tag, attrs, args, block] = this.endComponent();
 
     this.push([Ops.Component, tag, attrs, args, block]);

packages/@glimmer/compiler/lib/template-compiler.ts

@@ -113,9 +113,6 @@ export default class TemplateCompiler {
       this.attribute([typeAttr]);
     }
 
-    for (let i = 0; i < action.modifiers.length; i++) {
-      this.modifier([action.modifiers[i]]);
-    }
     this.opcode(['flushElement', action], null);
   }
 
@@ -124,6 +121,11 @@ export default class TemplateCompiler {
       this.opcode(['closeDynamicComponent', action], action);
     } else if (isComponent(action)) {
       this.opcode(['closeComponent', action], action);
+    } else if (action.modifiers.length > 0) {
+      for (let i = 0; i < action.modifiers.length; i++) {
+        this.modifier([action.modifiers[i]]);
+      }
+      this.opcode(['closeElement', action], action);
     } else {
       this.opcode(['closeElement', action], action);
     }

packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts

@@ -104,7 +104,7 @@ APPEND_OPCODES.add(Op.Modifier, (vm, { op1: handle }) => {
   let { manager, state } = vm.constants.resolveHandle<ModifierDefinition>(handle);
   let stack = vm.stack;
   let args = check(stack.pop(), CheckArguments);
-  let { constructing: element, updateOperations } = vm.elements();
+  let { element, updateOperations } = vm.elements();
   let dynamicScope = vm.dynamicScope();
   let modifier = manager.create(
     element as Simple.FIX_REIFICATION<Element>,

packages/@glimmer/runtime/test/modifiers-test.ts

@@ -0,0 +1,250 @@
+import { RenderTest, module, test } from '@glimmer/test-helpers';
+import { Opaque, Dict } from '../../util';
+
+class BaseModifier {
+  element?: Element;
+  didInsertElement(_params: Opaque[], _hash: Dict<Opaque>) {}
+  willDestroyElement() {}
+  didUpdate(_params: Opaque[], _hash: Dict<Opaque>) {}
+}
+
+abstract class AbstractInsertable extends BaseModifier {
+  abstract didInsertElement(_params: Opaque[], _hash: Dict<Opaque>): void;
+}
+
+abstract class AbstractDestroyable extends BaseModifier {
+  abstract willDestroyElement(): void;
+}
+
+class ModifierTests extends RenderTest {
+  @test
+  'Element modifier with hooks'(assert: Assert) {
+    assert.expect(4);
+
+    this.registerModifier(
+      'foo',
+      class {
+        element?: Element;
+        didInsertElement() {
+          assert.ok(this.element);
+          assert.equal(this.element!.getAttribute('data-ok'), 'true');
+        }
+
+        didUpdate() {
+          assert.ok(true);
+        }
+
+        willDestroyElement() {
+          assert.ok(true);
+        }
+      }
+    );
+
+    this.render('{{#if ok}}<div data-ok=true {{foo bar}}></div>{{/if}}', {
+      bar: 'bar',
+      ok: true,
+    });
+
+    this.rerender({ bar: 'foo' });
+    this.rerender({ ok: false });
+  }
+
+  @test
+  'didUpdate is not called when params are constants'(assert: Assert) {
+    assert.expect(2);
+    this.registerModifier(
+      'foo',
+      class {
+        element?: Element;
+        didInsertElement() {
+          assert.ok(true);
+        }
+        didUpdate() {
+          assert.ok(false);
+        }
+        willDestroyElement() {
+          assert.ok(true);
+        }
+      }
+    );
+
+    this.render('{{#if ok}}<div {{foo "foo" bar="baz"}}></div>{{/if}}{{ok}}', {
+      ok: true,
+      data: 'ok',
+    });
+    this.rerender({ data: 'yup' });
+    this.rerender({ ok: false });
+  }
+
+  @test
+  'do not work on component invocations'(assert: Assert) {
+    this.registerComponent('Glimmer', 'Foo', '<div ...attributes>Foo</div>');
+    this.registerModifier('bar', BaseModifier);
+    assert.throws(() => {
+      this.render('<Foo {{bar foo="foo"}} />');
+    }, 'Compile Error: Element modifiers are not allowed in components');
+
+    assert.throws(() => {
+      this.render('<Foo (bar foo="foo") />');
+    }, 'Compile Error: Element modifiers are not allowed in components');
+  }
+
+  @test
+  'parent -> child insertion order'(assert: Assert) {
+    let insertionOrder: string[] = [];
+
+    class Foo extends AbstractInsertable {
+      didInsertElement() {
+        insertionOrder.push('foo');
+      }
+    }
+
+    class Bar extends AbstractInsertable {
+      didInsertElement() {
+        insertionOrder.push('bar');
+      }
+    }
+    this.registerModifier('bar', Bar);
+    this.registerModifier('foo', Foo);
+
+    this.render('<div {{foo}}><div {{bar}}></div></div>');
+    assert.deepEqual(insertionOrder, ['bar', 'foo']);
+  }
+
+  @test
+  'parent -> child destruction order'(assert: Assert) {
+    let destructionOrder: string[] = [];
+
+    class Foo extends AbstractDestroyable {
+      willDestroyElement() {
+        destructionOrder.push('foo');
+      }
+    }
+
+    class Bar extends AbstractDestroyable {
+      willDestroyElement() {
+        destructionOrder.push('bar');
+      }
+    }
+    this.registerModifier('bar', Bar);
+    this.registerModifier('foo', Foo);
+
+    this.render('{{#if nuke}}<div {{foo}}><div {{bar}}></div></div>{{/if}}', { nuke: true });
+    assert.deepEqual(destructionOrder, []);
+    this.rerender({ nuke: false });
+    assert.deepEqual(destructionOrder, ['bar', 'foo']);
+  }
+
+  @test
+  'sibling insertion order'(assert: Assert) {
+    let insertionOrder: string[] = [];
+
+    class Foo extends AbstractInsertable {
+      didInsertElement() {
+        insertionOrder.push('foo');
+      }
+    }
+
+    class Bar extends AbstractInsertable {
+      didInsertElement() {
+        insertionOrder.push('bar');
+      }
+    }
+
+    class Baz extends AbstractInsertable {
+      didInsertElement() {
+        insertionOrder.push('baz');
+      }
+    }
+    this.registerModifier('bar', Bar);
+    this.registerModifier('foo', Foo);
+    this.registerModifier('baz', Baz);
+
+    this.render('<div {{foo}}><div {{bar}}></div><div {{baz}}></div></div>');
+    assert.deepEqual(insertionOrder, ['bar', 'baz', 'foo']);
+  }
+
+  @test
+  'sibling destruction order'(assert: Assert) {
+    let destructionOrder: string[] = [];
+
+    class Foo extends AbstractDestroyable {
+      willDestroyElement() {
+        destructionOrder.push('foo');
+      }
+    }
+
+    class Bar extends AbstractDestroyable {
+      willDestroyElement() {
+        destructionOrder.push('bar');
+      }
+    }
+
+    class Baz extends AbstractDestroyable {
+      willDestroyElement() {
+        destructionOrder.push('baz');
+      }
+    }
+    this.registerModifier('bar', Bar);
+    this.registerModifier('foo', Foo);
+    this.registerModifier('baz', Baz);
+
+    this.render('{{#if nuke}}<div {{foo}}><div {{bar}}></div><div {{baz}}></div></div>{{/if}}', {
+      nuke: true,
+    });
+    assert.deepEqual(destructionOrder, []);
+    this.rerender({ nuke: false });
+    assert.deepEqual(destructionOrder, ['bar', 'baz', 'foo']);
+  }
+
+  @test
+  'with params'(assert: Assert) {
+    assert.expect(2);
+    class Foo extends BaseModifier {
+      didInsertElement([bar]: string[]) {
+        assert.equal(bar, 'bar');
+      }
+      didUpdate([foo]: string[]) {
+        assert.equal(foo, 'foo');
+      }
+    }
+    this.registerModifier('foo', Foo);
+    this.render('<div {{foo bar}}></div>', { bar: 'bar' });
+    this.rerender({ bar: 'foo' });
+  }
+
+  @test
+  'with hash'(assert: Assert) {
+    assert.expect(2);
+    class Foo extends BaseModifier {
+      didInsertElement(_params: Opaque[], { bar }: Dict<string>) {
+        assert.equal(bar, 'bar');
+      }
+      didUpdate(_params: Opaque[], { bar }: Dict<string>) {
+        assert.equal(bar, 'foo');
+      }
+    }
+    this.registerModifier('foo', Foo);
+    this.render('<div {{foo bar=bar}}></div>', { bar: 'bar' });
+    this.rerender({ bar: 'foo' });
+  }
+
+  @test
+  'with hash and params'(assert: Assert) {
+    assert.expect(4);
+    class Foo extends BaseModifier {
+      didInsertElement([baz]: string[], { bar }: Dict<string>) {
+        assert.equal(bar, 'bar');
+        assert.equal(baz, 'baz');
+      }
+      didUpdate([foo]: string[], { bar }: Dict<string>) {
+        assert.equal(bar, 'foo');
+        assert.equal(foo, 'foo');
+      }
+    }
+    this.registerModifier('foo', Foo);
+    this.render('<div {{foo baz bar=bar}}></div>', { bar: 'bar', baz: 'baz' });
+    this.rerender({ bar: 'foo', baz: 'foo' });
+  }
+}
+module('Modifiers', ModifierTests);

packages/@glimmer/test-helpers/lib/environment/modifier.ts

@@ -7,7 +7,7 @@ import {
 } from '@glimmer/runtime';
 import { Option, Simple } from '@glimmer/interfaces';
 import { Tag, CONSTANT_TAG } from '@glimmer/reference';
-import { Destroyable } from '@glimmer/util';
+import { Destroyable, Opaque, Dict } from '@glimmer/util';
 
 export class InertModifierStateBucket {}
 
@@ -62,8 +62,8 @@ export interface TestModifierConstructor {
 
 export interface TestModifierInstance {
   element?: Simple.Element;
-  didInsertElement(): void;
-  didUpdate(): void;
+  didInsertElement(_params: Opaque[], _hash: Dict<Opaque>): void;
+  didUpdate(_params: Opaque[], _hash: Dict<Opaque>): void;
   willDestroyElement(): void;
 }
 
@@ -89,26 +89,26 @@ export class TestModifierManager
 
   install({ element, args, dom, state }: TestModifier) {
     this.installedElements.push(element);
-
-    let param = args.positional.at(0).value();
+    let firstParam = args.positional.at(0);
+    let param = firstParam !== undefined && firstParam.value();
     dom.setAttribute(element, 'data-modifier', `installed - ${param}`);
 
     if (state.instance && state.instance.didInsertElement) {
       state.instance.element = element;
-      state.instance.didInsertElement();
+      state.instance.didInsertElement(args.positional.value(), args.named.value());
     }
 
     return;
   }
 
   update({ element, args, dom, state }: TestModifier) {
     this.updatedElements.push(element);
-
-    let param = args.positional.at(0).value();
+    let firstParam = args.positional.at(0);
+    let param = firstParam !== undefined && firstParam.value();
     dom.setAttribute(element, 'data-modifier', `updated - ${param}`);
 
     if (state.instance && state.instance.didUpdate) {
-      state.instance.didUpdate();
+      state.instance.didUpdate(args.positional.value(), args.named.value());
     }
 
     return;
@@ -119,7 +119,7 @@ export class TestModifierManager
       destroy: () => {
         this.destroyedModifiers.push(modifier);
         let { element, dom, state } = modifier;
-        if (state.instance && state.instance.didUpdate) {
+        if (state.instance && state.instance.willDestroyElement) {
           state.instance.willDestroyElement();
         }
         dom.removeAttribute(element, 'data-modifier');

packages/@glimmer/wire-format/index.ts

@@ -219,7 +219,6 @@ export interface SerializedTemplateWithLazyBlock<Locator> {
 export type TemplateJavascript = string;
 
 // Statements
-export const isModifier = is<Statements.Modifier>(Opcodes.Modifier);
 export const isFlushElement = is<Statements.FlushElement>(Opcodes.FlushElement);
 export const isAttrSplat = is<Statements.AttrSplat>(Opcodes.AttrSplat);