Skip to content

Fix class setters invocation on nested x-data#4528

Merged
calebporzio merged 4 commits into
alpinejs:mainfrom
helio3197:fix/class-setters-calls
Feb 9, 2026
Merged

Fix class setters invocation on nested x-data#4528
calebporzio merged 4 commits into
alpinejs:mainfrom
helio3197:fix/class-setters-calls

Conversation

@helio3197

Copy link
Copy Markdown
Contributor

The current implementation of the set trap in mergeProxies restricts key lookup to the target's own property keys. This prevents class setters defined in the target's prototype from being called when there is more than one object in the x-data stack.

This allows components like this to work:

<script>
  class BaseHandler {
    _prop = 'original'
  
    get prop() {
      return this._prop
    }
  
    set prop(v)  {
      this._prop = v
    }
  }
  window.baseHandler = () => new BaseHandler()
</script>

<div x-data>
  <div x-data="baseHandler()">
    <button x-text="prop" @click="prop = 'edited'"></button>
  </div>
</div>

@ekwoka

ekwoka commented Feb 12, 2025

Copy link
Copy Markdown
Contributor

I'm not sure how the test demonstrates the issue.

You have code related to prototype chain but the test doesn't use the prototype chain at all.

@helio3197

Copy link
Copy Markdown
Contributor Author

It does, because the setter function is defined in the class instance's prototype not in the instance object itself, then when the setter is invoked e.g. value = 'somevalue' the current proxy's set trap won't be able to set the property value, because there is a check asserting that the invoked key must be an own property of the target (Object.prototype.hasOwnProperty.call(obj, name)) which evaluates to false in this case because the property itself is a getter and setter defined in the class instance prototype.

You can check the live issue in this codepen, and here is the same example with the implemented change.

@ekwoka

ekwoka commented Feb 13, 2025

Copy link
Copy Markdown
Contributor

Okay, and it bypasses the vue reactivity bug since it's a class setter and not own setter?

@helio3197

Copy link
Copy Markdown
Contributor Author

No, I had not considered the scenario described by this PR, therefore referencing other parent data-stack objects will result in error. However, I've modified the code to use the prototype, where the setter is defined as the 'target'. Thus, this check can pass, and the setter method can be called with the mergeProxy as the this value.

const descriptor = Object.getOwnPropertyDescriptor(target, name);
        if (descriptor?.set && descriptor?.get)

calebporzio and others added 2 commits February 8, 2026 21:49
Stop the prototype walk at Object.prototype to prevent accidentally
matching built-in properties (toString, valueOf, etc.) which could
lead to setting properties on Object.prototype instead of the
intended data stack fallback target.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@calebporzio

Copy link
Copy Markdown
Collaborator

PR Review: #4528 — Fix class setters invocation on nested x-data

Type: Bug fix
Verdict: Merge

What's happening (plain English)

When you use a class as your x-data component and that class defines getter/setter pairs (like get value() / set value(v)), those accessors live on the class prototype, not on the instance itself.

Here's the sequence that triggers the bug:

  1. You have nested x-data elements: an outer <div x-data> and an inner <div x-data="handler()"> where handler() returns a class instance.
  2. The data stack has two objects: [classInstance, outerScopeObject].
  3. When you write value = 'foo' in an Alpine expression, the proxy's set trap fires.
  4. The set trap loops through the data stack looking for which object owns value.
  5. The bug: it used hasOwnProperty to check — which only looks at own properties. Since get value()/set value() live on BaseHandler.prototype, not on the instance, hasOwnProperty returns false.
  6. No match is found, so it falls through to the last object in the stack (the outer scope).
  7. A plain value property gets created on the outer scope instead of invoking the class setter. The setter never runs.

This only breaks when there are multiple objects in the data stack (nested x-data). With a single x-data, the fallback objects[objects.length - 1] happens to be the class instance itself, so it accidentally works.

Other approaches considered

  1. Use Reflect.has() instead of hasOwnProperty in the set trap — Would detect prototype properties but only returns a boolean, not the actual prototype object where the descriptor lives. We need the specific object to call Object.getOwnPropertyDescriptor(target, name) and find the setter.
  2. Use name in obj + separate prototype walk — Functionally equivalent to the PR's approach but split across two steps. No simpler.
  3. The PR's approach: keyInPrototypeChain recursive helper — Walks up the prototype chain and returns the object that owns the key. This is the cleanest solution because it combines "does this object have the key?" and "give me the right target for the descriptor" in one step.

The PR's approach is the right one.

Changes Made

I pushed one fix: guard keyInPrototypeChain against Object.prototype. The original implementation would walk all the way up to Object.prototype, which means properties like toString, valueOf, hasOwnProperty, etc. would be found there instead of falling through to the intended fallback (objects[objects.length - 1]). In theory, Reflect.set(Object.prototype, 'toString', value) could pollute the global prototype. The fix adds an early return when reaching Object.prototype:

// Before (from PR)
function keyInPrototypeChain(obj, key) {
    if (Object.prototype.hasOwnProperty.call(obj, key)) return obj
    const proto = Object.getPrototypeOf(obj)
    return proto && keyInPrototypeChain(proto, key)
}

// After (with guard)
function keyInPrototypeChain(obj, key) {
    if (obj === null || obj === Object.prototype) return null
    if (Object.prototype.hasOwnProperty.call(obj, key)) return obj
    return keyInPrototypeChain(Object.getPrototypeOf(obj), key)
}

Test Results

  • scope.spec.js: 9/9 passing (7 existing + 2 new)
  • Regression verified: both new tests fail without the fix, pass with it
  • All 7 existing tests continue to pass (no regressions)
  • CI: build passing

Code Review

  • packages/alpinejs/src/scope.js:40-45: The keyInPrototypeChain helper is clean and minimal. Recursion depth is bounded by prototype chain length (typically 2-3 levels for class instances). No performance concern.
  • packages/alpinejs/src/scope.js:75-81: The set trap change is surgical — only the target lookup logic changed. The existing descriptor?.set && descriptor?.get check and the Vue reactivity workaround are untouched.
  • tests/cypress/integration/scope.spec.js:161-233: Both tests are well-structured. The first tests the core bug (class setter + nested x-data). The second tests the interaction with parent scope access through this in the setter (covering the Vue reactivity workaround path). Naming is clear and descriptive.
  • The get trap already uses Reflect.has() which walks prototypes — this fix brings the set trap into parity. The asymmetry was the root cause.

Security

The Object.prototype guard I added prevents a theoretical prototype pollution vector. No other security concerns identified.

Verdict

This is a legitimate bug with a clean, minimal fix. The contributor correctly identified the root cause (get/set asymmetry in prototype chain handling), provided two well-targeted tests, and handled the Vue reactivity workaround interaction. The discussion in the comments shows they iterated thoughtfully after @ekwoka's feedback. I've added one safety guard to prevent prototype pollution via Object.prototype. Recommend merge.


Reviewed by Claude

@calebporzio
calebporzio merged commit 9b0e995 into alpinejs:main Feb 9, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants