Enable plain methods to work on classes when called from templates#21341
Open
NullVoxPopuli wants to merge 12 commits intomainfrom
Open
Enable plain methods to work on classes when called from templates#21341NullVoxPopuli wants to merge 12 commits intomainfrom
NullVoxPopuli wants to merge 12 commits intomainfrom
Conversation
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) <[email protected]>
Address review: the fix is about path expression binding, not the on modifier specifically. Use a plain class + gjs template to verify that `foo.foo` invoked from a template preserves `this`. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Replace the runtime heuristic (isClassMethod/maybeBindPrototypeMethod in
childRefFor) with a compile-time approach as requested in review.
When the template compiler sees a `this.*` path (GetSymbol with sym=0),
it now emits VM_GET_PROPERTY_BOUND_OP for the last path segment instead
of VM_GET_PROPERTY_OP. The runtime handler for this opcode creates a
reference that auto-binds function values to their parent via .bind(),
with caching for stable identity across revalidations.
Changes:
- New opcode: VM_GET_PROPERTY_BOUND_OP (113) in interfaces, constants,
and debug metadata
- Opcode compiler: GetSymbol handler emits bound opcode for last segment
of this.* paths
- Runtime: boundChildRefFor() creates a cached bound reference, skipping
binding for values with component or modifier managers
- Removed: runtime heuristics from @glimmer/reference (isClassMethod,
maybeBindPrototypeMethod, etc.)
- Tests: classic Component with class method via {{on "click" this.foo}},
and nested this.obj.method chain
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Instead of eagerly calling getProp in the bound ref's compute function (which created parallel tracking and autotracking issues), the bound ref now returns a stable wrapper function that defers the property read to call time: parent[path].call(parent, ...args). This means the property is only tracked through childRefFor's existing tracking (for typeof check), and the actual method lookup happens when the function is invoked, not during render. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Pass a bindTransform callback to childRefFor so that binding happens inside the same ref — same tracking, same caching, same debug labels. Eliminates the wrapper ComputeRef that was causing autotracking divergence and identity issues. The transform wraps function values in a stable callable that defers property lookup to invocation time. Non-function values (strings, objects, components, modifiers) pass through unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Transform: invalidate wrapper cache when underlying value changes, so on/fn modifiers detect callback swaps correctly - Transform: skip wrapping functions with own enumerable keys, so each-in can iterate function properties - Update fn/on contract tests: the old "no this context" and "untouchable context" assertions now verify that this IS preserved - Update debug-render-tree: use predicate for on-modifier args since the callback identity changes with auto-binding - Remove unused DEBUG import from fn-test.js Dev: 9140 pass / 0 fail / 17 skip Prod: 8945 pass / 0 fail / 52 skip Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
CallableFunction's typed .call() method doesn't accept arbitrary this contexts. Use Reflect.apply with a typeof guard instead. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- this-binding-test.gjs: use strict mode gjs templates, named classes, constructors (not init), no register calls - fn-test.js: extract context to variable, assert seenThis === context to show that this is the defining object (not just non-null) - Revert debug-render-tree-test.ts to original — the render tree should retain original values, this test should not change - Restore hasDefinitionManager check with detailed comment explaining why it's needed (WeakMap-based manager lookups break without it) The debug-render-tree modifier test still fails (1 failure) because the bindTransform wrapper changes function identity for arrow functions passed via this.* paths. This is a design tension: the wrapper is needed for class methods but changes identity for all functions. Need reviewer input on the right boundary. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Instead of wrapping function values in a callable (which changes identity and breaks render tree inspection, modifier WeakMap lookups, etc.), tag the ref with its parent via a WeakMap. The original value flows through unchanged — valueForRef returns the original function. Consumers that invoke callbacks (on modifier, fn helper) check getBindingParentRef(ref) and bind `this` at invocation time: - on: callback.bind(parentValue) instead of untouchableContext - fn: fn.call(parentValue, ...) instead of fn.call(context, ...) This eliminates: - hasDefinitionManager check (no identity change = no WeakMap issue) - Object.keys check (no wrapper = no property loss) - BOUND_FN_CACHE (no wrapper to cache) - bindTransform / ChildRefTransform (no value transformation) - debug-render-tree test changes (original values preserved) Dev: 9140 pass / 0 fail / 17 skip Prod: 8945 pass / 0 fail / 52 skip Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
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]>
- on.ts comment: "this.*" → "path read" since binding applies to all paths now - fn-test.ts: assert seenThis has expected property (arg1 === 'foo') instead of just non-null, matching reviewer feedback - expressions.ts: alphabetize imports Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Iterates over an array of Item instances with {{#each}}, passes
item.greet to {{on "click"}} for each, and asserts that each
handler fires with the correct Item as this.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
4 tasks
Contributor
📊 Package size report 0.08%↑
🤖 This report was automatically generated by pkg-size-action |
kategengler
reviewed
May 1, 2026
Member
kategengler
left a comment
kategengler
left a comment
There was a problem hiding this comment.
Is there a corresponding RFC? Or other way we make sure this gets documented?
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #21315
Demo: https://test-ember-source-fix-this-b.limber-glimdown.pages.dev/edit?c=JYWwDg9gTgLgBAYQuCA7Apq%2BAzKy4DkAAgOYA2oI6UA9AMbKQZYEDcAUKJLHAN5wwoAQzoBrdABM4AXzi58xcpWo1BI0cFQk2nFD35oZcvCEJF0IAEYqQECcGzBqO9ugAe3eBPTYhAVzJ4OjIhAGdQuAAJdDIyCAB1aDIpdxhMCQikFGZ4XnY4OCI1MUk4Bj8sOABeOAAGDny4TTooC0wYAAoASj5GgpgAC2BQgDpyyoBqGoBGDgLpdkaAHjTwELSAPj64JbANgE0IPzgBoQA3dDKKEqlBy8s-GBhDXl5B4bGjrGlZGFB0UZLGh7RYFApLB5PQwwACeYHQVQARJDnqhEXxeIZEcFgGJ0e9Rs1WlRvtINghrqIgSi0FtwaoLGB1ugtgsgA&format=gjs
Previous description:
Summary
When a template path like
this.fooresolves to a function on the component, auto-bind it so thatthisis preserved when the function is invoked as a callback (e.g. via{{on "click" this.foo}}). No@actiondecorator, arrow-function field, or manual.bind()needed.Implementation: compile-time VM_GET_PROPERTY_BOUND_OP
Instead of runtime heuristics in
childRefFor(the first iteration), this now uses a compile-time approach with a new VM opcode:VM_GET_PROPERTY_BOUND_OP(113) — same stack behavior asVM_GET_PROPERTY_OPbut creates a bound referenceGetSymbolseessym === 0(=this) with a path, emitsVM_GET_PROPERTY_BOUND_OPfor the last segment. Intermediate segments use normalVM_GET_PROPERTY_OP. This means:this.foo→GetVariable(0)+GetPropertyBound("foo")this.obj.method→GetVariable(0)+GetProperty("obj")+GetPropertyBound("method")boundChildRefFor(parentRef, path)reads the property and, if the value is a function (without a component/modifier manager), returns a cached.bind(parent)version. Uses.bind()for V8's optimized BoundFunction path. Non-function values pass through unchanged.<this.dynamicComponent>and{{this.modifier}}still work). Helper managers are excluded from the check because ALL functions have a default helper manager per RFC #756.