From 96d5d1c54653d256ad9462ec5c95a4b11594b4d1 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Fri, 12 Jun 2026 15:56:30 -0400 Subject: [PATCH 1/4] fix plain method calling --- .../tests/integration/helpers/fn-test.js | 16 +- .../tests/integration/this-binding-test.gjs | 256 ++++++++++++++++++ .../integration-tests/test/helpers/fn-test.ts | 29 +- .../test/modifiers/on-test.ts | 19 +- packages/@glimmer/reference/index.ts | 1 + packages/@glimmer/reference/lib/reference.ts | 24 ++ packages/@glimmer/runtime/lib/helpers/fn.ts | 20 +- packages/@glimmer/runtime/lib/modifiers/on.ts | 51 ++-- 8 files changed, 363 insertions(+), 53 deletions(-) create mode 100644 packages/@ember/-internals/glimmer/tests/integration/this-binding-test.gjs diff --git a/packages/@ember/-internals/glimmer/tests/integration/helpers/fn-test.js b/packages/@ember/-internals/glimmer/tests/integration/helpers/fn-test.js index ae2e23add3e..38449eee7f6 100644 --- a/packages/@ember/-internals/glimmer/tests/integration/helpers/fn-test.js +++ b/packages/@ember/-internals/glimmer/tests/integration/helpers/fn-test.js @@ -1,5 +1,4 @@ import { set } from '@ember/object'; -import { DEBUG } from '@glimmer/env'; import { RenderingTestCase, moduleFor, runTask } from 'internal-test-helpers'; import { template } from '@ember/template-compiler/runtime'; import { Component } from '../../utils/helpers'; @@ -129,19 +128,22 @@ moduleFor( assert.equal(this.stashedFn(), 'arg1: foo, arg2: bar'); } - '@test there is no `this` context within the callback'(assert) { - if (DEBUG) { - assert.expect(0); - return; - } + '@test `this` within the callback is the object the method was read from'(assert) { + let seenThis; this.render(`{{stash stashedFn=(fn this.myFunc this.arg1)}}`, { myFunc() { - assert.strictEqual(this, null, 'this is bound to null in production builds'); + seenThis = this; }, }); this.stashedFn(); + + assert.strictEqual( + seenThis, + this.context, + 'this is bound to the object that myFunc was read from' + ); } '@test can use `this` if bound prior to passing to fn'(assert) { diff --git a/packages/@ember/-internals/glimmer/tests/integration/this-binding-test.gjs b/packages/@ember/-internals/glimmer/tests/integration/this-binding-test.gjs new file mode 100644 index 00000000000..a6b70462f82 --- /dev/null +++ b/packages/@ember/-internals/glimmer/tests/integration/this-binding-test.gjs @@ -0,0 +1,256 @@ +import { set } from '@ember/object'; +import { RenderingTestCase, moduleFor, runTask } from 'internal-test-helpers'; +import { fn } from '@ember/helper'; +import { on } from '@ember/modifier'; +import { Component } from '../utils/helpers'; + +moduleFor( + 'Path expression this-binding for class methods', + class extends RenderingTestCase { + ['@test this.foo maintains this binding'](assert) { + let instance; + let seenThis; + + class Demo extends Component { + constructor(...args) { + super(...args); + instance = this; + } + + foo() { + seenThis = this; + } + + + } + + this.render(``, { Demo }); + + assert.ok(instance, 'component instance was captured'); + + runTask(() => this.$('button').click()); + + assert.strictEqual( + seenThis, + instance, + '`this` inside the class method should be the component instance' + ); + } + + ['@test this.obj.method maintains this binding through property chain'](assert) { + let innerInstance; + let seenThis; + + class Inner { + constructor() { + innerInstance = this; + } + + method() { + seenThis = this; + } + } + + class Demo extends Component { + constructor(...args) { + super(...args); + this.obj = new Inner(); + } + + + } + + this.render(``, { Demo }); + + assert.ok(innerInstance, 'inner instance was captured'); + + runTask(() => this.$('button').click()); + + assert.strictEqual( + seenThis, + innerInstance, + '`this` inside the nested method should be the inner object instance' + ); + } + + ['@test replacing the base object rebinds `this` for subsequent invocations'](assert) { + let instance; + let seenThis; + + class Inner { + method() { + seenThis = this; + } + } + + class Demo extends Component { + constructor(...args) { + super(...args); + instance = this; + this.obj = new Inner(); + } + + + } + + this.render(``, { Demo }); + + let first = instance.obj; + + runTask(() => this.$('button').click()); + assert.strictEqual(seenThis, first, 'first click sees the original object'); + + let second = new Inner(); + runTask(() => set(instance, 'obj', second)); + + runTask(() => this.$('button').click()); + assert.strictEqual(seenThis, second, 'after the base object is replaced, `this` is fresh'); + } + + ['@test a method passed as an argument stays bound to its original object'](assert) { + let instance; + let seenThis; + + class Child extends Component { + + } + + class Demo extends Component { + constructor(...args) { + super(...args); + instance = this; + } + + foo() { + seenThis = this; + } + + + } + + this.render(``, { Demo }); + + runTask(() => this.$('button').click()); + + assert.strictEqual( + seenThis, + instance, + '`this` inside the method is the component the method was read from, not the child' + ); + } + + ['@test #let block param preserves this binding on method access'](assert) { + let innerInstance; + let seenThis; + let receivedArg; + + class Inner { + constructor() { + innerInstance = this; + } + + method(arg) { + seenThis = this; + receivedArg = arg; + } + } + + class Demo extends Component { + constructor(...args) { + super(...args); + this.obj = new Inner(); + } + + + } + + this.render(``, { Demo }); + + assert.ok(innerInstance, 'inner instance was captured'); + + runTask(() => this.$('button').click()); + + assert.strictEqual( + seenThis, + innerInstance, + '`this` inside o.method should be the inner object instance' + ); + assert.strictEqual(receivedArg, 'did it', 'argument from fn is passed through'); + } + + ['@test each over array of objects preserves this binding on item methods'](assert) { + let seenPairs = []; + + class Item { + constructor(name) { + this.name = name; + } + + greet() { + seenPairs.push({ self: this, name: this.name }); + } + } + + let items = [new Item('alice'), new Item('bob'), new Item('carol')]; + + class Demo extends Component { + constructor(...args) { + super(...args); + this.items = items; + } + + + } + + this.render(``, { Demo }); + + runTask(() => this.$('button.alice').click()); + runTask(() => this.$('button.bob').click()); + runTask(() => this.$('button.carol').click()); + + assert.strictEqual(seenPairs.length, 3, 'all three handlers fired'); + assert.strictEqual(seenPairs[0].self, items[0], 'alice: this is the Item instance'); + assert.strictEqual(seenPairs[0].name, 'alice', 'alice: this.name is correct'); + assert.strictEqual(seenPairs[1].self, items[1], 'bob: this is the Item instance'); + assert.strictEqual(seenPairs[1].name, 'bob', 'bob: this.name is correct'); + assert.strictEqual(seenPairs[2].self, items[2], 'carol: this is the Item instance'); + assert.strictEqual(seenPairs[2].name, 'carol', 'carol: this.name is correct'); + } + + ['@test already-bound functions are unaffected'](assert) { + let instance; + let seenThis; + + class Demo extends Component { + constructor(...args) { + super(...args); + instance = this; + } + + foo = () => { + seenThis = this; + }; + + + } + + this.render(``, { Demo }); + + runTask(() => this.$('button').click()); + + assert.strictEqual( + seenThis, + instance, + 'an arrow function field still sees the component instance' + ); + } + } +); diff --git a/packages/@glimmer-workspace/integration-tests/test/helpers/fn-test.ts b/packages/@glimmer-workspace/integration-tests/test/helpers/fn-test.ts index e3ae8ebded6..4b1ec97e6a4 100644 --- a/packages/@glimmer-workspace/integration-tests/test/helpers/fn-test.ts +++ b/packages/@glimmer-workspace/integration-tests/test/helpers/fn-test.ts @@ -159,8 +159,8 @@ class FnTest extends RenderTest { }, /You must pass a function as the `fn` helper's first argument, you passed null. While rendering:\n{2}this.myFunc/u); } - @test({ skip: !DEBUG }) - 'asserts if the provided function accesses `this` without being bound prior to passing to fn'( + @test + 'an unbound method passed to fn is invoked with the object it was read from as `this`'( assert: Assert ) { this.render(``, { @@ -172,24 +172,33 @@ class FnTest extends RenderTest { arg2: 'bar', }); - assert.throws(() => { - this.stashedFn?.(); - }, /You accessed `this.arg2` from a function passed to the `fn` helper, but the function itself was not bound to a valid `this` context. Consider updating to use a bound function./u); + assert.strictEqual( + this.stashedFn?.(), + 'arg1: foo, arg2: bar', + 'this.myFunc preserves its `this` through fn' + ); } - @test({ skip: !DEBUG }) - 'there is no `this` context within the callback'(assert: Assert) { + @test + '`this` within the callback is the object the method was read from'(assert: Assert) { + let seenThis: unknown; + this.render(``, { myFunc() { assert.step('calling stashed function'); - // eslint-disable-next-line @typescript-eslint/no-base-to-string - assert.throws(() => String(this), /not bound to a valid `this` context/u); - // assert.strictEqual(this, null, 'this is bound to null in production builds'); + seenThis = this; }, + + arg1: 'foo', }); this.stashedFn?.(); assert.verifySteps(['calling stashed function']); + assert.strictEqual( + (seenThis as Record | undefined)?.['arg1'], + 'foo', + 'this is the object myFunc was read from' + ); } @test diff --git a/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts b/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts index e67965b6027..b72e3e6bdc0 100644 --- a/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts +++ b/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts @@ -366,21 +366,26 @@ if (hasDom) { }, /You must pass a function as the second argument to the `on` modifier; you passed null. While rendering:\n{2}this.foo/u); } - @test({ skip: !DEBUG }) - 'asserts if the provided callback accesses `this` without being bound prior to passing to on'( - assert: Assert - ) { + @test + 'an unbound method is invoked with the object it was read from as `this`'(assert: Assert) { + let seenThis: unknown; + this.render(``, { myFunc(this: any) { - assert.throws(() => { - consume(this.arg1); - }, /You accessed `this.arg1` from a function passed to the `on` modifier, but the function itself was not bound to a valid `this` context. Consider updating to use a bound function/u); + seenThis = this; + consume(this.arg1); }, arg1: 'foo', }); this.findButton().click(); + + assert.strictEqual( + (seenThis as Record | undefined)?.['arg1'], + 'foo', + '`this` is the object myFunc was read from' + ); } @test({ skip: !DEBUG }) diff --git a/packages/@glimmer/reference/index.ts b/packages/@glimmer/reference/index.ts index 176cf42353e..77f01ea9cc2 100644 --- a/packages/@glimmer/reference/index.ts +++ b/packages/@glimmer/reference/index.ts @@ -22,6 +22,7 @@ export { isInvokableRef, isUpdatableRef, NULL_REFERENCE, + parentRefFor, REFERENCE, type Reference, type ReferenceEnvironment, diff --git a/packages/@glimmer/reference/lib/reference.ts b/packages/@glimmer/reference/lib/reference.ts index c7232d0469a..2c57257a73f 100644 --- a/packages/@glimmer/reference/lib/reference.ts +++ b/packages/@glimmer/reference/lib/reference.ts @@ -42,6 +42,12 @@ class ReferenceImpl implements Reference { public children: Nullable> = null; + /** + * The reference this one was created from via a property read (`parent.path`), + * if any. See {@linkcode parentRefFor}. + */ + public parent: Nullable = null; + public compute: Nullable<() => T> = null; public update: Nullable<(val: T) => void> = null; @@ -238,11 +244,27 @@ export function childRefFor(_parentRef: Reference, path: string): Reference { } } + if (child !== UNDEFINED_REFERENCE) { + (child as ReferenceImpl).parent = parentRef; + } + children.set(path, child); return child; } +/** + * Returns the reference for the object a property was read from, when the + * given reference was created by a property read (e.g. the `obj` in `obj.someFn`). + * + * This allows consumers that invoke function values, like the `on` modifier and + * the `fn` helper, to call the function with the same `this` JavaScript would + * use for `obj.someFn()`, rather than requiring the function to be pre-bound. + */ +export function parentRefFor(_ref: Reference): Nullable { + return (_ref as ReferenceImpl).parent; +} + export function childRefFromParts(root: Reference, parts: string[]): Reference { let reference = root; @@ -262,6 +284,8 @@ if (DEBUG) { ref[REFERENCE] = inner[REFERENCE]; + (ref as ReferenceImpl).parent = (inner as ReferenceImpl).parent; + ref.debugLabel = debugLabel; return ref; diff --git a/packages/@glimmer/runtime/lib/helpers/fn.ts b/packages/@glimmer/runtime/lib/helpers/fn.ts index a65d49976e1..cfeec0ebb95 100644 --- a/packages/@glimmer/runtime/lib/helpers/fn.ts +++ b/packages/@glimmer/runtime/lib/helpers/fn.ts @@ -6,6 +6,7 @@ import buildUntouchableThis from '@glimmer/debug-util/lib/untouchable-this'; import { createComputeRef, isInvokableRef, + parentRefFor, updateRef, valueForRef, } from '@glimmer/reference/lib/reference'; @@ -60,8 +61,8 @@ const context = buildUntouchableThis('`fn` helper'); string `'foo'` as its second argument. In the example above, we used an arrow function to ensure that - `handleSelected` is properly bound to the `items-list`, but let's explore what - happens if we left out the arrow function: + `handleSelected` is properly bound to the `items-list`, but a regular method + works as well: ```app/components/items-list.gjs import Component from '@glimmer/component'; @@ -73,10 +74,10 @@ const context = buildUntouchableThis('`fn` helper'); } ``` - In this example, when `handleSelected` is invoked inside the `display-item` - component, it will **not** have access to the component instance. In other - words, it will have no `this` context, so please make sure your functions - are bound (via an arrow function or other means) before passing into `fn`! + When the function passed to `fn` is a path like `this.handleSelected`, it is + invoked with the object it was read from as its `this` — the same `this` + JavaScript would use for `this.handleSelected(item)`. Functions that are + already bound (arrow functions, `.bind()`ed functions) are unaffected. See also [partial application](https://en.wikipedia.org/wiki/Partial_application). @@ -99,8 +100,13 @@ export const fn = internalHelper(({ positional }: CapturedArguments) => { let value = args.length > 0 ? args[0] : invocationArgs[0]; return void updateRef(callbackRef, value); } else { + // When the callback was a property read (`this.foo`, `item.greet`, ...), + // invoke it with the same `this` JavaScript would use for `obj.foo()`. + let parentRef = parentRefFor(callbackRef); + let self = parentRef !== null ? valueForRef(parentRef) : context; + // eslint-disable-next-line @typescript-eslint/no-unsafe-return -- @fixme - return (fn as AnyFn).call(context, ...args, ...invocationArgs); + return (fn as AnyFn).call(self, ...args, ...invocationArgs); } }; }, diff --git a/packages/@glimmer/runtime/lib/modifiers/on.ts b/packages/@glimmer/runtime/lib/modifiers/on.ts index 885dc0b9d45..5ef4dad48dc 100644 --- a/packages/@glimmer/runtime/lib/modifiers/on.ts +++ b/packages/@glimmer/runtime/lib/modifiers/on.ts @@ -17,7 +17,7 @@ import { import buildUntouchableThis from '@glimmer/debug-util/lib/untouchable-this'; import { registerDestructor } from '@glimmer/destroyable'; import { setInternalModifierManager } from '@glimmer/manager/lib/internal/api'; -import { valueForRef } from '@glimmer/reference/lib/reference'; +import { parentRefFor, valueForRef } from '@glimmer/reference/lib/reference'; import { createUpdatableTag } from '@glimmer/validator/lib/validators'; import { reifyNamed } from '../vm/arguments'; @@ -183,24 +183,32 @@ export class OnModifierState { if (shouldUpdate) { let callback = userProvidedCallback; - if (DEBUG) { - callback = userProvidedCallback.bind(untouchableContext); + // When the callback was a property read (`this.foo`, `item.greet`, ...), + // invoke it with the same `this` JavaScript would use for `obj.foo()`. + // The parent is read lazily per-event so a swapped parent object is + // never invoked with a stale `this`. + let parentRef = arg1 ? parentRefFor(arg1) : null; - if (passive) { - let _callback = callback; + if (parentRef !== null) { + callback = (event) => userProvidedCallback.call(valueForRef(parentRef), event); + } else if (DEBUG) { + callback = userProvidedCallback.bind(untouchableContext); + } - callback = (event) => { - event.preventDefault = () => { - throw new Error( - `You marked this listener as 'passive', meaning that you must not call 'event.preventDefault()': \n\n${ - userProvidedCallback.name || `{anonymous function}` - }` - ); - }; + if (DEBUG && passive) { + let _callback = callback; - return _callback(event); + callback = (event) => { + event.preventDefault = () => { + throw new Error( + `You marked this listener as 'passive', meaning that you must not call 'event.preventDefault()': \n\n${ + userProvidedCallback.name || `{anonymous function}` + }` + ); }; - } + + return _callback(event); + }; } this.listener = { @@ -310,9 +318,8 @@ function addEventListener( ### Function Context - In the example above, we used an arrow function to ensure that `likePost` is - properly bound to the `items-list`, but let's explore what happens if we - left out the arrow function: + In the example above, we used an arrow function to ensure that `saveLike` is + properly bound to the component, but a regular method works as well: ```app/components/like-post.gjs import Component from '@glimmer/component'; @@ -324,10 +331,10 @@ function addEventListener( } ``` - In this example, when the button is clicked `saveLike` will be invoked, - it will **not** have access to the component instance. In other - words, it will have no `this` context, so please make sure your functions - are bound (via an arrow function or other means) before passing into `on`! + When the callback passed to `on` is a path like `this.saveLike`, it is + invoked with the object it was read from as its `this` — the same `this` + JavaScript would use for `this.saveLike()`. Functions that are already + bound (arrow functions, `.bind()`ed functions) are unaffected. @method on @public From c1574265d47d7a1c093ea5e1f06a061334464328 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Tue, 16 Jun 2026 14:28:44 -0400 Subject: [PATCH 2/4] Revert this-binding changes for (fn) --- .../tests/integration/helpers/fn-test.js | 16 +++++----- .../integration-tests/test/helpers/fn-test.ts | 29 +++++++------------ packages/@glimmer/runtime/lib/helpers/fn.ts | 20 +++++-------- 3 files changed, 24 insertions(+), 41 deletions(-) diff --git a/packages/@ember/-internals/glimmer/tests/integration/helpers/fn-test.js b/packages/@ember/-internals/glimmer/tests/integration/helpers/fn-test.js index 38449eee7f6..ae2e23add3e 100644 --- a/packages/@ember/-internals/glimmer/tests/integration/helpers/fn-test.js +++ b/packages/@ember/-internals/glimmer/tests/integration/helpers/fn-test.js @@ -1,4 +1,5 @@ import { set } from '@ember/object'; +import { DEBUG } from '@glimmer/env'; import { RenderingTestCase, moduleFor, runTask } from 'internal-test-helpers'; import { template } from '@ember/template-compiler/runtime'; import { Component } from '../../utils/helpers'; @@ -128,22 +129,19 @@ moduleFor( assert.equal(this.stashedFn(), 'arg1: foo, arg2: bar'); } - '@test `this` within the callback is the object the method was read from'(assert) { - let seenThis; + '@test there is no `this` context within the callback'(assert) { + if (DEBUG) { + assert.expect(0); + return; + } this.render(`{{stash stashedFn=(fn this.myFunc this.arg1)}}`, { myFunc() { - seenThis = this; + assert.strictEqual(this, null, 'this is bound to null in production builds'); }, }); this.stashedFn(); - - assert.strictEqual( - seenThis, - this.context, - 'this is bound to the object that myFunc was read from' - ); } '@test can use `this` if bound prior to passing to fn'(assert) { diff --git a/packages/@glimmer-workspace/integration-tests/test/helpers/fn-test.ts b/packages/@glimmer-workspace/integration-tests/test/helpers/fn-test.ts index 4b1ec97e6a4..e3ae8ebded6 100644 --- a/packages/@glimmer-workspace/integration-tests/test/helpers/fn-test.ts +++ b/packages/@glimmer-workspace/integration-tests/test/helpers/fn-test.ts @@ -159,8 +159,8 @@ class FnTest extends RenderTest { }, /You must pass a function as the `fn` helper's first argument, you passed null. While rendering:\n{2}this.myFunc/u); } - @test - 'an unbound method passed to fn is invoked with the object it was read from as `this`'( + @test({ skip: !DEBUG }) + 'asserts if the provided function accesses `this` without being bound prior to passing to fn'( assert: Assert ) { this.render(``, { @@ -172,33 +172,24 @@ class FnTest extends RenderTest { arg2: 'bar', }); - assert.strictEqual( - this.stashedFn?.(), - 'arg1: foo, arg2: bar', - 'this.myFunc preserves its `this` through fn' - ); + assert.throws(() => { + this.stashedFn?.(); + }, /You accessed `this.arg2` from a function passed to the `fn` helper, but the function itself was not bound to a valid `this` context. Consider updating to use a bound function./u); } - @test - '`this` within the callback is the object the method was read from'(assert: Assert) { - let seenThis: unknown; - + @test({ skip: !DEBUG }) + 'there is no `this` context within the callback'(assert: Assert) { this.render(``, { myFunc() { assert.step('calling stashed function'); - seenThis = this; + // eslint-disable-next-line @typescript-eslint/no-base-to-string + assert.throws(() => String(this), /not bound to a valid `this` context/u); + // assert.strictEqual(this, null, 'this is bound to null in production builds'); }, - - arg1: 'foo', }); this.stashedFn?.(); assert.verifySteps(['calling stashed function']); - assert.strictEqual( - (seenThis as Record | undefined)?.['arg1'], - 'foo', - 'this is the object myFunc was read from' - ); } @test diff --git a/packages/@glimmer/runtime/lib/helpers/fn.ts b/packages/@glimmer/runtime/lib/helpers/fn.ts index cfeec0ebb95..a65d49976e1 100644 --- a/packages/@glimmer/runtime/lib/helpers/fn.ts +++ b/packages/@glimmer/runtime/lib/helpers/fn.ts @@ -6,7 +6,6 @@ import buildUntouchableThis from '@glimmer/debug-util/lib/untouchable-this'; import { createComputeRef, isInvokableRef, - parentRefFor, updateRef, valueForRef, } from '@glimmer/reference/lib/reference'; @@ -61,8 +60,8 @@ const context = buildUntouchableThis('`fn` helper'); string `'foo'` as its second argument. In the example above, we used an arrow function to ensure that - `handleSelected` is properly bound to the `items-list`, but a regular method - works as well: + `handleSelected` is properly bound to the `items-list`, but let's explore what + happens if we left out the arrow function: ```app/components/items-list.gjs import Component from '@glimmer/component'; @@ -74,10 +73,10 @@ const context = buildUntouchableThis('`fn` helper'); } ``` - When the function passed to `fn` is a path like `this.handleSelected`, it is - invoked with the object it was read from as its `this` — the same `this` - JavaScript would use for `this.handleSelected(item)`. Functions that are - already bound (arrow functions, `.bind()`ed functions) are unaffected. + In this example, when `handleSelected` is invoked inside the `display-item` + component, it will **not** have access to the component instance. In other + words, it will have no `this` context, so please make sure your functions + are bound (via an arrow function or other means) before passing into `fn`! See also [partial application](https://en.wikipedia.org/wiki/Partial_application). @@ -100,13 +99,8 @@ export const fn = internalHelper(({ positional }: CapturedArguments) => { let value = args.length > 0 ? args[0] : invocationArgs[0]; return void updateRef(callbackRef, value); } else { - // When the callback was a property read (`this.foo`, `item.greet`, ...), - // invoke it with the same `this` JavaScript would use for `obj.foo()`. - let parentRef = parentRefFor(callbackRef); - let self = parentRef !== null ? valueForRef(parentRef) : context; - // eslint-disable-next-line @typescript-eslint/no-unsafe-return -- @fixme - return (fn as AnyFn).call(self, ...args, ...invocationArgs); + return (fn as AnyFn).call(context, ...args, ...invocationArgs); } }; }, From c41a0c7b6a6929ad2feaa34e01bc46593f5f9159 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Tue, 16 Jun 2026 14:29:01 -0400 Subject: [PATCH 3/4] Revert this-binding changes for {{on}} --- .../test/modifiers/on-test.ts | 19 +++---- packages/@glimmer/runtime/lib/modifiers/on.ts | 51 ++++++++----------- 2 files changed, 29 insertions(+), 41 deletions(-) diff --git a/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts b/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts index b72e3e6bdc0..e67965b6027 100644 --- a/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts +++ b/packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts @@ -366,26 +366,21 @@ if (hasDom) { }, /You must pass a function as the second argument to the `on` modifier; you passed null. While rendering:\n{2}this.foo/u); } - @test - 'an unbound method is invoked with the object it was read from as `this`'(assert: Assert) { - let seenThis: unknown; - + @test({ skip: !DEBUG }) + 'asserts if the provided callback accesses `this` without being bound prior to passing to on'( + assert: Assert + ) { this.render(``, { myFunc(this: any) { - seenThis = this; - consume(this.arg1); + assert.throws(() => { + consume(this.arg1); + }, /You accessed `this.arg1` from a function passed to the `on` modifier, but the function itself was not bound to a valid `this` context. Consider updating to use a bound function/u); }, arg1: 'foo', }); this.findButton().click(); - - assert.strictEqual( - (seenThis as Record | undefined)?.['arg1'], - 'foo', - '`this` is the object myFunc was read from' - ); } @test({ skip: !DEBUG }) diff --git a/packages/@glimmer/runtime/lib/modifiers/on.ts b/packages/@glimmer/runtime/lib/modifiers/on.ts index 5ef4dad48dc..885dc0b9d45 100644 --- a/packages/@glimmer/runtime/lib/modifiers/on.ts +++ b/packages/@glimmer/runtime/lib/modifiers/on.ts @@ -17,7 +17,7 @@ import { import buildUntouchableThis from '@glimmer/debug-util/lib/untouchable-this'; import { registerDestructor } from '@glimmer/destroyable'; import { setInternalModifierManager } from '@glimmer/manager/lib/internal/api'; -import { parentRefFor, valueForRef } from '@glimmer/reference/lib/reference'; +import { valueForRef } from '@glimmer/reference/lib/reference'; import { createUpdatableTag } from '@glimmer/validator/lib/validators'; import { reifyNamed } from '../vm/arguments'; @@ -183,32 +183,24 @@ export class OnModifierState { if (shouldUpdate) { let callback = userProvidedCallback; - // When the callback was a property read (`this.foo`, `item.greet`, ...), - // invoke it with the same `this` JavaScript would use for `obj.foo()`. - // The parent is read lazily per-event so a swapped parent object is - // never invoked with a stale `this`. - let parentRef = arg1 ? parentRefFor(arg1) : null; - - if (parentRef !== null) { - callback = (event) => userProvidedCallback.call(valueForRef(parentRef), event); - } else if (DEBUG) { + if (DEBUG) { callback = userProvidedCallback.bind(untouchableContext); - } - if (DEBUG && passive) { - let _callback = callback; + if (passive) { + let _callback = callback; - callback = (event) => { - event.preventDefault = () => { - throw new Error( - `You marked this listener as 'passive', meaning that you must not call 'event.preventDefault()': \n\n${ - userProvidedCallback.name || `{anonymous function}` - }` - ); - }; + callback = (event) => { + event.preventDefault = () => { + throw new Error( + `You marked this listener as 'passive', meaning that you must not call 'event.preventDefault()': \n\n${ + userProvidedCallback.name || `{anonymous function}` + }` + ); + }; - return _callback(event); - }; + return _callback(event); + }; + } } this.listener = { @@ -318,8 +310,9 @@ function addEventListener( ### Function Context - In the example above, we used an arrow function to ensure that `saveLike` is - properly bound to the component, but a regular method works as well: + In the example above, we used an arrow function to ensure that `likePost` is + properly bound to the `items-list`, but let's explore what happens if we + left out the arrow function: ```app/components/like-post.gjs import Component from '@glimmer/component'; @@ -331,10 +324,10 @@ function addEventListener( } ``` - When the callback passed to `on` is a path like `this.saveLike`, it is - invoked with the object it was read from as its `this` — the same `this` - JavaScript would use for `this.saveLike()`. Functions that are already - bound (arrow functions, `.bind()`ed functions) are unaffected. + In this example, when the button is clicked `saveLike` will be invoked, + it will **not** have access to the component instance. In other + words, it will have no `this` context, so please make sure your functions + are bound (via an arrow function or other means) before passing into `on`! @method on @public From 9c29b5100ee14a63dd1eb1090fe10d38bd58c43a Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Tue, 16 Jun 2026 16:53:47 -0400 Subject: [PATCH 4/4] *actually* use JS bind semantics, and don't attempt to fix the problems of the language --- .../tests/integration/this-binding-test.gjs | 194 ++++++++++++------ .../@glimmer/interfaces/lib/references.d.ts | 1 + packages/@glimmer/manager/lib/internal/api.ts | 10 + packages/@glimmer/reference/index.ts | 1 - packages/@glimmer/reference/lib/reference.ts | 12 +- .../lib/compiled/opcodes/expressions.ts | 45 +++- 6 files changed, 192 insertions(+), 71 deletions(-) diff --git a/packages/@ember/-internals/glimmer/tests/integration/this-binding-test.gjs b/packages/@ember/-internals/glimmer/tests/integration/this-binding-test.gjs index a6b70462f82..05e77c7664b 100644 --- a/packages/@ember/-internals/glimmer/tests/integration/this-binding-test.gjs +++ b/packages/@ember/-internals/glimmer/tests/integration/this-binding-test.gjs @@ -1,13 +1,13 @@ import { set } from '@ember/object'; import { RenderingTestCase, moduleFor, runTask } from 'internal-test-helpers'; import { fn } from '@ember/helper'; -import { on } from '@ember/modifier'; +import { helperCapabilities, setHelperManager } from '@glimmer/manager'; import { Component } from '../utils/helpers'; moduleFor( - 'Path expression this-binding for class methods', + 'Automatic this-binding for class methods directly called from the template', class extends RenderingTestCase { - ['@test this.foo maintains this binding'](assert) { + ['@test class method'](assert) { let instance; let seenThis; @@ -21,23 +21,16 @@ moduleFor( seenThis = this; } - + } this.render(``, { Demo }); - assert.ok(instance, 'component instance was captured'); - - runTask(() => this.$('button').click()); - - assert.strictEqual( - seenThis, - instance, - '`this` inside the class method should be the component instance' - ); + assert.ok(instance); + assert.strictEqual(seenThis, instance); } - ['@test this.obj.method maintains this binding through property chain'](assert) { + ['@test maintains binding through property chain'](assert) { let innerInstance; let seenThis; @@ -57,23 +50,16 @@ moduleFor( this.obj = new Inner(); } - + } this.render(``, { Demo }); - assert.ok(innerInstance, 'inner instance was captured'); - - runTask(() => this.$('button').click()); - - assert.strictEqual( - seenThis, - innerInstance, - '`this` inside the nested method should be the inner object instance' - ); + assert.ok(innerInstance); + assert.strictEqual(seenThis, innerInstance); } - ['@test replacing the base object rebinds `this` for subsequent invocations'](assert) { + ['@test replacing the base object uses correct `this`'](assert) { let instance; let seenThis; @@ -90,21 +76,18 @@ moduleFor( this.obj = new Inner(); } - + } this.render(``, { Demo }); let first = instance.obj; - - runTask(() => this.$('button').click()); - assert.strictEqual(seenThis, first, 'first click sees the original object'); + assert.strictEqual(seenThis, first); let second = new Inner(); runTask(() => set(instance, 'obj', second)); - runTask(() => this.$('button').click()); - assert.strictEqual(seenThis, second, 'after the base object is replaced, `this` is fresh'); + assert.strictEqual(seenThis, second); } ['@test a method passed as an argument stays bound to its original object'](assert) { @@ -112,7 +95,7 @@ moduleFor( let seenThis; class Child extends Component { - + } class Demo extends Component { @@ -130,13 +113,7 @@ moduleFor( this.render(``, { Demo }); - runTask(() => this.$('button').click()); - - assert.strictEqual( - seenThis, - instance, - '`this` inside the method is the component the method was read from, not the child' - ); + assert.strictEqual(seenThis, instance); } ['@test #let block param preserves this binding on method access'](assert) { @@ -163,23 +140,16 @@ moduleFor( } this.render(``, { Demo }); - assert.ok(innerInstance, 'inner instance was captured'); - - runTask(() => this.$('button').click()); - - assert.strictEqual( - seenThis, - innerInstance, - '`this` inside o.method should be the inner object instance' - ); - assert.strictEqual(receivedArg, 'did it', 'argument from fn is passed through'); + assert.ok(innerInstance); + assert.strictEqual(seenThis, innerInstance); + assert.strictEqual(receivedArg, 'did it', 'argument is passed through to the method'); } ['@test each over array of objects preserves this binding on item methods'](assert) { @@ -204,19 +174,17 @@ moduleFor( } } this.render(``, { Demo }); - runTask(() => this.$('button.alice').click()); - runTask(() => this.$('button.bob').click()); - runTask(() => this.$('button.carol').click()); - - assert.strictEqual(seenPairs.length, 3, 'all three handlers fired'); + assert.strictEqual(seenPairs.length, 3); assert.strictEqual(seenPairs[0].self, items[0], 'alice: this is the Item instance'); assert.strictEqual(seenPairs[0].name, 'alice', 'alice: this.name is correct'); assert.strictEqual(seenPairs[1].self, items[1], 'bob: this is the Item instance'); @@ -239,17 +207,119 @@ moduleFor( seenThis = this; }; - + + } + + this.render(``, { Demo }); + + assert.strictEqual(seenThis, instance); + } + + ['@test (fn) on methods still behaves appropriately'](assert) { + let instance; + let seenThis; + + class Demo extends Component { + constructor(...args) { + super(...args); + instance = this; + } + + foo = () => { + seenThis = this; + }; + + } this.render(``, { Demo }); - runTask(() => this.$('button').click()); + assert.strictEqual(seenThis, instance); + } + + ['@test a function with a custom helper manager read off a path keeps its manager'](assert) { + let sawDefinition; + + class MyHelperManager { + capabilities = helperCapabilities('3.23', { hasValue: true }); + createHelper(definition, args) { + return { definition, args }; + } + + getValue({ definition }) { + sawDefinition = definition; + return 'CUSTOM_MANAGER_RAN'; + } + + getDebugName() { + return 'my-custom-helper'; + } + } + + function customHelper() { + return 'PLAIN_FN_RAN'; + } + setHelperManager(() => new MyHelperManager(), customHelper); + + class Inner { + customHelper = customHelper; + } + + class Demo extends Component { + constructor(...args) { + super(...args); + this.obj = new Inner(); + } + + + } + + this.render(``, { Demo }); + + this.assertText('CUSTOM_MANAGER_RAN'); assert.strictEqual( + sawDefinition, + customHelper, + // i.e.: a.foo !== a.foo.bind(a) + 'the custom manager received the original function, not a `.bind()` wrapper' + ); + } + + ['@test a plain method passed through (fn) is not this-bound'](assert) { + let innerInstance; + let seenThis = 'UNSET'; + + class Inner { + constructor() { + innerInstance = this; + } + + method() { + seenThis = this; + } + } + + class Demo extends Component { + constructor(...args) { + super(...args); + this.obj = new Inner(); + } + + + } + + this.render(``, { Demo }); + + assert.notStrictEqual(seenThis, 'UNSET', 'the method actually ran'); + assert.notStrictEqual( seenThis, - instance, - 'an arrow function field still sees the component instance' + innerInstance, + 'a plain method passed through `(fn)` is NOT bound to the object it was read from — `(fn)` value-passes the function and only the direct template call site binds' ); } } diff --git a/packages/@glimmer/interfaces/lib/references.d.ts b/packages/@glimmer/interfaces/lib/references.d.ts index 9bb1415eb57..86abdaa9d39 100644 --- a/packages/@glimmer/interfaces/lib/references.d.ts +++ b/packages/@glimmer/interfaces/lib/references.d.ts @@ -26,4 +26,5 @@ export interface Reference { debugLabel?: string | false | undefined; compute: Nullable<() => T>; children: null | Map; + parent: Nullable; } diff --git a/packages/@glimmer/manager/lib/internal/api.ts b/packages/@glimmer/manager/lib/internal/api.ts index c5b5135e1ae..bf077812c93 100644 --- a/packages/@glimmer/manager/lib/internal/api.ts +++ b/packages/@glimmer/manager/lib/internal/api.ts @@ -235,6 +235,16 @@ export function hasInternalModifierManager(definition: object): boolean { ); } +/** + * Whether a value has a helper manager explicitly associated with it, as + * opposed to a plain function (which falls back to the default function helper + * manager). Used to decide whether a function invoked from a path expression + * may be safely re-bound to the object it was read from. + */ +export function hasCustomHelperManager(definition: object): boolean { + return getManager(HELPER_MANAGERS, definition) !== undefined; +} + function hasDefaultComponentManager(_definition: object): boolean { return false; } diff --git a/packages/@glimmer/reference/index.ts b/packages/@glimmer/reference/index.ts index 77f01ea9cc2..176cf42353e 100644 --- a/packages/@glimmer/reference/index.ts +++ b/packages/@glimmer/reference/index.ts @@ -22,7 +22,6 @@ export { isInvokableRef, isUpdatableRef, NULL_REFERENCE, - parentRefFor, REFERENCE, type Reference, type ReferenceEnvironment, diff --git a/packages/@glimmer/reference/lib/reference.ts b/packages/@glimmer/reference/lib/reference.ts index 2c57257a73f..6e0aa154994 100644 --- a/packages/@glimmer/reference/lib/reference.ts +++ b/packages/@glimmer/reference/lib/reference.ts @@ -245,7 +245,7 @@ export function childRefFor(_parentRef: Reference, path: string): Reference { } if (child !== UNDEFINED_REFERENCE) { - (child as ReferenceImpl).parent = parentRef; + child.parent = parentRef; } children.set(path, child); @@ -257,12 +257,12 @@ export function childRefFor(_parentRef: Reference, path: string): Reference { * Returns the reference for the object a property was read from, when the * given reference was created by a property read (e.g. the `obj` in `obj.someFn`). * - * This allows consumers that invoke function values, like the `on` modifier and - * the `fn` helper, to call the function with the same `this` JavaScript would + * This allows a function value invoked directly from a template (e.g. + * `{{(this.obj.someFn)}}`) to be called with the same `this` JavaScript would * use for `obj.someFn()`, rather than requiring the function to be pre-bound. */ -export function parentRefFor(_ref: Reference): Nullable { - return (_ref as ReferenceImpl).parent; +export function parentRefFor(ref: Reference): Nullable { + return ref.parent; } export function childRefFromParts(root: Reference, parts: string[]): Reference { @@ -284,7 +284,7 @@ if (DEBUG) { ref[REFERENCE] = inner[REFERENCE]; - (ref as ReferenceImpl).parent = (inner as ReferenceImpl).parent; + ref.parent = inner.parent; ref.debugLabel = debugLabel; diff --git a/packages/@glimmer/runtime/lib/compiled/opcodes/expressions.ts b/packages/@glimmer/runtime/lib/compiled/opcodes/expressions.ts index 0e99c73c52f..a04367e5e15 100644 --- a/packages/@glimmer/runtime/lib/compiled/opcodes/expressions.ts +++ b/packages/@glimmer/runtime/lib/compiled/opcodes/expressions.ts @@ -41,11 +41,15 @@ import debugToString from '@glimmer/debug-util/lib/debug-to-string'; import assert from '@glimmer/debug-util/lib/assert'; import { _hasDestroyableChildren, associateDestroyableChild, destroy } from '@glimmer/destroyable'; import { debugAssert, toBool } from '@glimmer/global-context'; -import { getInternalHelperManager } from '@glimmer/manager/lib/internal/api'; +import { + getInternalHelperManager, + hasCustomHelperManager, +} from '@glimmer/manager/lib/internal/api'; import { childRefFor, createComputeRef, FALSE_REFERENCE, + parentRefFor, TRUE_REFERENCE, UNDEFINED_REFERENCE, valueForRef, @@ -125,7 +129,7 @@ APPEND_OPCODES.add(VM_DYNAMIC_HELPER_OP, (vm) => { associateDestroyableChild(helperInstanceRef, helperRef); } else if (isIndexable(definition)) { - let helper = resolveHelper(definition, ref); + let helper = resolveHelper(bindFunctionToParent(definition, ref), ref); helperRef = helper(args, initialOwner); if (_hasDestroyableChildren(helperRef)) { @@ -146,6 +150,43 @@ APPEND_OPCODES.add(VM_DYNAMIC_HELPER_OP, (vm) => { vm.loadValue($v0, helperValueRef); }); +/** + * When a plain function is invoked from a path expression (`{{(this.obj.method)}}`, + * `{{item.greet}}`), call it with the object it was read from as `this`, matching + * the JavaScript semantics of `obj.method()`. + * + * This is the template-invocation call site, so it only affects functions that are + * actually *invoked* here. Functions merely passed along as values (e.g. to the + * `{{on}}` modifier or the `(fn)` helper) flow through different opcodes and are + * left untouched. Functions with an explicitly-associated helper manager, and + * already-bound functions (arrow functions, `.bind()`ed functions), are unaffected. + * + * The parent's value is read here, inside the helper compute, so that replacing the + * base object rebinds `this` on the next revision. + */ +function bindFunctionToParent( + definition: HelperDefinitionState, + ref: Reference +): HelperDefinitionState { + if (typeof definition !== 'function' || hasCustomHelperManager(definition)) { + return definition; + } + + let parentRef = parentRefFor(ref); + + if (parentRef === null) { + return definition; + } + + let self = valueForRef(parentRef); + + if ((typeof self === 'object' && self !== null) || typeof self === 'function') { + return (definition as (this: unknown, ...args: unknown[]) => unknown).bind(self); + } + + return definition; +} + function resolveHelper(definition: HelperDefinitionState, ref: Reference): Helper { let managerOrHelper = getInternalHelperManager(definition, true); let helper;