Skip to content

Commit df94070

Browse files
NullVoxPopuliclaude
andcommitted
Auto-bind prototype methods resolved via template path reads
When a template path like `this.foo` resolves to a prototype method on the parent (a class method), return a callable Proxy that binds `this` to the parent on invocation. Previously the raw unbound function was handed to consumers like `{{on "click" this.foo}}`, so the class method would fire with the wrong `this` (or trip the `on` modifier's DEBUG guardrail). The Proxy approach is used instead of `Function.prototype.bind` because `bind()` creates a new function object that loses the original's own properties, which would break subsequent traversals such as `this.func.aProp` where `func` happens to be a function carrying its own properties. The bound Proxy is cached per (parent, path) in a WeakMap so callback identity stays stable across revalidations — required by `{{on}}`, which only re-installs the DOM listener when the callback reference changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 1886f8d commit df94070

2 files changed

Lines changed: 118 additions & 2 deletions

File tree

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
import { moduleFor, RenderingTestCase, runTask } from 'internal-test-helpers';
2+
import { precompileTemplate } from '@ember/template-compilation';
3+
import { setComponentTemplate } from '@glimmer/manager';
4+
5+
import { Component } from '../../utils/helpers';
6+
7+
moduleFor(
8+
'{{on}} Modifier | this-binding for class methods',
9+
class extends RenderingTestCase {
10+
['@test a class method passed to {{on}} keeps its `this` context'](assert) {
11+
let instance;
12+
let seenThis = null;
13+
14+
let DemoComponent = class extends Component {
15+
init() {
16+
super.init(...arguments);
17+
instance = this;
18+
}
19+
20+
foo() {
21+
seenThis = this;
22+
}
23+
};
24+
25+
this.owner.register(
26+
'component:demo-el',
27+
setComponentTemplate(
28+
precompileTemplate('<button type="button" {{on "click" this.foo}}>click me</button>', {
29+
strictMode: false,
30+
}),
31+
DemoComponent
32+
)
33+
);
34+
35+
this.render('{{demo-el}}');
36+
37+
assert.ok(instance, 'component instance was captured');
38+
39+
runTask(() => this.$('button').click());
40+
41+
assert.strictEqual(
42+
seenThis,
43+
instance,
44+
'`this` inside the class method should be the component instance'
45+
);
46+
}
47+
}
48+
);

packages/@glimmer/reference/lib/reference.ts

Lines changed: 70 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,72 @@ export function updateRef(_ref: Reference, value: unknown) {
193193
update(value);
194194
}
195195

196+
// When a path read like `this.foo` resolves to a prototype method on the
197+
// parent, we want the consumer (e.g. `{{on "click" this.foo}}`) to see a
198+
// `this`-bound version, but we must NOT break subsequent property reads
199+
// (`this.func.aProp` where `func` is a function carrying its own properties).
200+
//
201+
// A `Function.prototype.bind` call creates a brand new function object that
202+
// loses the original's own properties, which breaks the latter case. So
203+
// instead we return a callable Proxy: invoking it binds `this` to the parent,
204+
// while property access is forwarded transparently to the original function.
205+
//
206+
// The result is cached on the parent (weakly) so that identity is stable
207+
// across revalidations — required by modifiers that diff callback identity
208+
// (e.g. `{{on}}` only re-installs the DOM listener when the callback ref
209+
// changes).
210+
type AnyFn = (...args: unknown[]) => unknown;
211+
212+
const BOUND_METHODS: WeakMap<object, Map<string, AnyFn>> = new WeakMap();
213+
214+
// True when `path` resolves on the parent's prototype chain to a function
215+
// declared as a class method. Class-body method shorthand (`foo() {}`) is
216+
// non-enumerable on the prototype, while functions assigned via object
217+
// literals (e.g. `Component.extend({ foo() {} })` or a plain context like
218+
// `{ foo() {} }`) end up enumerable. We only auto-bind the former so that
219+
// existing object-literal contracts (no implicit `this`) are preserved.
220+
function isClassMethod(parent: object, path: string): boolean {
221+
let proto: object | null = Object.getPrototypeOf(parent);
222+
while (proto !== null && proto !== Object.prototype && proto !== Function.prototype) {
223+
const desc = Object.getOwnPropertyDescriptor(proto, path);
224+
if (desc !== undefined) {
225+
return desc.enumerable === false && typeof desc.value === 'function';
226+
}
227+
proto = Object.getPrototypeOf(proto);
228+
}
229+
return false;
230+
}
231+
232+
function maybeBindPrototypeMethod(parent: object, path: string, value: unknown): unknown {
233+
if (
234+
typeof value !== 'function' ||
235+
parent === null ||
236+
(typeof parent !== 'object' && typeof parent !== 'function') ||
237+
Object.prototype.hasOwnProperty.call(parent, path) ||
238+
!isClassMethod(parent, path)
239+
) {
240+
return value;
241+
}
242+
243+
let cache = BOUND_METHODS.get(parent);
244+
if (cache === undefined) {
245+
cache = new Map();
246+
BOUND_METHODS.set(parent, cache);
247+
}
248+
249+
let bound = cache.get(path);
250+
if (bound === undefined) {
251+
const target = value as AnyFn;
252+
bound = new Proxy(target, {
253+
apply(fn, _thisArg, args) {
254+
return Reflect.apply(fn, parent, args);
255+
},
256+
});
257+
cache.set(path, bound);
258+
}
259+
return bound;
260+
}
261+
196262
export function childRefFor(_parentRef: Reference, path: string): Reference {
197263
const parentRef = _parentRef as ReferenceImpl;
198264

@@ -213,8 +279,9 @@ export function childRefFor(_parentRef: Reference, path: string): Reference {
213279
const parent = valueForRef(parentRef);
214280

215281
if (isDict(parent)) {
282+
const raw = (parent as Record<string, unknown>)[path];
216283
child = createUnboundRef(
217-
(parent as Record<string, unknown>)[path],
284+
maybeBindPrototypeMethod(parent, path, raw),
218285
DEBUG && `${parentRef.debugLabel}.${path}`
219286
);
220287
} else {
@@ -226,7 +293,8 @@ export function childRefFor(_parentRef: Reference, path: string): Reference {
226293
const parent = valueForRef(parentRef);
227294

228295
if (isDict(parent)) {
229-
return getProp(parent, path);
296+
const value = getProp(parent, path);
297+
return maybeBindPrototypeMethod(parent, path, value);
230298
}
231299
},
232300
(val) => {

0 commit comments

Comments
 (0)