Restores modifier execution order

Closes #926

packages/@glimmer/compiler/lib/allocate-symbols.ts

@@ -169,8 +169,8 @@ export class SymbolAllocator
  text(_op: string) {}
  comment(_op: string) {}
  openComponent(_op: AST.ElementNode) {}
- openElement(_op: AST.ElementNode) {}
- openSplattedElement(_op: AST.ElementNode) {}
+ openElement(_op: [AST.ElementNode, boolean]) {}
+ openElementWithOperations(_op: AST.ElementNode) {}
  staticArg(_op: string) {}
  dynamicArg(_op: string) {}
  staticAttr(_op: [string, Option<string>]) {}

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

@@ -13,10 +13,9 @@ export interface CompilerOps<Variable> {
  endBlock: null;
  text: string;
  comment: string;
- openElement: AST.ElementNode;
+ openElement: [AST.ElementNode, boolean];
  openComponent: AST.ElementNode;
  openNamedBlock: AST.ElementNode;
- openSplattedElement: AST.ElementNode;
  flushElement: AST.ElementNode;
  closeElement: AST.ElementNode;
  closeComponent: AST.ElementNode;

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

@@ -283,27 +283,15 @@ export default class JavaScriptCompiler
  this.blocks.push(block);
  }
 
- openSplattedElement(element: AST.ElementNode) {
+ openElement([element, simple]: [AST.ElementNode, boolean]) {
  let tag = element.tag;
 
  if (element.blockParams.length > 0) {
  throw new Error(
  `Compile Error: <${element.tag}> is not a component and doesn't support block parameters`
  );
  } else {
- this.push([SexpOpcodes.OpenSplattedElement, tag]);
- }
- }
-
- openElement(element: AST.ElementNode) {
- let tag = element.tag;
-
- if (element.blockParams.length > 0) {
- throw new Error(
- `Compile Error: <${element.tag}> is not a component and doesn't support block parameters`
- );
- } else {
- this.push([SexpOpcodes.OpenElement, tag]);
+ this.push([SexpOpcodes.OpenElement, tag, simple]);
  }
  }
 

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

@@ -79,16 +79,20 @@ export default class TemplateCompiler {
 
  openElement([action]: [AST.ElementNode]) {
  let attributes = action.attributes;
- let hasSplat = false;
+ let simple = true;
 
  for (let i = 0; i < attributes.length; i++) {
  let attr = attributes[i];
  if (attr.name === '...attributes') {
- hasSplat = true;
+ simple = false;
  break;
  }
  }
 
+ if (action.modifiers.length > 0) {
+ simple = false;
+ }
+
  let actionIsComponent = false;
 
  if (isDynamicComponent(action)) {
@@ -105,10 +109,8 @@ export default class TemplateCompiler {
  } else if (isComponent(action)) {
  this.opcode(['openComponent', action], action);
  actionIsComponent = true;
- } else if (hasSplat) {
- this.opcode(['openSplattedElement', action], action);
  } else {
- this.opcode(['openElement', action], action);
+ this.opcode(['openElement', [action, simple]], action);
  }
 
  if (!isNamedBlock(action)) {
@@ -120,11 +122,11 @@ export default class TemplateCompiler {
  typeAttr = attrs[i];
  continue;
  }
- this.attribute([attrs[i]], hasSplat || actionIsComponent);
+ this.attribute([attrs[i]], !simple || actionIsComponent);
  }
 
  if (typeAttr) {
- this.attribute([typeAttr], hasSplat || actionIsComponent);
+ this.attribute([typeAttr], !simple || actionIsComponent);
  }
 
  for (let i = 0; i < action.modifiers.length; i++) {

packages/@glimmer/integration-tests/test/modifiers-test.ts

@@ -302,6 +302,52 @@ class ModifierTests extends RenderTest {
  assert.deepEqual(elementIds, ['outer-div', 'inner-div'], 'Modifiers are called on all levels');
  }
 
+ @test
+ 'same element 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}} {{bar}}></div>');
+ assert.deepEqual(insertionOrder, ['foo', 'bar']);
+ }
+
+ @test
+ 'same element 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}} {{bar}}></div>{{/if}}', { nuke: true });
+ assert.deepEqual(destructionOrder, []);
+ this.rerender({ nuke: false });
+ assert.deepEqual(destructionOrder, ['foo', 'bar']);
+ }
+
  @test
  'parent -> child insertion order'(assert: Assert) {
  let insertionOrder: string[] = [];

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

@@ -23,7 +23,6 @@ export const enum SexpOpcodes {
  Component = 5,
  DynamicComponent = 6,
  OpenElement = 7,
- OpenSplattedElement = 8,
  FlushElement = 9,
  CloseElement = 10,
  StaticAttr = 11,
@@ -62,7 +61,6 @@ export interface SexpOpcodeMap {
  [SexpOpcodes.Component]: Statements.Component;
  [SexpOpcodes.DynamicComponent]: Statements.DynamicComponent;
  [SexpOpcodes.OpenElement]: Statements.OpenElement;
- [SexpOpcodes.OpenSplattedElement]: Statements.SplatElement;
  [SexpOpcodes.FlushElement]: Statements.FlushElement;
  [SexpOpcodes.CloseElement]: Statements.CloseElement;
  [SexpOpcodes.StaticAttr]: Statements.StaticAttr;
@@ -163,8 +161,7 @@ export namespace Statements {
  Hash,
  Blocks
  ];
- export type OpenElement = [SexpOpcodes.OpenElement, str];
- export type SplatElement = [SexpOpcodes.OpenSplattedElement, str];
+ export type OpenElement = [SexpOpcodes.OpenElement, str, boolean];
  export type FlushElement = [SexpOpcodes.FlushElement];
  export type CloseElement = [SexpOpcodes.CloseElement];
  export type StaticAttr = [SexpOpcodes.StaticAttr, str, str, Option<str>];
@@ -191,7 +188,6 @@ export namespace Statements {
  | Component
  | DynamicComponent
  | OpenElement
- | SplatElement
  | FlushElement
  | CloseElement
  | StaticAttr

packages/@glimmer/interfaces/lib/dom/attributes.d.ts

@@ -12,6 +12,7 @@ import { ElementOperations, Environment } from '../runtime';
 import { GlimmerTreeConstruction, GlimmerTreeChanges } from './changes';
 import { Stack } from '../stack';
 import { LinkedList, LinkedListNode } from '../list';
+import { ModifierManager } from '@glimmer/interfaces';
 
 export interface LiveBlock extends Bounds {
  openElement(element: SimpleElement): void;
@@ -45,7 +46,7 @@ export interface DOMStack {
  popRemoteElement(): void;
  popElement(): void;
  openElement(tag: string, _operations?: ElementOperations): SimpleElement;
- flushElement(): void;
+ flushElement(modifiers: Option<[ModifierManager, unknown][]>): void;
  appendText(string: string): SimpleText;
  appendComment(string: string): SimpleComment;
 
@@ -61,7 +62,8 @@ export interface DOMStack {
  isTrusting: boolean,
  namespace: Option<string>
  ): AttributeOperation;
- closeElement(): void;
+
+ closeElement(): Option<[ModifierManager, unknown][]>;
 }
 
 export interface TreeOperations {

packages/@glimmer/interfaces/lib/runtime/element.d.ts

@@ -1,5 +1,6 @@
 import { VersionedReference } from '@glimmer/reference';
 import { Option } from '../core';
+import { ModifierManager } from '@glimmer/interfaces';
 
 export interface ElementOperations {
  setAttribute(
@@ -8,4 +9,6 @@ export interface ElementOperations {
  trusting: boolean,
  namespace: Option<string>
  ): void;
+
+ addModifier<S>(manager: ModifierManager<S>, state: S): void;
 }

packages/@glimmer/node/lib/serialize-builder.ts

@@ -1,4 +1,4 @@
-import { Bounds, Environment, Option, ElementBuilder } from '@glimmer/interfaces';
+import { Bounds, Environment, Option, ElementBuilder, ModifierManager } from '@glimmer/interfaces';
 import { ConcreteBounds, NewElementBuilder } from '@glimmer/runtime';
 import { RemoteLiveBlock } from '@glimmer/runtime';
 import { SimpleElement, SimpleNode, SimpleText } from '@simple-dom/interface';
@@ -68,13 +68,13 @@ class SerializeBuilder extends NewElementBuilder implements ElementBuilder {
  return super.__appendText(string);
  }
 
- closeElement() {
+ closeElement(): Option<[ModifierManager, unknown][]> {
  if (NEEDS_EXTRA_CLOSE.has(this.element)) {
  NEEDS_EXTRA_CLOSE.delete(this.element);
  super.closeElement();
  }
 
- super.closeElement();
+ return super.closeElement();
  }
 
  openElement(tag: string) {
@@ -86,7 +86,7 @@ class SerializeBuilder extends NewElementBuilder implements ElementBuilder {
  // account for the insertion since it is injected here and not
  // really in the template.
  NEEDS_EXTRA_CLOSE.set(this.constructing!, true);
- this.flushElement();
+ this.flushElement(null);
  }
  }
 

packages/@glimmer/opcode-compiler/lib/syntax/statements.ts

@@ -60,12 +60,13 @@ STATEMENTS.add(SexpOpcodes.TrustingComponentAttr, ([, name, value, namespace]) =
  op(Op.ComponentAttr, name, true, namespace),
 ]);
 
-STATEMENTS.add(SexpOpcodes.OpenElement, ([, tag]) => op(Op.OpenElement, tag));
-
-STATEMENTS.add(SexpOpcodes.OpenSplattedElement, ([, tag]) => [
- op(Op.PutComponentOperations),
- op(Op.OpenElement, tag),
-]);
+STATEMENTS.add(SexpOpcodes.OpenElement, ([, tag, simple]) => {
+ if (simple) {
+ return op(Op.OpenElement, tag);
+ } else {
+ return [op(Op.PutComponentOperations), op(Op.OpenElement, tag)];
+ }
+});
 
 STATEMENTS.add(SexpOpcodes.DynamicComponent, ([, definition, attrs, args, blocks]) => {
  return op('DynamicComponent', {

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

@@ -35,6 +35,7 @@ import {
  WithCreateInstance,
  JitRuntimeResolver,
  RuntimeResolver,
+ ModifierManager,
 } from '@glimmer/interfaces';
 import {
  CONSTANT_TAG,
@@ -411,6 +412,7 @@ interface DeferredAttribute {
 export class ComponentElementOperations implements ElementOperations {
  private attributes = dict<DeferredAttribute>();
  private classes: VersionedReference<unknown>[] = [];
+ private modifiers: [ModifierManager<unknown>, unknown][] = [];
 
  setAttribute(
  name: string,
@@ -427,7 +429,11 @@ export class ComponentElementOperations implements ElementOperations {
  this.attributes[name] = deferred;
  }
 
- flush(vm: InternalVM<JitOrAotBlock>) {
+ addModifier<S>(manager: ModifierManager<S>, state: S): void {
+ this.modifiers.push([manager, state]);
+ }
+
+ flush(vm: InternalVM<JitOrAotBlock>): [ModifierManager<unknown>, unknown][] {
  for (let name in this.attributes) {
  let attr = this.attributes[name];
  let { value: reference, namespace, trusting } = attr;
@@ -461,6 +467,8 @@ export class ComponentElementOperations implements ElementOperations {
  vm.updateWith(new UpdateDynamicAttributeOpcode(reference, attribute));
  }
  }
+
+ return this.modifiers;
  }
 }
 

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

@@ -8,7 +8,7 @@ import {
  isConstTag,
 } from '@glimmer/reference';
 import { check, CheckString, CheckElement } from '@glimmer/debug';
-import { Op, Option } from '@glimmer/interfaces';
+import { Op, Option, ModifierManager } from '@glimmer/interfaces';
 import { $t0 } from '@glimmer/vm';
 import {
  ModifierDefinition,
@@ -69,17 +69,29 @@ APPEND_OPCODES.add(Op.PopRemoteElement, vm => {
 
 APPEND_OPCODES.add(Op.FlushElement, vm => {
  let operations = check(vm.fetchValue($t0), CheckOperations);
+ let modifiers: Option<[ModifierManager, unknown][]> = null;
 
  if (operations) {
- operations.flush(vm);
+ modifiers = operations.flush(vm);
  vm.loadValue($t0, null);
  }
 
- vm.elements().flushElement();
+ vm.elements().flushElement(modifiers);
 });
 
 APPEND_OPCODES.add(Op.CloseElement, vm => {
- vm.elements().closeElement();
+ let modifiers = vm.elements().closeElement();
+
+ if (modifiers) {
+ modifiers.forEach(([manager, modifier]) => {
+ vm.env.scheduleInstallModifier(modifier, manager);
+ let d = manager.getDestructor(modifier);
+
+ if (d) {
+ vm.associateDestroyable(d);
+ }
+ });
+ }
 });
 
 APPEND_OPCODES.add(Op.Modifier, (vm, { op1: handle }) => {
@@ -89,19 +101,19 @@ APPEND_OPCODES.add(Op.Modifier, (vm, { op1: handle }) => {
  let { constructing, updateOperations } = vm.elements();
  let dynamicScope = vm.dynamicScope();
  let modifier = manager.create(
- expect(constructing, 'ElementModifier could not find the element it applies to'),
+ expect(constructing, 'BUG: ElementModifier could not find the element it applies to'),
  state,
  args,
  dynamicScope,
  updateOperations
  );
 
- vm.env.scheduleInstallModifier(modifier, manager);
- let d = manager.getDestructor(modifier);
+ let operations = expect(
+ check(vm.fetchValue($t0), CheckOperations),
+ 'BUG: ElementModifier could not find operations to append to'
+ );
 
- if (d) {
- vm.associateDestroyable(d);
- }
+ operations.addModifier(manager, modifier);
 
  let tag = manager.getTag(modifier);