Merge pull request #925 from cibernox/backport-rfc-435

Backport RFC# 435

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

@@ -14,8 +14,7 @@ import {
   Ops,
   isFlushElement,
   isArgument,
-  isAttribute,
-  isAttrSplat,
+  isAttribute
 } from '@glimmer/wire-format';
 import { Processor, CompilerOps, OpName, Op } from './compiler-ops';
 
@@ -90,8 +89,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');
       }
@@ -167,7 +164,6 @@ export default class JavaScriptCompiler
       }
       (this[opcode] as any)(arg);
     });
-
     return this.template;
   }
 
@@ -266,9 +262,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, block] = this.endComponent();
 
     this.push([Ops.Component, tag, attrs, args, block]);

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

@@ -113,6 +113,10 @@ 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);
   }
 
@@ -121,11 +125,6 @@ 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

@@ -30,6 +30,7 @@ import { Assert } from './vm';
 import { DynamicAttribute } from '../../vm/attributes/dynamic';
 import { ComponentElementOperations } from './component';
 import { CheckReference, CheckArguments } from './-debug-strip';
+import { expect } from '@glimmer/util';
 
 APPEND_OPCODES.add(Op.Text, (vm, { op1: text }) => {
   vm.elements().appendText(vm.constants.getString(text));
@@ -104,10 +105,10 @@ 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 { element, updateOperations } = vm.elements();
+  let { constructing, updateOperations } = vm.elements();
   let dynamicScope = vm.dynamicScope();
   let modifier = manager.create(
-    element as Simple.FIX_REIFICATION<Simple.Element>,
+    expect(constructing, 'ElementModifier could not find the element it applies to'),
     state,
     args,
     dynamicScope,

packages/@glimmer/runtime/test/ember-component-test.ts

@@ -1339,18 +1339,6 @@ QUnit.test(`Ensure components can be invoked`, function() {
   equalsElement(view.element, 'div', {}, 'hi!');
 });
 
-QUnit.test(`Glimmer component with element modifier`, function(assert) {
-  env.registerEmberishGlimmerComponent('NonBlock', null, `  <div>In layout</div>  `);
-
-  assert.throws(
-    () => {
-      appendViewFor('<NonBlock {{action}} />');
-    },
-    new Error('Compile Error: Element modifiers are not allowed in components'),
-    'should throw error'
-  );
-});
-
 QUnit.test('Custom element with element modifier', function(assert) {
   assert.expect(0);
 

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

@@ -1,6 +1,8 @@
 import { RenderTest, module, test } from '@glimmer/test-helpers';
 import { Opaque, Dict } from '../../util';
 import { Simple } from '@glimmer/interfaces';
+import { SimpleElement } from '@simple-dom/interface';
+import { Option } from '@glimmer/interfaces';
 
 class BaseModifier {
   element?: Simple.Element;
@@ -78,16 +80,223 @@ 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: Option<unknown[]> = null;
+    let modifierNamedArgs: Option<Dict<unknown>> = 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 elementIds: Option<string>[] = [];
+    class Bar extends AbstractInsertable {
+      didInsertElement(params: unknown[], namedArgs: Dict<unknown>) {
+        assert.deepEqual(params, ['something']);
+        assert.deepEqual(namedArgs, { foo: 'else' });
+        if (this.element) {
+          elementIds.push(this.element.getAttribute('id'));
+        }
+      }
+    }
+    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.deepEqual(
+      elementIds,
+      ['inner-one', 'inner-two'],
+      'The modifier has been instantiated twice, once for each element with splattributes'
+    );
+  }
+
+  @test
+  'modifiers on components accept bound arguments and track changes on them'(assert: Assert) {
+    let modifierParams: Option<unknown[]> = null;
+    let modifierNamedArgs: Option<Dict<unknown>> = null;
+    let modifiedElement: SimpleElement | undefined;
+    class Bar extends AbstractInsertable {
+      didInsertElement(params: unknown[], namedArgs: Dict<unknown>) {
+        modifierParams = params;
+        modifierNamedArgs = namedArgs;
+        modifiedElement = this.element;
+      }
+      didUpdate(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'
+    );
+    this.rerender({ something: 'another', foo: 'thingy' });
+    assert.deepEqual(modifierParams, ['another']);
+    assert.deepEqual(modifierNamedArgs, { foo: 'thingy' });
+    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, and updates when it changes'(
+    assert: Assert
+  ) {
+    let modifierParams: Option<unknown[]> = null;
+    let modifierNamedArgs: Option<Dict<unknown>> = null;
+    let modifiedElement: SimpleElement | undefined;
+    class Bar extends AbstractInsertable {
+      didInsertElement(params: unknown[], namedArgs: Dict<unknown>) {
+        modifierParams = params;
+        modifierNamedArgs = namedArgs;
+        modifiedElement = this.element;
+      }
+      didUpdate(params: unknown[], namedArgs: Dict<unknown>) {
+        modifierParams = params;
+        modifierNamedArgs = namedArgs;
+        modifiedElement = this.element;
+      }
+    }
+    let context = { id: 1 };
+    let context2 = { id: 2 };
+    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'
+    );
+    this.rerender(context2);
+    assert.deepEqual(modifierParams, ['name', context2]);
+    assert.deepEqual(modifierNamedArgs, { foo: context2 });
+    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, and updates when they change'(
+    assert: Assert
+  ) {
+    let modifierParams: Option<unknown[]> = null;
+    let modifierNamedArgs: Option<Dict<unknown>> = null;
+    let modifiedElement: SimpleElement | undefined;
+    class Bar extends AbstractInsertable {
+      didInsertElement(params: unknown[], namedArgs: Dict<unknown>) {
+        modifierParams = params;
+        modifierNamedArgs = namedArgs;
+        modifiedElement = this.element;
+      }
+      didUpdate(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(
+      `
+      {{#with this.foo as |v|}}
+        <TheFoo {{bar v foo=v}}/>
+      {{/with}}
+    `,
+      { 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'
+    );
+    this.rerender({ foo: 'qux' });
+    assert.deepEqual(modifierParams, ['qux']);
+    assert.deepEqual(modifierNamedArgs, { foo: 'qux' });
+    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: Option<unknown[]> = null;
+    let modifierNamedArgs: Option<Dict<unknown>> = null;
+    let elementIds: Option<string>[] = [];
+
+    class Bar extends AbstractInsertable {
+      didInsertElement(params: unknown[], namedArgs: Dict<unknown>) {
+        modifierParams = params;
+        modifierNamedArgs = namedArgs;
+        if (this.element) {
+          elementIds.push(this.element.getAttribute('id'));
+        }
+      }
+    }
+    this.registerComponent(
+      'Glimmer',
+      'TheInner',
+      '<div id="inner-div" ...attributes>{{yield}}</div>'
+    );
+    this.registerComponent(
+      'Glimmer',
+      'TheFoo',
+      '<div id="outer-div" ...attributes>Outer</div><TheInner ...attributes>Hello</TheInner>'
+    );
+    this.registerModifier('bar', Bar);
+    this.render(
+      `
+      {{#with this.foo as |v|}}
+        <TheFoo {{bar v foo=v}}/>
+      {{/with}}
+    `,
+      { foo: 'bar' }
+    );
+    assert.deepEqual(modifierParams, ['bar']);
+    assert.deepEqual(modifierNamedArgs, { foo: 'bar' });
+    assert.deepEqual(elementIds, ['outer-div', 'inner-div'], 'Modifiers are called on all levels');
   }
 
   @test
@@ -109,7 +318,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
@@ -133,7 +342,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
@@ -162,7 +371,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
@@ -195,7 +404,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/wire-format/index.ts

@@ -1,7 +1,6 @@
 import { Dict, Option, Opaque } from '@glimmer/util';
 import { Opcodes } from './lib/opcodes';
-
-export { Opcodes as Ops } from './lib/opcodes';
+export { Opcodes as Ops };
 
 type JsonValue = string | number | boolean | JsonObject | JsonArray;
 
@@ -226,7 +225,9 @@ export function isAttribute(val: Statement): val is Statements.Attribute {
   return (
     val[0] === Opcodes.StaticAttr ||
     val[0] === Opcodes.DynamicAttr ||
-    val[0] === Opcodes.TrustingAttr
+    val[0] === Opcodes.TrustingAttr ||
+    val[0] === Opcodes.AttrSplat ||
+    val[0] === Opcodes.Modifier
   );
 }