Merge pull request #1293 from glimmerjs/bugfix/nested-dynamic-helper-…

…calls Fix nested calls to helpers in dynamic helpers
glimmerjs · Apr 15, 2021 · a3091ee · a3091ee
2 parents 83664b8 + b832ff2
commit a3091ee
Show file tree

Hide file tree

Showing 4 changed files with 90 additions and 7 deletions.
diff --git a/packages/@glimmer/integration-tests/test/helpers/dynamic-helpers-test.ts b/packages/@glimmer/integration-tests/test/helpers/dynamic-helpers-test.ts
@@ -1,4 +1,11 @@
-import { RenderTest, test, jitSuite, GlimmerishComponent, defineSimpleHelper } from '../..';
+import {
+  RenderTest,
+  test,
+  jitSuite,
+  GlimmerishComponent,
+  defineSimpleHelper,
+  defineComponent,
+} from '../..';
 
 class DynamicHelpersResolutionModeTest extends RenderTest {
   static suiteName = 'dynamic helpers in resolution mode';
@@ -35,6 +42,41 @@ class DynamicHelpersResolutionModeTest extends RenderTest {
       /The `x\.foo` property path was used in the `.*` template without using `this`/
     );
   }
+
+  @test
+  'Can use a dynamic helper with nested helpers'() {
+    const foo = defineSimpleHelper(() => 'world!');
+    const bar = defineSimpleHelper((value: string) => 'Hello, ' + value);
+    const Bar = defineComponent(
+      { foo },
+      '{{this.bar (foo)}}',
+      class extends GlimmerishComponent {
+        bar = bar;
+      }
+    );
+
+    this.renderComponent(Bar);
+    this.assertHTML('Hello, world!');
+    this.assertStableRerender();
+  }
+
+  @test
+  'Can use a dynamic helper with nested dynamic helpers'() {
+    const foo = defineSimpleHelper(() => 'world!');
+    const bar = defineSimpleHelper((value: string) => 'Hello, ' + value);
+    const Bar = defineComponent(
+      {},
+      '{{this.bar (this.foo)}}',
+      class extends GlimmerishComponent {
+        foo = foo;
+        bar = bar;
+      }
+    );
+
+    this.renderComponent(Bar);
+    this.assertHTML('Hello, world!');
+    this.assertStableRerender();
+  }
 }
 
 jitSuite(DynamicHelpersResolutionModeTest);
diff --git a/packages/@glimmer/integration-tests/test/modifiers/dynamic-modifiers-test.ts b/packages/@glimmer/integration-tests/test/modifiers/dynamic-modifiers-test.ts
@@ -5,6 +5,8 @@ import {
   defineSimpleModifier,
   syntaxErrorFor,
   GlimmerishComponent,
+  defineSimpleHelper,
+  defineComponent,
 } from '../..';
 
 class DynamicModifiersResolutionModeTest extends RenderTest {
@@ -131,6 +133,45 @@ class DynamicModifiersResolutionModeTest extends RenderTest {
       this.registerComponent('TemplateOnly', 'Bar', '<div {{x.foo}}></div>');
     }, syntaxErrorFor('You attempted to invoke a path (`{{#x.foo}}`) as a modifier, but x was not in scope. Try adding `this` to the beginning of the path', '{{x.foo}}', 'an unknown module', 1, 5));
   }
+
+  @test
+  'Can use a dynamic modifier with a nested helper'() {
+    const foo = defineSimpleHelper(() => 'Hello, world!');
+    const bar = defineSimpleModifier(
+      (element: Element, value: string) => (element.innerHTML = value)
+    );
+    const Bar = defineComponent(
+      { foo },
+      '<div {{this.bar (foo)}}></div>',
+      class extends GlimmerishComponent {
+        bar = bar;
+      }
+    );
+
+    this.renderComponent(Bar);
+    this.assertHTML('<div>Hello, world!</div>');
+    this.assertStableRerender();
+  }
+
+  @test
+  'Can use a dynamic modifier with a nested dynamic helper'() {
+    const foo = defineSimpleHelper(() => 'Hello, world!');
+    const bar = defineSimpleModifier(
+      (element: Element, value: string) => (element.innerHTML = value)
+    );
+    const Bar = defineComponent(
+      {},
+      '<div {{this.bar (this.foo)}}></div>',
+      class extends GlimmerishComponent {
+        foo = foo;
+        bar = bar;
+      }
+    );
+
+    this.renderComponent(Bar);
+    this.assertHTML('<div>Hello, world!</div>');
+    this.assertStableRerender();
+  }
 }
 
 jitSuite(DynamicModifiersResolutionModeTest);
diff --git a/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/vm.ts b/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/vm.ts
@@ -1,4 +1,4 @@
-import { $v0 } from '@glimmer/vm';
+import { $fp, $v0 } from '@glimmer/vm';
 import {
   Option,
   Op,
@@ -79,13 +79,13 @@ export function CallDynamic(
   named: WireFormat.Core.Hash,
   append?: () => void
 ): void {
-  op(Op.Load, $v0);
   op(MachineOp.PushFrame);
   SimpleArgs(op, positional, named, false);
-  op(Op.DynamicHelper, $v0);
+  op(Op.Dup, $fp, 1);
+  op(Op.DynamicHelper);
   if (append) {
     op(Op.Fetch, $v0);
-    append?.();
+    append();
     op(MachineOp.PopFrame);
   } else {
     op(MachineOp.PopFrame);

diff --git a/packages/@glimmer/runtime/lib/compiled/opcodes/expressions.ts b/packages/@glimmer/runtime/lib/compiled/opcodes/expressions.ts
@@ -73,10 +73,10 @@ APPEND_OPCODES.add(Op.Curry, (vm, { op1: type, op2: _isStrict }) => {
   );
 });
 
-APPEND_OPCODES.add(Op.DynamicHelper, (vm, { op1: _definitionRegister }) => {
+APPEND_OPCODES.add(Op.DynamicHelper, (vm) => {
   let stack = vm.stack;
+  let ref = check(stack.popJs(), CheckReference);
   let args = check(stack.popJs(), CheckArguments).capture();
-  let ref = vm.fetchValue<Reference>(_definitionRegister);
 
   let helperRef: Reference;
   let initialOwner: Owner = vm.getOwner();