Skip to content

Commit 87d8b52

Browse files
NullVoxPopuliclaude
andcommitted
Apply binding to all path reads, add #let block param test
Move GetPropertyBound emission into withPath so ALL multi-segment path reads (this.*, @arg.*, blockParam.*, lexical.*) tag the last segment's ref with its parent. With the ref-tagging approach this is safe — the value identity is unchanged, only on/fn check the tag. This means {{#let this.obj as |o|}} followed by o.method correctly binds method to obj, matching the behavior of this.obj.method. Also adds a test for the #let + fn pattern per review request: {{#let this.obj as |o|}} <button {{on 'click' (fn o.method 'did it')}}>click me</button> {{/let}} Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
1 parent 4ebeef2 commit 87d8b52

2 files changed

Lines changed: 49 additions & 12 deletions

File tree

packages/@ember/-internals/glimmer/tests/integration/this-binding-test.gjs

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { RenderingTestCase, moduleFor, runTask } from 'internal-test-helpers';
2+
import { fn } from '@ember/helper';
23
import { on } from '@ember/modifier';
34
import { Component } from '../utils/helpers';
45

@@ -70,5 +71,47 @@ moduleFor(
7071
'`this` inside the nested method should be the inner object instance'
7172
);
7273
}
74+
['@test #let block param preserves this binding on method access'](assert) {
75+
let innerInstance;
76+
let seenThis;
77+
let receivedArg;
78+
79+
class Inner {
80+
constructor() {
81+
innerInstance = this;
82+
}
83+
84+
method(arg) {
85+
seenThis = this;
86+
receivedArg = arg;
87+
}
88+
}
89+
90+
class Demo extends Component {
91+
constructor(...args) {
92+
super(...args);
93+
this.obj = new Inner();
94+
}
95+
96+
<template>
97+
{{#let this.obj as |o|}}
98+
<button type="button" {{on "click" (fn o.method "did it")}}>click me</button>
99+
{{/let}}
100+
</template>
101+
}
102+
103+
this.render(`<this.Demo />`, { Demo });
104+
105+
assert.ok(innerInstance, 'inner instance was captured');
106+
107+
runTask(() => this.$('button').click());
108+
109+
assert.strictEqual(
110+
seenThis,
111+
innerInstance,
112+
'`this` inside o.method should be the inner object instance'
113+
);
114+
assert.strictEqual(receivedArg, 'did it', 'argument from fn is passed through');
115+
}
73116
}
74117
);

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

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -56,17 +56,7 @@ EXPRESSIONS.add(SexpOpcodes.Curry, (op, [, expr, type, positional, named]) => {
5656

5757
EXPRESSIONS.add(SexpOpcodes.GetSymbol, (op, [, sym, path]) => {
5858
op(VM_GET_VARIABLE_OP, sym);
59-
60-
if (sym === 0 && path !== undefined && path.length > 0) {
61-
// For `this.*` paths, emit GetPropertyBound for the last segment so that
62-
// class methods preserve their `this` binding when passed as callbacks.
63-
for (let i = 0; i < path.length - 1; i++) {
64-
op(VM_GET_PROPERTY_OP, path[i]);
65-
}
66-
op(VM_GET_PROPERTY_BOUND_OP, path[path.length - 1]);
67-
} else {
68-
withPath(op, path);
69-
}
59+
withPath(op, path);
7060
});
7161

7262
EXPRESSIONS.add(SexpOpcodes.GetLexicalSymbol, (op, [, sym, path]) => {
@@ -95,9 +85,13 @@ EXPRESSIONS.add(SexpOpcodes.GetFreeAsHelperHead, (op, expr) => {
9585
function withPath(op: PushExpressionOp, path?: string[]) {
9686
if (path === undefined || path.length === 0) return;
9787

98-
for (let i = 0; i < path.length; i++) {
88+
for (let i = 0; i < path.length - 1; i++) {
9989
op(VM_GET_PROPERTY_OP, path[i]);
10090
}
91+
92+
// The last segment uses GetPropertyBound to tag the ref with its parent,
93+
// so that consumers (on, fn) can bind `this` at invocation time.
94+
op(VM_GET_PROPERTY_BOUND_OP, path[path.length - 1]);
10195
}
10296

10397
EXPRESSIONS.add(SexpOpcodes.Undefined, (op) => PushPrimitiveReference(op, undefined));

0 commit comments

Comments
 (0)