RFC glimmerjs#435: Forward element modifiers with splattributes

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

@@ -2,7 +2,7 @@ import { assert } from '@glimmer/util';
 import { Stack, DictSet, Option, expect } from '@glimmer/util';
 import { AST } from '@glimmer/syntax';
 import { CompileOptions } from './template-compiler';
-import { isFlushElement, isArgument, isAttribute, isAttrSplat } from '@glimmer/wire-format';
+import { isFlushElement, isArgument, isAttribute } from '@glimmer/wire-format';
 import { Processor, CompilerOps, OpName, Op } from './compiler-ops';
 import {
   WireFormat,
@@ -94,8 +94,6 @@ export class ComponentBlock extends Block {
         this.arguments.push(statement);
       } else if (isAttribute(statement)) {
         this.attributes.push(statement);
-      } else if (isAttrSplat(statement)) {
-        this.attributes.push(statement);
       } else {
         throw new Error('Compile Error: only parameters allowed before flush-element');
       }
@@ -110,7 +108,6 @@ export class ComponentBlock extends Block {
 
   toJSON(): [string, Statements.Attribute[], Core.Hash, Core.Blocks] {
     let blocks: Core.Blocks;
-
     let args = this.arguments;
     let keys = args.map(arg => arg[1]);
     let values = args.map(arg => arg[2]);
@@ -205,7 +202,6 @@ export default class JavaScriptCompiler
       }
       (this[opcode] as any)(arg);
     });
-
     return this.template;
   }
 
@@ -243,7 +239,6 @@ export default class JavaScriptCompiler
   modifier(name: string) {
     let params = this.popValue<Params>();
     let hash = this.popValue<Hash>();
-
     this.push([SexpOpcodes.Modifier, name, params, hash]);
   }
 
@@ -317,10 +312,6 @@ 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, blocks] = this.endComponent();
 
     this.push([SexpOpcodes.Component, tag, attrs, args, blocks]);

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

@@ -127,22 +127,21 @@ export default class TemplateCompiler {
         this.attribute([typeAttr], hasSplat || actionIsComponent);
       }
 
