Skip to content

fix: read the parent @computed through super before the override is read - #4749

Open
nk-batman21 wants to merge 2 commits into
mobxjs:mainfrom
nk-batman21:fix/computed-super-before-override-read
Open

nk-batman21 wants to merge 2 commits into
mobxjs:mainfrom
nk-batman21:fix/computed-super-before-override-read

Conversation

@nk-batman21

@nk-batman21 nk-batman21 commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

With 2022.3 decorators, super.x returns the subclass override instead of the parent value if it runs before the subclass's own @computed override of x was read on that instance. This is the case of the skipped test that #4661 added for #4660.

class Cart {
    @observable accessor price = 100
    @computed get total() { return this.price }
}

class DiscountedCart extends Cart {
    @computed override get total() { return super.total * 0.9 }
    @computed get savings() { return super.total - this.total }
}

const cart = new DiscountedCart()
cart.savings // 0, expected 10

In an autorun or observer that reads savings on a fresh instance, the wrong 0 is what gets cached, and it stays until price changes.

Since the computeds became lazy, the parent getter finds no entry in values_ yet and falls through to getObservablePropValue_, which materializes the last registered factory (the subclass one) and evaluates the override. When there is no entry yet and the running getter is not the one this[key] resolves to, which is the case for a super read, the getter now materializes the entry first, so the existing derivation !== get check sends the parent read to its own computed. A plain this.x read, including one in a parent constructor before the override is registered, takes the same path as before, and so does every read that already has an entry.

I un-skipped the test and shortened the comment above it, since it described the bug. Happy to keep the original comment if you prefer. Two more tests pin the parent constructor cases from the review: a read in the constructor returns the parent value, and an autorun started there follows the override once it is registered.

Code change checklist

  • Added/updated unit tests
  • Updated /docs. For new functionality, at least API.md should be updated (not needed for this fix)
  • Verified that there is no significant performance drop (npm -w mobx run test:performance)

Ran: the mobx suite (798 passed, 9 skipped; before: 795 and 10), tsc --noEmit, eslint and prettier on the changed files, npm run check-size and the lazy @computed benchmark. The mobx-react, mobx-react-lite, eslint-plugin-mobx and mobx-undecorate tests ran on the first commit.
Checked: mobx.cjs.production.min.js grows by 124 bytes (61 gzipped); the lazy @computed benchmark, run alternately against main, shows no difference beyond run-to-run noise.

With 2022.3 decorators, the parent getter wrapper only checked values_,
which is still empty while the subclass @computed override is lazy. It
then fell through to getObservablePropValue_, which materialized and
evaluated the subclass factory, so super.x returned the override value.

Materialize the entry before the derivation check, so the parent read
goes to its own computed. Un-skips the mobxjs#4660 test added in mobxjs#4661.
@changeset-bot

changeset-bot Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 50c95bc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
mobx Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@kubk

kubk commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

MobX documentation has an amazing advice regarding inheritance:

Try to keep things simple and prefer composition over inheritance.

https://mobx.js.org/subclassing.html

I'll leave a couple of examples that will produce a wrong result if the PR is merged, based on Claude's findings:

import { autorun, computed, observable, runInAction } from "../../src/mobx"

test("parent constructor reaction on an overridden @computed", () => {
    const log: number[] = []

    class Parent {
        @observable accessor price = 100

        constructor() {
            autorun(() => log.push(this.total))
        }

        @computed get total() {
            return this.price
        }
    }

    class Child extends Parent {
        @observable accessor discount = 0.5

        @computed override get total() {
            return super.total * this.discount
        }
    }

    const c = new Child()
    runInAction(() => (c.price = 2))
    runInAction(() => (c.discount = 1))
    expect(log).toEqual([100, 1, 2])
})

test("parent constructor reads an overridden @computed", () => {
    class Parent {
        @observable accessor price = 100
        initialTotal: number

        constructor() {
            this.initialTotal = this.total
        }

        @computed get total() {
            return this.price
        }
    }

    class Child extends Parent {
        @observable accessor discount = 0.5

        @computed override get total() {
            return super.total * this.discount
        }
    }

    expect(new Child().initialTotal).toBe(100)
})

@nk-batman21

Copy link
Copy Markdown
Contributor Author

@kubk thanks, both examples broke with the first version. 50c95bc narrows the change: the getter only materializes the override first when the running getter is not the one this[key] resolves to, which is the super read from #4660. A plain this.total read, including one in a parent constructor before the override is registered, takes the same path as on main, so your examples give 100 and [100, 1, 2] again. I added both as tests, and they fail on the previous commit. Agreed on composition over inheritance; this only keeps a super read of a still lazy override from returning the override.

This branch has not been deployed

No deployments
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.

2 participants