Skip to content

Commit

Permalink
Fix nested calls to helpers in dynamic helpers
Browse files Browse the repository at this point in the history
Currently, nested calls to helpers in dynamic helpers cause problems
because the nested helper overwrites the register that we were using to
pass along the helper definition. This is actually a worse way of doing
something that the `Dup` opcode is designed to do anyways, so I updated
to do that, following the same pattern in DynamicModifiers. Added tests
for both dynamic helpers and modifiers with nested helpers.
  • Loading branch information
Chris Garrett committed Apr 1, 2021
1 parent 1bc620b commit b832ff2
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 7 deletions.
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -35,6 +42,41 @@ class DynamicHelpersResolutionModeTest extends RenderTest {
/The `x\.foo` property path was used in a template for the `.*` component 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);
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import {
defineSimpleModifier,
syntaxErrorFor,
GlimmerishComponent,
defineSimpleHelper,
defineComponent,
} from '../..';

class DynamicModifiersResolutionModeTest extends RenderTest {
Expand Down Expand Up @@ -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);
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { $v0 } from '@glimmer/vm';
import { $fp, $v0 } from '@glimmer/vm';
import {
Option,
Op,
Expand Down Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions packages/@glimmer/runtime/lib/compiled/opcodes/expressions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down

0 comments on commit b832ff2

Please sign in to comment.