+      for (let i = 0; i < action.modifiers.length; i++) {
+        this.modifier([action.modifiers[i]]);
+      }
+
       this.opcode(['flushElement', action], null);
     }
   }
 
   closeElement([action]: [AST.ElementNode]) {
-    if (isDynamicComponent(action)) {
-      this.opcode(['closeDynamicComponent', action], action);
-    } else if (isNamedBlock(action)) {
+    if (isNamedBlock(action)) {
       this.opcode(['closeNamedBlock', action]);
+    } else if (isDynamicComponent(action)) {
+      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/integration-tests/test/modifiers-test.ts

@@ -82,16 +82,180 @@ class ModifierTests extends RenderTest {
   }
 
   @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');
+  'modifiers on components are forwarded to a single element receiving the splattributes'(
+    assert: Assert
+  ) {
+    let modifierParams = null;
+    let modifierNamedArgs = null;
+    let modifiedElement: SimpleElement | undefined;
+    class Bar extends AbstractInsertable {
+      didInsertElement(params: unknown[], namedArgs: Dict<unknown>) {
+        modifierParams = params;
+        modifierNamedArgs = namedArgs;
+        modifiedElement = this.element;
+      }
+    }
+    this.registerComponent('Glimmer', 'TheFoo', '<div id="inner-div" ...attributes>Foo</div>');
+    this.registerModifier('bar', Bar);
+    this.render('<TheFoo {{bar "something" foo="else"}}/>');
+    assert.deepEqual(modifierParams, ['something']);
+    assert.deepEqual(modifierNamedArgs, { foo: 'else' });
+    assert.equal(
+      modifiedElement && modifiedElement.getAttribute('id'),
+      'inner-div',
+      'Modifier is called on the element receiving the splattributes'
+    );
+  }
+
+  @test
+  'modifiers on components are forwarded to all the elements receiving the splattributes'(
+    assert: Assert
+  ) {
+    let elements: SimpleElement[] = [];
+    class Bar extends AbstractInsertable {
+      didInsertElement(params: unknown[], namedArgs: Dict<unknown>) {
+        assert.deepEqual(params, ['something']);
+        assert.deepEqual(namedArgs, { foo: 'else' });
+        if (this.element) {
+          elements.push(this.element);
+        }
+      }
+    }
+    this.registerComponent(
+      'Glimmer',
+      'TheFoo',
+      '<div id="inner-one" ...attributes>Foo</div><div id="inner-two" ...attributes>Bar</div>'
+    );
+    this.registerModifier('bar', Bar);
+    this.render('<TheFoo {{bar "something" foo="else"}}/>');
+    assert.equal(
+      elements.length,
+      2,
+      'The modifier has been instantiated twice, once for each element with splattributes'
+    );
+  }
+
+  @test
+  'modifiers on components accept bound arguments'(assert: Assert) {
+    let modifierParams = null;
+    let modifierNamedArgs = null;
+    let modifiedElement: SimpleElement | undefined;
+    class Bar extends AbstractInsertable {
+      didInsertElement(params: unknown[], namedArgs: Dict<unknown>) {
+        modifierParams = params;
+        modifierNamedArgs = namedArgs;
+        modifiedElement = this.element;
+      }
+    }
+    this.registerComponent('Glimmer', 'TheFoo', '<div id="inner-div" ...attributes>Foo</div>');
+    this.registerModifier('bar', Bar);
+    this.render('<TheFoo {{bar this.something foo=this.foo}}/>', {
+      something: 'something',
+      foo: 'else',
+    });
+    assert.deepEqual(modifierParams, ['something']);
+    assert.deepEqual(modifierNamedArgs, { foo: 'else' });
+    assert.equal(
+      modifiedElement && modifiedElement.getAttribute('id'),
+      'inner-div',
+      'Modifier is called on the element receiving the splattributes'
+    );
+  }
+
+  @test
+  'modifiers on components accept `this` in both positional params and named arguments'(
+    assert: Assert
+  ) {
+    let modifierParams = null;
+    let modifierNamedArgs = null;
+    let modifiedElement: SimpleElement | undefined;
+    class Bar extends AbstractInsertable {
+      didInsertElement(params: unknown[], namedArgs: Dict<unknown>) {
+        modifierParams = params;
+        modifierNamedArgs = namedArgs;
+        modifiedElement = this.element;
+      }
+    }
+    let context = {};
+    this.registerComponent('Glimmer', 'TheFoo', '<div id="inner-div" ...attributes>Foo</div>');
+    this.registerModifier('bar', Bar);
+    this.render('<TheFoo {{bar "name" this foo=this}}/>', context);
+    assert.deepEqual(modifierParams, ['name', context]);
+    assert.deepEqual(modifierNamedArgs, { foo: context });
+    assert.equal(
+      modifiedElement && modifiedElement.getAttribute('id'),
+      'inner-div',
+      'Modifier is called on the element receiving the splattributes'
+    );
+  }
+
+  @test
+  'modifiers on components accept local variables in both positional params and named arguments'(
+    assert: Assert
+  ) {
+    let modifierParams = null;
+    let modifierNamedArgs = null;
+    let modifiedElement: SimpleElement | undefined;
+    class Bar extends AbstractInsertable {
+      didInsertElement(params: unknown[], namedArgs: Dict<unknown>) {
+        modifierParams = params;
+        modifierNamedArgs = namedArgs;
+        modifiedElement = this.element;
+      }
+    }
+    this.registerComponent('Glimmer', 'TheFoo', '<div id="inner-div" ...attributes>Foo</div>');
+    this.registerModifier('bar', Bar);
+    this.render(
+      `
+      {{#let this.foo as |v|}}
+        <TheFoo {{bar v foo=v}}/>
+      {{/let}}
+    `,
+      { foo: 'bar' }
+    );
+    assert.deepEqual(modifierParams, ['bar']);
+    assert.deepEqual(modifierNamedArgs, { foo: 'bar' });
+    assert.equal(
+      modifiedElement && modifiedElement.getAttribute('id'),
+      'inner-div',
+      'Modifier is called on the element receiving the splattributes'
+    );
+  }
+
+  @test
+  'modifiers on components can be received and forwarded to inner components'(assert: Assert) {
+    let modifierParams = null;
+    let modifierNamedArgs = null;
+    let modifiedElement: SimpleElement | undefined;
+    class Bar extends AbstractInsertable {
+      didInsertElement(params: unknown[], namedArgs: Dict<unknown>) {
+        modifierParams = params;
+        modifierNamedArgs = namedArgs;
+        modifiedElement = this.element;
+      }
+    }
+    this.registerComponent(
+      'Glimmer',
+      'TheInner',
+      '<div id="inner-div" ...attributes>{{yield}}</div>'
+    );
+    this.registerComponent('Glimmer', 'TheFoo', '<TheInner ...attributes>Hello</TheInner>');
+    this.registerModifier('bar', Bar);
+    this.render(
+      `
+      {{#let this.foo as |v|}}
+        <TheFoo {{bar v foo=v}}/>
+      {{/let}}
+    `,
+      { foo: 'bar' }
+    );
+    assert.deepEqual(modifierParams, ['bar']);
+    assert.deepEqual(modifierNamedArgs, { foo: 'bar' });
+    assert.equal(
+      modifiedElement && modifiedElement.getAttribute('id'),
+      'inner-div',
+      'Modifier is called on the element of the inner component'
+    );
   }
 
   @test
@@ -113,7 +277,7 @@ class ModifierTests extends RenderTest {
     this.registerModifier('foo', Foo);
 
     this.render('<div {{foo}}><div {{bar}}></div></div>');
-    assert.deepEqual(insertionOrder, ['bar', 'foo']);
+    assert.deepEqual(insertionOrder, ['foo', 'bar']);
   }
 
   @test
@@ -137,7 +301,7 @@ class ModifierTests extends RenderTest {
     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']);
+    assert.deepEqual(destructionOrder, ['foo', 'bar']);
   }
 
   @test
@@ -166,7 +330,7 @@ class ModifierTests extends RenderTest {
     this.registerModifier('baz', Baz);
 
     this.render('<div {{foo}}><div {{bar}}></div><div {{baz}}></div></div>');
-    assert.deepEqual(insertionOrder, ['bar', 'baz', 'foo']);
+    assert.deepEqual(insertionOrder, ['foo', 'bar', 'baz']);
   }
 
   @test
@@ -199,7 +363,7 @@ class ModifierTests extends RenderTest {
     });
     assert.deepEqual(destructionOrder, []);
     this.rerender({ nuke: false });
-    assert.deepEqual(destructionOrder, ['bar', 'baz', 'foo']);
+    assert.deepEqual(destructionOrder, ['foo', 'bar', 'baz']);
   }
 
   @test

packages/@glimmer/interfaces/lib/compile/wire-format.d.ts

@@ -211,6 +211,7 @@ export namespace Statements {
     | Statements.DynamicAttr
     | Statements.ComponentAttr
     | Statements.TrustingComponentAttr
+    | Statements.Modifier
     | Statements.AttrSplat;
 
   export type Argument = Statements.StaticArg | Statements.DynamicArg;

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

@@ -23,6 +23,7 @@ import { DynamicAttribute } from '../../vm/attributes/dynamic';
 import { CheckReference, CheckArguments, CheckOperations } from './-debug-strip';
 import { CONSTANTS } from '../../symbols';
 import { SimpleElement, SimpleNode } from '@simple-dom/interface';
+import { expect } from '@glimmer/util';
 
 APPEND_OPCODES.add(Op.Text, (vm, { op1: text }) => {
   vm.elements().appendText(vm[CONSTANTS].getString(text));
@@ -93,9 +94,15 @@ APPEND_OPCODES.add(Op.Modifier, (vm, { op1: handle }) => {
   let { manager, state } = vm.runtime.resolver.resolve<ModifierDefinition>(handle);
   let stack = vm.stack;
   let args = check(stack.pop(), CheckArguments);
-  let { element, updateOperations } = vm.elements();
+  let { constructing, updateOperations } = vm.elements();
   let dynamicScope = vm.dynamicScope();
-  let modifier = manager.create(element, state, args, dynamicScope, updateOperations);
+  let modifier = manager.create(
+    expect(constructing, 'ElementModifier could not find the element it applies to'),
+    state,
+    args,
+    dynamicScope,
+    updateOperations
+  );
 
   vm.env.scheduleInstallModifier(modifier, manager);
   let d = manager.getDestructor(modifier);

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

@@ -8,14 +8,15 @@ export function is<T>(variant: number): (value: any) => value is T {
 
 // Statements
 export const isFlushElement = is<Statements.FlushElement>(SexpOpcodes.FlushElement);
-export const isAttrSplat = is<Statements.AttrSplat>(SexpOpcodes.AttrSplat);
 
 export function isAttribute(val: Statement): val is Statements.Attribute {
   return (
     val[0] === SexpOpcodes.StaticAttr ||
     val[0] === SexpOpcodes.DynamicAttr ||
     val[0] === SexpOpcodes.TrustingDynamicAttr ||
-    val[0] === SexpOpcodes.ComponentAttr
+    val[0] === SexpOpcodes.ComponentAttr ||
+    val[0] === SexpOpcodes.AttrSplat ||
+    val[0] === SexpOpcodes.Modifier
   );
 }